8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text

hjohn · hjohn · commit 398f104d6ba7 · 2024-04-28T18:22:23.000Z
Reviewed-by: kpk, angorya
diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLine.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLine.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,9 +27,12 @@
 
 import com.sun.javafx.geom.RectBounds;
 
+/**
+ * Represents a full line of text that is all rendered on a single line.
+ */
 public interface TextLine {
     /**
-     * Returns the list of GlyphList in the line. The list is visually orderded.
+     * Returns the list of GlyphList in the line. The list is visually ordered.
      */
     public GlyphList[] getRuns();
 
diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextSpan.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextSpan.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,6 +27,13 @@
 
 import com.sun.javafx.geom.RectBounds;
 
+/**
+ * Represents a sequence of characters all using the same font, or
+ * an embedded object if no font is supplied.
+ * <p>
+ * A text span can contain line breaks if the text should span multiple
+ * lines.
+ */
 public interface TextSpan {
     /**
      * The text for the span, can be empty but not null.
diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
@@ -930,8 +930,11 @@ private void shape(TextRun run, char[] chars, GlyphLayout layout) {
         }
     }
 
-    private TextLine createLine(int start, int end, int startOffset) {
+    private TextLine createLine(int start, int end, int startOffset, float collapsedSpaceWidth) {
         int count = end - start + 1;
+
+        assert count > 0 : "number of TextRuns in a TextLine cannot be less than one: " + count;
+
         TextRun[] lineRuns = new TextRun[count];
         if (start < runCount) {
             System.arraycopy(runs, start, lineRuns, 0, count);
@@ -948,11 +951,43 @@ private TextLine createLine(int start, int end, int startOffset) {
             leading = Math.max(leading, run.getLeading());
             length += run.getLength();
         }
+
+        width -= collapsedSpaceWidth;
+
         if (width > layoutWidth) layoutWidth = width;
         return new TextLine(startOffset, length, lineRuns,
                             width, ascent, descent, leading);
     }
 
+    /**
+     * Computes the size of the white space trailing a given run.
+     *
+     * @param run the run to compute trailing space width for, cannot be {@code null}
+     * @return the X size of the white space trailing the run
+     */
+    private float computeTrailingSpaceWidth(TextRun run) {
+        float trailingSpaceWidth = 0;
+        char[] chars = getText();
+
+        /*
+         * As the loop below exits when encountering a non-white space character,
+         * testing each trailing glyph in turn for white space is safe, as white
+         * space is always represented with only a single glyph:
+         */
+
+        for (int i = run.getGlyphCount() - 1; i >= 0; i--) {
+            int textOffset = run.getStart() + run.getCharOffset(i);
+
+            if (!Character.isWhitespace(chars[textOffset])) {
+                break;
+            }
+
+            trailingSpaceWidth += run.getAdvance(i);
+        }
+
+        return trailingSpaceWidth;
+    }
+
     private void reorderLine(TextLine line) {
         TextRun[] runs = line.getRuns();
         int length = runs.length;
@@ -1059,6 +1094,94 @@ private float getTabAdvance() {
         return tabSize * spaceAdvance;
     }
 
+    /*
+     * The way JavaFX lays out text:
+     *
+     * JavaFX distinguishes between soft wraps and hard wraps. Soft wraps
+     * occur when a wrap width has been set and the text requires wrapping
+     * to stay within the set wrap width. Hard wraps are explicitly part of
+     * the text in the form of line feeds (LF) and carriage returns (CR).
+     * Hard wrapping considers a singular LF or CR, or the combination of
+     * CR+LF (or LF+CR) as a single wrap location. Hard wrapping also occurs
+     * between TextSpans when multiple TextSpans were supplied (for wrapping
+     * purposes, there is no difference between two TextSpans and a single
+     * TextSpan where the text was concatenated with a line break in between).
+     *
+     * Soft wrapping occurs when a wrap width has been set. This occurs at
+     * the first character that does not fit.
+     *
+     * - If that character is not a white space, the break is set immediately
+     *   after the first white space encountered before that character
+     *   - If there is no white space before the preferred break character, the
+     *     break is done at the first character that does not fit (the wrap
+     *     then occurs in the middle of a (long) word)
+     * - If the preferred break character is white space, and it is followed by
+     *   more white space, the break is moved to the end of the white space (thus
+     *   a break in white space always occurs at first non white space character
+     *   following a white space sequence)
+     *
+     * White space collapsing:
+     *
+     * Only white space that is present at soft wrapped locations is collapsed to
+     * zero. Any other white space is preserved. This includes white space between
+     * words, leading and trailing white space, and white space around hard wrapped
+     * locations.
+     *
+     * Alignment:
+     *
+     * The alignment calculation only looks at the width of all the significant
+     * characters in each line. Significant characters are any non white space
+     * characters and any white space that has been preserved (white space that wasn't
+     * collapsed due to soft wrapping).
+     *
+     * Alignment does not take text effects, such as strike through and underline, into
+     * account. This means that such effects can appear unaligned. Trailing spaces at a
+     * soft wrap location (that are underlined for example), may show the underline go
+     * outside the logical bounds of the text.
+     *
+     * Example, where <SW> indicates a soft wrap location, and <LF> is a line feed:
+     *
+     *     "   The   quick <SW>brown fox jumps <SW> over the <LF> lazy dog   "
+     *
+     * Would be rendered as (left aligned):
+     *
+     *     "   The   quick"
+     *     "brown fox jumps"
+     *     "over the "
+     *     " lazy dog   "
+     *
+     * The alignment calculation uses the above bounds indicated by the double
+     * quotes, and so right aligned text would look like:
+     *
+     *      "   The   quick"
+     *     "brown fox jumps"
+     *           "over the "
+     *        " lazy dog   "
+     *
+     * Note that only the white space at the soft wrap locations is collapsed.
+     * In all other locations the space was preserved (the space between words
+     * where no soft wrap occurred, the leading and trailing space, and the
+     * space around the hard wrapped location).
+     *
+     * Text effects have no effect on the alignment, and so with underlining on
+     * the right aligned text would look like:
+     *
+     *      "___The___quick_"     (one collapsed space becomes visible here)
+     *     "brown_fox_jumps__"    (two collapsed spaces become visible here)
+     *           "over_the_"
+     *        "_lazy_dog___"
+     *
+     * Note that text alignment has not changed at all, but the bounds are exceeded
+     * in some locations to allow for the underline. Controls displaying such texts
+     * will likely clip the underlined parts exceeding the bounds.
+     *
+     * Users wishing to mitigate some of these perhaps surprising results can ensure
+     * they use trimmed texts, and avoid the use of line breaks, or at least ensure
+     * that line breaks are not preceded or succeeded by white space (activating
+     * line wrapping is not equivalent to collapsing any consecutive white space
+     * no matter where it occurs).
+     */
+
     private void layout() {
         /* Try the cache */
         initCache();
@@ -1126,17 +1249,22 @@ private void layout() {
                 /* Find offset of the first character that does not fit on the line */
                 int hitOffset = run.getStart() + run.getWrapIndex(wrapWidth - lineWidth);
 
-                /* Only keep whitespaces (not tabs) in the current run to avoid
+                /*
+                 * Only keep white spaces (not tabs) in the current run to avoid
                  * dealing with unshaped runs.
+                 *
+                 * If the run is a tab, the run will be always of length 1 (see
+                 * buildRuns()). As there is no "next" character that can be selected
+                 * as the wrap index in this run, the white space skipping logic
+                 * below won't skip tabs.
                  */
+
                 int offset = hitOffset;
                 int runEnd = run.getEnd();
-                while (offset + 1 < runEnd && chars[offset] == ' ') {
+
+                // Don't take white space into account at the preferred wrap index:
+                while (offset + 1 < runEnd && Character.isWhitespace(chars[offset])) {
                     offset++;
-                    /* Preserve behaviour: only keep one white space in the line
-                     * before wrapping. Needed API to allow change.
-                     */
-                    break;
                 }
 
                 /* Find the break opportunity */
@@ -1233,7 +1361,7 @@ private void layout() {
 
             lineWidth += runWidth;
             if (run.isBreak()) {
-                TextLine line = createLine(startIndex, i, startOffset);
+                TextLine line = createLine(startIndex, i, startOffset, computeTrailingSpaceWidth(runs[i]));
                 linesList.add(line);
                 startIndex = i + 1;
                 startOffset += line.getLength();
@@ -1242,11 +1370,11 @@ private void layout() {
         }
         if (layout != null) layout.dispose();
 
-        linesList.add(createLine(startIndex, runCount - 1, startOffset));
+        linesList.add(createLine(startIndex, runCount - 1, startOffset, 0));
         lines = new TextLine[linesList.size()];
         linesList.toArray(lines);
 
-        float fullWidth = Math.max(wrapWidth, layoutWidth);
+        float fullWidth = wrapWidth > 0 ? wrapWidth : layoutWidth;  // layoutWidth = widest line, wrapWidth is user set
         float lineY = 0;
         float align;
         if (isMirrored()) {
@@ -1263,7 +1391,8 @@ private void layout() {
             RectBounds bounds = line.getBounds();
 
             /* Center and right alignment */
-            float lineX = (fullWidth - bounds.getWidth()) * align;
+            float unusedWidth = fullWidth - bounds.getWidth();
+            float lineX = unusedWidth * align;
             line.setAlignment(lineX);
 
             /* Justify */
@@ -1281,7 +1410,7 @@ private void layout() {
                         if (hitChar && chars[j] == ' ') wsCount++;
                     }
                     if (wsCount != 0) {
-                        float inc = (fullWidth - bounds.getWidth()) / wsCount;
+                        float inc = unusedWidth / wsCount;
                         done:
                         for (int j = 0; j < lineRunCount; j++) {
                             TextRun textRun = lineRuns[j];
diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextLine.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextLine.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,11 +28,13 @@
 import com.sun.javafx.geom.RectBounds;
 
 public class TextLine implements com.sun.javafx.scene.text.TextLine {
-    TextRun[] runs;
-    RectBounds bounds;
-    float lsb, rsb, leading;
-    int start;
-    int length;
+    private final TextRun[] runs;
+    private final RectBounds bounds;
+    private final int start;
+    private final int length;
+    private final float leading;
+
+    private float lsb, rsb;
 
     public TextLine(int start, int length, TextRun[] runs,
                     float width, float ascent, float descent, float leading) {
diff --git a/modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java b/modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -343,6 +343,19 @@ public int getWrapIndex(float width) {
     }
 
     public float getAdvance(int glyphIndex) {
+
+        /*
+         * When positions is null it means that the TextRun only contains
+         * a line break, assuming that the class is used correctly ("shape"
+         * must be called before calling this method, unless the class user is
+         * sure that the run is empty). This class could benefit from better
+         * encapsulation to make it easier to reason about.
+         */
+
+        if (positions == null) {
+            return 0;
+        }
+
         if ((flags & FLAGS_COMPACT) != 0) {
             return positions[start + glyphIndex];
         } else {
diff --git a/modules/javafx.graphics/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java b/modules/javafx.graphics/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java
diff --git a/tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java b/tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java
