1850620 - On some lines the last character gets wrapped to a new line when used with 'white-space' CSS rule

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Benjamin Neff [:supertux88]]

I sadly wasn't able to create a minimal example to reproduce this, as it doesn't happen always, and I don't exactly know what triggers it. But it happens a lot in concourse build logs (see for example here).

There is enough space on these lines, so there is no need for them to break to two lines on the last character. There is a CSS rule white-space: break-spaces; (and white-space: pre-wrap;). When I disable these rules in the dev-tools, the unnecessary linebreak doesn't happen anymore (but also the wanted breaks when the line is too long doesn't happen anymore). So I assume it has something to do with the white-space CSS rule, but since it doesn't happen on all the lines, there must also be something else that causes it.

Mozregression pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6d877fdb9a1e892fe6528a26aab81b53cfae55c5&tochange=d6760db62dc1cc0831c09c42f086cf57d1c2bf48

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:m_kato, since you are the author of the regressor, bug 1719535, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1719535

Comment 3

[Brian Birtles (:birtles)]

Kato-san, are you going to take this?

Comment 4

[Maire Reavy [:mreavy]]

Doing a platform-wide re-triage of all S2 bugs: this looks to be an S3 (normal bug that we really want to fix, not a major one likely to cause lots of user churn). Please chime in and needinfo me if this assessment is wrong. Thanks!

Comment 5

[Makoto Kato [:m_kato]]

Attachment: testcase

Comment 6

[Makoto Kato [:m_kato]]

Attachment: testcase

Comment 7

[Makoto Kato [:m_kato]]

Ting-Yu, this issue is that this is one of compatibility issue of legacy segmenter vs new segmenter. UAX#14 doesn't break before LF, but legacy segmenter breaks it. This issue occurs by this different.

It seems that last LF isn't collapsed/trimed with white-space: pre-wrap in layout side. Where do we collapse space and CRLF to calculate min-content?

Of course, we will be able to fix this by segmenter side like the following. I don't know which (segmenter side or layout side) is better.

diff --git a/dom/base/nsLineBreaker.cpp b/dom/base/nsLineBreaker.cpp
--- a/dom/base/nsLineBreaker.cpp
+++ b/dom/base/nsLineBreaker.cpp
@@ -158,16 +158,25 @@ nsresult nsLineBreaker::FlushCurrentWord
     // is non-breakable. (See the comment of kNonBreakableASCII).
     memset(breakState.Elements(),
            gfxTextRun::CompressedGlyph::FLAG_BREAK_TYPE_NONE,
            length * sizeof(uint8_t));
   } else {
     LineBreaker::ComputeBreakPositions(
         mCurrentWord.Elements(), length, mWordBreak, mLineBreak,
         mScriptIsChineseOrJapanese, breakState.Elements());
+    // Collapsible white space or CRLF.
+    for (uint32_t i = length; i > 0; i--) {
+      if (!IsSegmentSpace(mCurrentWord[i - 1]) && mCurrentWord[i - 1] != 0x0a) {
+        if (i != length) {
+          breakState[i] = 1;
+        }
+        break;
+      }
+    }
   }

   bool autoHyphenate = mCurrentWordLanguage && !mCurrentWordContainsMixedLang;
   uint32_t i;
   for (i = 0; autoHyphenate && i < mTextItems.Length(); ++i) {
     TextItem* ti = &mTextItems[i];
     if (!(ti->mFlags & BREAK_USE_AUTO_HYPHENATION)) {
       autoHyphenate = false;

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: testcase #2 (use display:inline-block instead of display:flex)

I change display: flex to display: inline-block and simplify it to make sure this bug is not related to flex layout.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Jonathan, do you mind take look at Makato's comment 7 regarding whitespace handling. I guess the answer is somewhere in nsTextFrame, and you might have a better insight than me in text layout.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: testcase 3 - showing the effect of letter-spacing on the newline

It looks like the issue here is caused by the letter-spacing being applied to the preserved newline. When measuring the inherent width of the block, the newline is not included, but then when reflowing the text to fit that width, the newline is considered to have non-zero width due to the letter-spacing, and doesn't fit at the end of the line.

This testcase demonstrates this: in example 1, the bug occurs, because the newline (expanded by letter-spacing) doesn't fit. In example 2, letter-spacing has been disabled for the newline only, and this fixes the issue. Example 3 shows that letter-spacing only on the newline is sufficient to trigger the bug.

To fix this, I think we just need to fix the CanAddSpacingAfter function so that it never thinks spacing can be added after a preserved newline. Currently, it assumes spacing can always be added at the end of the textrun, but that should not be the case if the last character was a preserved newline.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1850620 - Ensure we never try to add letter-spacing to a preserved newline. r=#layout

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3bec7a68bb8
Ensure we never try to add letter-spacing to a preserved newline. r=dholbert

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43235 for changes under testing/web-platform/tests

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Thank you Makoto for the investigation and Jonathan for fixing the bug promptly!

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b3bec7a68bb8

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot