On Mon, 16 Jan 2023 10:46:12 GMT, Karthik P K <k...@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
