Skip to content

Commit

Permalink
8230833: LabeledSkinBase computes wrong height with ContentDisplay.GR…
Browse files Browse the repository at this point in the history 
…APHIC_ONLY

Reviewed-by: aghaisas, jhendrikx, angorya
  • Loading branch information
@karthikpandelu @aghaisas
karthikpandelu authored and aghaisas committed Feb 10, 2023
1 parent e7d3191 commit d3654e3
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 57 deletions.
93 changes: 52 additions & 41 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
View file
Expand Up @@ -315,35 +315,42 @@ protected void updateChildren() {
@Override protected double computePrefWidth(double height, double topInset, double rightInset, double bottomInset, double leftInset) {
// Get the preferred width of the text
final Labeled labeled = getSkinnable();
final Font font = text.getFont();
String cleanText = getCleanText();
boolean emptyText = cleanText == null || cleanText.isEmpty();

boolean isIgnoreText = isIgnoreText();
double widthPadding = leftInset + rightInset;

if (!isIgnoreText()) {
widthPadding += leftLabelPadding() + rightLabelPadding();
}
double txWidth;
if (isIgnoreText) {
txWidth = 0.0;
} else {
widthPadding += (leftLabelPadding() + rightLabelPadding());

double textWidth = 0.0;
if (!emptyText) {
textWidth = Utils.computeTextWidth(font, cleanText, 0);
String cleanText = getCleanText();
boolean emptyText = cleanText == null || cleanText.isEmpty();
txWidth = emptyText ? 0.0 : Utils.computeTextWidth(text.getFont(), cleanText, 0);
}

// Fix for RT-39889
double graphicWidth = graphic == null ? 0.0 :
Utils.boundedSize(graphic.prefWidth(-1), graphic.minWidth(-1), graphic.maxWidth(-1));

// Now add on the graphic, gap, and padding as appropriate
double width;
if (isIgnoreGraphic()) {
return textWidth + widthPadding;
} else if (isIgnoreText()) {
return graphicWidth + widthPadding;
} else if (labeled.getContentDisplay() == ContentDisplay.LEFT
|| labeled.getContentDisplay() == ContentDisplay.RIGHT) {
return textWidth + labeled.getGraphicTextGap() + graphicWidth + widthPadding;
width = txWidth;
} else {
return Math.max(textWidth, graphicWidth) + widthPadding;
// Fix for RT-39889
double graphicWidth = graphic == null ? 0.0 :
Utils.boundedSize(graphic.prefWidth(-1), graphic.minWidth(-1), graphic.maxWidth(-1));

if (isIgnoreText) {
width = graphicWidth;
} else {
ContentDisplay contentDisplay = labeled.getContentDisplay();
if (contentDisplay == ContentDisplay.LEFT || contentDisplay == ContentDisplay.RIGHT) {
width = txWidth + labeled.getGraphicTextGap() + graphicWidth;
} else {
width = Math.max(txWidth, graphicWidth);
}
}
}

return width + widthPadding;
}

/** {@inheritDoc} */
Expand All @@ -352,11 +359,14 @@ protected void updateChildren() {
final Font font = text.getFont();
final ContentDisplay contentDisplay = labeled.getContentDisplay();
final double gap = labeled.getGraphicTextGap();
boolean isIgnoreText = isIgnoreText();
boolean isIgnoreGraphic = isIgnoreGraphic();

width -= leftInset + rightInset;

if (!isIgnoreText()) {
double padding = topInset + bottomInset;
if (!isIgnoreText) {
width -= leftLabelPadding() + rightLabelPadding();
padding += topLabelPadding() + bottomLabelPadding();
}

String cleanText = getCleanText();
Expand All @@ -365,35 +375,36 @@ protected void updateChildren() {
cleanText = cleanText.substring(0, cleanText.length() - 1);
}

double textWidth = width;
if (!isIgnoreGraphic() &&
double txWidth = width;
if (!isIgnoreGraphic &&
(contentDisplay == LEFT || contentDisplay == RIGHT)) {
textWidth -= (graphic.prefWidth(-1) + gap);
txWidth -= (graphic.prefWidth(-1) + gap);
}

// TODO figure out how to cache this effectively.
final double textHeight = Utils.computeTextHeight(font, cleanText,
labeled.isWrapText() ? textWidth : 0,
labeled.isWrapText() ? txWidth : 0,
labeled.getLineSpacing(), text.getBoundsType());

// Now we want to add on the graphic if necessary!
double h = textHeight;
if (!isIgnoreGraphic()) {
final Node graphic = labeled.getGraphic();
if (contentDisplay == TOP || contentDisplay == BOTTOM) {
h = graphic.prefHeight(width) + gap + textHeight;
double height;
if (isIgnoreGraphic) {
height = textHeight;
} else {
// Calculate the graphic height and use based on contentDisplay value
double graphicHeight = graphic == null ? 0.0 :
Utils.boundedSize(graphic.prefHeight(width), graphic.minHeight(width), graphic.maxHeight(width));

// Add the graphic, gap, and padding as appropriate
if (isIgnoreText) {
height = graphicHeight;
} else if (contentDisplay == TOP || contentDisplay == BOTTOM) {
height = graphicHeight + gap + textHeight;
} else {
h = Math.max(textHeight, graphic.prefHeight(width));
height = Math.max(textHeight, graphicHeight);
}
}

double padding = topInset + bottomInset;

if (!isIgnoreText()) {
padding += topLabelPadding() + bottomLabelPadding();
}

return h + padding;
return height + padding;
}

/** {@inheritDoc} */
Expand Down
24 changes: 8 additions & 16 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
View file
Expand Up @@ -1249,8 +1249,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.TOP);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.prefHeight(-1), 0);
assertEquals(14 + 23, label.prefHeight(-1), 0);
}

@Test public void whenTextIsEmptyAndGraphicIsSetWithTOPContentDisplay_computePrefHeight_ReturnsRightAnswer() {
Expand All @@ -1260,8 +1259,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.TOP);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.prefHeight(-1), 0);
assertEquals(14 + 23, label.prefHeight(-1), 0);
}

@Test public void whenTextIsSetAndGraphicIsSetWithTOPContentDisplay_computePrefHeight_ReturnsRightAnswer() {
Expand Down Expand Up @@ -1312,8 +1310,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.BOTTOM);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.prefHeight(-1), 0);
assertEquals(14 + 23, label.prefHeight(-1), 0);
}

@Test public void whenTextIsEmptyAndGraphicIsSetWithBOTTOMContentDisplay_computePrefHeight_ReturnsRightAnswer() {
Expand All @@ -1323,8 +1320,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.BOTTOM);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.prefHeight(-1), 0);
assertEquals(14 + 23, label.prefHeight(-1), 0);
}

@Test public void whenTextIsSetAndGraphicIsSetWithBOTTOMContentDisplay_computePrefHeight_ReturnsRightAnswer() {
Expand Down Expand Up @@ -1807,8 +1803,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.TOP);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.maxHeight(-1), 0);
assertEquals(14 + 23, label.maxHeight(-1), 0);
}

@Test public void whenTextIsEmptyAndGraphicIsSetWithTOPContentDisplay_computeMaxHeight_ReturnsRightAnswer() {
Expand All @@ -1818,8 +1813,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.TOP);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.maxHeight(-1), 0);
assertEquals(14 + 23, label.maxHeight(-1), 0);
}

@Test public void whenTextIsSetAndGraphicIsSetWithTOPContentDisplay_computeMaxHeight_ReturnsRightAnswer() {
Expand Down Expand Up @@ -1870,8 +1864,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.BOTTOM);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.maxHeight(-1), 0);
assertEquals(14 + 23, label.maxHeight(-1), 0);
}

@Test public void whenTextIsEmptyAndGraphicIsSetWithBOTTOMContentDisplay_computeMaxHeight_ReturnsRightAnswer() {
Expand All @@ -1881,8 +1874,7 @@ public class LabelSkinTest {
label.setGraphicTextGap(2);
label.setGraphic(r);
label.setContentDisplay(ContentDisplay.BOTTOM);
final double lineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
assertEquals(14 + 23 + lineHeight + 2, label.maxHeight(-1), 0);
assertEquals(14 + 23, label.maxHeight(-1), 0);
}

@Test public void whenTextIsSetAndGraphicIsSetWithBOTTOMContentDisplay_computeMaxHeight_ReturnsRightAnswer() {
Expand Down

0 comments on commit d3654e3

Please sign in to comment.