1127107 - "white-space: nowrap" and "pre" are nerfed (i.e. lines get wrapped) when combined with "-moz-hyphens: auto; word-break: break-all;"

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
1. Load attached testcase.

EXPECTED RESULTS: No wrapping (I think).

ACTUAL RESULTS: Wrapping.

The testcase has a fixed-width <div> with:
      white-space: nowrap;
      -moz-hyphens: auto;
      word-break: break-all;

I'd expect "white-space: nowrap" to prevent wrapping here. (see discussion below). However, when combined with *both* of the other two declarations, we end up wrapping at the right edge of the div.

(You need *both* declarations, -moz-hyphens *and* word-break, to trigger this issue. If I only include one or the other of those, then I get EXPECTED RESULTS.)

DISCUSSION: The CSS Text 3 spec says:
>  [...] like 'pre', it does not allow wrapping.
http://dev.w3.org/csswg/css-text-3/#valdef-white-space-nowrap

...and the chunk of the 'pre' definition that it's referring to is (I think):
> Lines only break at forced line breaks; content that
> does not fit within the block container overflows it.
http://dev.w3.org/csswg/css-text-3/#valdef-white-space-pre

In that definition, "forced line breaks" is linkified to this chunk:
 http://dev.w3.org/csswg/css-text-3/#forced-line-break
...which explains that "forced lines breaks" are distinct from "soft line breaks".

And the "word-break" & "hyphens" properties are each defined as creating "soft wrap opportunities".
http://dev.w3.org/csswg/css-text-3/#word-break-property
http://dev.w3.org/csswg/css-text-3/#hyphens-property
So I don't think these properties should introduce any "forced breaks", which (per white-space:pre text quoted above definition) means there shouldn't be any breaks in this testcase, AFAICT.

Neither IE nor Chrome have linebreaks in this testcase.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast.ogv

This bug seems to be responsible for text wrapping (instead of overflowing and ellipsisizing) at http://music.baidu.com/rings -- screencast attached, showing me disabling both "-moz-hyphens: auto" decls that affect the element.  This fixes their layout and lets their desired ellipsizing happen.

(Note: this site only loads if you have a mobile UA -- I'm using the User Agent Overrider extension's "Firefox 29 for Android" UA. With a desktop UA, I just get a 404.)

This is our #2 fixlist site for mainland china, according to bug 1107378 comment 4.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> This is our #2 fixlist site for mainland china, according to bug 1107378
> comment 4.

(Though I suspect the issues that got it on the fixlist may have been unrelated, worse problems that are now fixed [I think])

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with "white-space: pre")

Here's a testcase with "white-space: pre" to show that that's affected, too. There are no forced linebreaks in the <div> here, and yet we wrap eagerly.

Comment 4

[Daniel Holbert [:dholbert]]

For thoroughness' sake: I'm using Nightly 38.0a1 (2015-01-28), and it goes back at least as far as Nightly 20.0a1 (2013-01-01) -- so it's not a recent regression.

I'm betting it was introduced when bug 253317 added "-moz-hyphens" support. (I did confirm that a build before that -- Minefield 4.09b13pre (2011-03-18) gives EXPECTED RESULTS.)

(I tried a few other intermediate builds just for fun, but a lot of old builds hit startup crashes these days, for some reason; probably a library mismatch or something.)

Comment 5

[Jonathan Kew [:jfkthame]]

I think I see the problem: the equality test at [1] is broken due to the addition of the BREAK_USE_AUTO_HYPHENATION flag, which should be ignored there.

I'll post a patch in a while, after testing whether it breaks other stuff....

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I think I see the problem: the equality test at [1] is broken due to the
> addition of the BREAK_USE_AUTO_HYPHENATION flag, which should be ignored
> there.

Oops.... [1] there was meant to refer to

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsLineBreaker.cpp#197

Comment 7

[Jonathan Kew [:jfkthame]]

Inspecting the code reveals that it's not only "-moz-hyphens:auto" that will trigger this incorrect wrapping. You can get a similar effect by applying "text-transform:capitalize":

  data:text/html,<div style="width:100px;white-space:nowrap;word-break:break-all;
                  text-transform:capitalize">this text is not supposed to wrap

Fixing this as well, without breaking the text-transform behavior, is going to add a few more lines to the necessary patch. Re-testing...

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: The presence of hyphens:auto and/or text-transform:capitalize should not affect no-wrap behavior

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for spurious interaction between hyphenation/text-transform and line-wrap behavior

Comment 10

[Jonathan Kew [:jfkthame]]

I believe reftests are all happy with the patch above, but I've started a try run to double-check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8e0a48d189

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8556586 [details] [diff] [review]
The presence of hyphens:auto and/or text-transform:capitalize should not affect no-wrap behavior

Thanks for the quick action on this!


>diff --git a/dom/base/nsLineBreaker.cpp b/dom/base/nsLineBreaker.cpp
[...]
> 
>+#define NO_BREAKS_NEEDED_FLAGS (BREAK_SUPPRESS_INITIAL | \
>+                                BREAK_SUPPRESS_INSIDE | \
>+                                BREAK_SKIP_SETTING_NO_BREAKS)

Could you add a brief comment explaining the significance of this mask? e.g....
  // If aFlags has *all* these bits set, then we don't need to worry about
  // finding break opportunities in the appended text.
...or something along those lines.

>+  bool capitalizationNeeded = false;
>   nsTArray<bool> capitalizationState;
>   if (aSink && (aFlags & BREAK_NEED_CAPITALIZATION)) {
>     if (!capitalizationState.AppendElements(aLength))
>       return NS_ERROR_OUT_OF_MEMORY;
>     memset(capitalizationState.Elements(), false, aLength*sizeof(bool));
>+    capitalizationNeeded = true;
>   }

(Maybe this should be "noCapitalizationNeeded", with flipped polarity, so it matches "noBreaksNeeded" in all their combined or adjacent checks? Maybe that's more confusing, though. Whatever you prefer on this WFM.)

>+  if (noBreaksNeeded && !capitalizationNeeded) {
>     // Skip to the space before the last word, since either the break data
>     // here is not needed, or no breaks are set in the sink and there cannot
>     // be any breaks in this chunk; all we need is the context for the next
>     // chunk (if any)

It looks like this comment needs an update, to match its updated condition. (We're skipping because we don't need breaks *and we don't need capitalization* for this stretch.)

r=me

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2762676005
https://hg.mozilla.org/integration/mozilla-inbound/rev/889db6ac72f0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d2762676005
https://hg.mozilla.org/mozilla-central/rev/889db6ac72f0