RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]

[Karthik P K]

On Tue, 17 Jan 2023 11:40:47 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use width while calling prefHeight method. Use graphicHeight instead of multiple calls to prefHeight
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 393:
> 
>> 391:             height = graphic.prefHeight(-1) + gap + textHeight;
>> 392:         } else {
>> 393:             height = Math.max(textHeight, graphic.prefHeight(-1));
> 
> The logic of this code seems to have changed more than just the fixing bug. Why are the `prefHeight` functions now called with `-1`?  Normally these should respect the content bias of the node involved.
> 
> Also, I notice that you check `graphic == null`, while the `isIgnoreGraphic` function checks quite a bit more:
> 
>     boolean isIgnoreGraphic() {
>         return (graphic == null ||
>                 !graphic.isManaged() ||
>                 getSkinnable().getContentDisplay() == ContentDisplay.TEXT_ONLY);
>     }
> 
> Doing all those operations on `graphic` when for example the `ContentDisplay` is `TEXT_ONLY` seems unnecessary.  Looking at the rest of the code, `graphicHeight` is only used in one branch in your if/elseif/elseif/else.
> 
> I also wonder if a simple fix like this would have the same result:
> 
> 
> -       final double textHeight = Utils.computeTextHeight(font, cleanText,
> +       final double textHeight = isIgnoreText() ? 0.0 : Utils.computeTextHeight(font, cleanText,
>                  labeled.isWrapText() ? textWidth : 0,
>                  labeled.getLineSpacing(), text.getBoundsType());

@hjohn please take a look and review the changes made.

-------------

PR: https://git.openjdk.org/jfx/pull/996