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:

> 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.
> 

I used `-1` while calling `prefHeight` as I tried to make `computePrefHeight` in consistent with `computePrefWidth` method. But looking at the documentation in Node.java file, it looks like width should be used. So I made modifications to use width while calling `prefHeight`.


> 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 carried forward the conditions which were already present. I believe removing the condition like `graphic == null` or `txt == null` from `isIgnoreGraphic()` or `isIgnoreText()` methods will create more combinations of conditions combined with ContentDisplay value during height calculation and we have to consider which all the height components to be considered in these cases. Since `graphic==null` is as good as considering  ContentDisplay.TEXT_ONLY, I feel it is better to keep this check.

I have updated the code to use `graphicHeight` in other cases of if/else branch.

> I also wonder if a simple fix like this would have the same result:
> 
> ```diff
> -       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());
> ```

This fixes only the issue of height calculation when text height needs to be ignored. But the original if/else branch present, considers text gap when not required. So I believe modification to the if/else branch is necessary.

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

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