RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY

[John Hendrikx]

On Mon, 16 Jan 2023 10:46:12 GMT, Karthik P K <kpk at openjdk.org> wrote:

> Ignore text condition was not considered in `computePrefHeight` method while calculating the Label height.
> 
> Added `isIgnoreText` condition in `computePrefHeight` method while calculating Label height.
> 
> Tests are present for all the ContentDisplay types. Modified tests which were failing because of the new height value getting calculated.

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());

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

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