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

RESOLVED FIXED in Firefox 68

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: emilio, Assigned: jfkthame)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph
Bug Flags:
webcompat ?

Firefox Tracking Flags

(Webcompat Priority:?, firefox68 fixed)

Details

(Whiteboard: [webcompat][wptsync upstream], )

Attachments

(5 attachments)

Posted file 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.
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...
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
Priority: -- → P3
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
Duplicate of this bug: 1512979
(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.)
Posted image Edge 17 screenshot
FWIW, https://crisal.io/tmp/wb-break-all-inline.html shows Edge matching us, unless I've done something wrong?
So maybe edge is more subtle here...
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?
Flags: webcompat?
Whiteboard: [webcompat]

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.

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

Webcompat Priority: --- → ?

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

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 }
Posted file 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.)

Type: enhancement → defect

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.

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.)

Blocks: 1549728
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62fc44768704
Add WPT reftests for word-break:break-all applied to an inline element. r=emilio
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a2919808555
Respect word-break value set on an inline element. r=emilio

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.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → jfkthame
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16887 for changes under testing/web-platform/tests
Whiteboard: [webcompat] → [webcompat][wptsync upstream]
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.