1507744 - word-break: break-all on an inline doesn't cause breaks

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

I don't know whether this is a bug in our code, in the spec, or in WebKit / Blink.

We agree with Edge here.

This causes github.com's dashboard to be broken if somebody stars or pushes to a repo with an extremely long name.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Jonathan, do you happen to know who's right here?

The spec says in https://drafts.csswg.org/css-text/#propdef-word-break:

  Applies to: Inline boxes.

So it might be the case that WebKit and Blink are doing the right thing here, but...

Comment 2

[Jonathan Kew [:jfkthame]]

If I'm understanding the spec properly, this should cause breaks (so webkit/blink are correct), and trying to think as an author, that seems intuitively the right thing to do.

So it looks like a Gecko bug to me. If Edge matches our current behavior, though, it may be worth asking what they think.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

So the related code is in BuildTextRunsScanner::SetupBreakSinksForTextRun[1], where we clearly pick word-break value from the line container (i.e. the block) rather than the current frame.

The difficulity here is that we seem to bind the line breaking with text run. I guess the easiest way forward would be breaking text run when word-break changes as part of BuildTextRunsScanner::ContinueTextRunAcrossFrames[2], we can probably then use any frame in the flow for value of word-break. It's not conceptually prefect, but it should be the minimal change given the current settings. Decoupling line breaking from text run might need some non-trivial work...


[1] https://searchfox.org/mozilla-central/rev/ff46b36ac2ebb243ec95fdab01340391963d62e5/layout/generic/nsTextFrame.cpp#2515-2526
[2] https://searchfox.org/mozilla-central/rev/ff46b36ac2ebb243ec95fdab01340391963d62e5/layout/generic/nsTextFrame.cpp#1855-1865

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> If Edge matches our current behavior,
> though, it may be worth asking what they think.

I tested Edge, and they match webkit/blink on this (not us). (EDIT: Edge 18 matches WebKit/Blink, though the previous version [Edge 17] did match us.)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Edge 17 screenshot

FWIW, https://crisal.io/tmp/wb-break-all-inline.html shows Edge matching us, unless I've done something wrong?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So maybe edge is more subtle here...

Comment 8

[Daniel Holbert [:dholbert]]

Yeah, sorry, just noticed that my comment 5 disagreed with your comment 0. What I meant was that Edge on my machine agrees with blink/webkit on the rendering of this bug's attached testcase (https://bugzilla.mozilla.org/attachment.cgi?id=9025608 which is the same as your cristal.io-hosted testcase maybe?) and also in https://jsfiddle.net/c8yet53w/ which I made (using flex) when reducing the github webcompat issue.

But Edge agrees on the rendering of the full complex affected github page itself, in https://github.com/webcompat/web-bugs/issues/22348

So there are other things going on in Edge that make it look like they agree with us.  But it seems like that's due to a different Edge-bug/interaction in the full github website, and Edge/Blink/WebKit/our-intuition all are in agreement that Firefox is wrong on the minimized testcases that we've come up with here.

Also: my Edge version is newer than yours -- I'm testing "Microsoft EdgeHTML 18.17763" on actual Win10. That might be testable as "Edge Preview" or something like that under browserstack?

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://briansmith.org/ecc-inversion-addition-chains-01 is an example of a page that has bad rendering only in Firefox due to this bug; Search for "Binary representation of" to see the long binary numbers that aren't wrapped in Firefox. They are wrapped in current versions of Safari (tested on iOS), Edge, and Chrome.

Comment 10

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 11

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://briansmith.org/ecc-inversion-addition-chains-01

I fixed this in Firefox with this change:

• .breaky { word-break: break-all }

• .breaky { word-break: break-all; overflow-wrap: break-word }

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Additional testcase

I looked at this a bit, and I think we should at least try a patch based on Xidorn's comment 3. This fixes simple testcases where word-break is applied to an inline element, and should solve most of the webcompat issues.

In my testing, it doesn't entirely fix the issue; in particular, if an inline element with word-break:break-all starts mid-word, without a preceding space, we don't handle the changed breaking behavior properly.

In the attached testcase, only the first example (a), where word-break is applied to the containing block, renders correctly in current Firefox.

(FWIW, the last example (e) shows slightly different behavior between Chrome and Safari, with Safari's rendering being more correct IMO. But that's getting far into edge-case territory, while we currently get even the basic case wrong.)

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1507744 - Respect word-break value set on an inline element. r=emilio

Comment 15

[Jonathan Kew [:jfkthame]]

This patch fixes example (b) in the above testcase, which I think reflects the main webcompat issues we're seeing. As noted above, it does not fully resolve all cases, but it's a step in the right direction, and IMO it'd be worth taking this simple patch for now and filing a followup regarding trickier cases like where the value changes mid-word.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1507744 - Add WPT reftests for word-break:break-all applied to an inline element. r=emilio

Tests 001-003 are fixed by the patch in this bug; 004-007 still fail in Firefox after the patch is applied.
(Safari passes all these tests; Chrome fails 004 and 007 in my testing.)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62fc44768704
Add WPT reftests for word-break:break-all applied to an inline element. r=emilio

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a2919808555
Respect word-break value set on an inline element. r=emilio

Comment 19

[Jonathan Kew [:jfkthame]]

Oops, my bad: the patch was supposed to land first, and then the added testcases, as the tests alone will show 3 wpt-reftest failures. In the event this needs to be backed out, we should put them into the correct order for re-landing.

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/62fc44768704
https://hg.mozilla.org/mozilla-central/rev/6a2919808555

Comment 21

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

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

Comment 22

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

Upstream PR merged