1283964 - CSS property word-break not honoured for inline elements

Status: NEW

(Core :: Layout: Block and Inline, defect, P3)

Description

[Konstantin]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606113944

Steps to reproduce:

Set the word-break CSS propery of an element with "display:inline;" (e.g. a <span>) to "word-break:brak-all;". A simple test case is the following:

<div style="background:yellow;width: 400px">
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
<span style="word-break:break-all;">http://some.url/somesuperlongidthatmightappearinurlstoflickrorgoogledriveorsimilarwebservicesandneedstogetwrappedproperly/somemoretokensintheurl/andanotherpart/</span>
Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
</div>

https://jsfiddle.net/r7fo0n5z/


Actual results:

Firefox ignores the "word-break:brak-all;" style of the span and only breaks the url at the word boundaries (the forward slashed "/"), resulting in the URL overflowing the containing box in one line and not using the available space in another.


Expected results:

Firefox should honour "word-break:brak-all;" and thus allow line breaks at any point within the URL.

Firefox only seems to honour the word-break style for block elements (i.e. display:block).

Chromium handles the word-brek property correctly.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

BuildTextRunsScanner::BuildTextRunForFrames explicitly uses mLineContainer->StyleText()->mWordBreak in order to use the block's word-break style.  That code dates back to the original implementation in https://hg.mozilla.org/mozilla-central/rev/78c508bfee65 (bug 249159).

But https://drafts.csswg.org/css-text/#word-break-property says the property applies to "all elements".

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This should be pretty easy to fix by fixing that function (which is in layout/generic/nsTextFrame.cpp).  The main thing will probably be writing tests to make sure that the obvious fix (using implicit-this instead of mLineContainer) actually does the right thing.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, no, it's actually not that simple since that's a method on BuildTextRunsScanner rather than nsTextFrame!

Comment 4

[Thomas Wisniewski]

I wouldn't mind giving this a shot, but could I get some more pointers on what a "correct" fix would be? I've traced things back to the point where the InlineReflowInput is created in nsInlineFrame::Reflow (around line 450), and its caller where the nsLineLayout is created in nsBlockFrame::ReflowInlineFrames (around line 3752), but I'm not sure what to change so that BuildTextRunForFrame will eventually get the correct StyleText.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So it already has the correct styleText, in some sense.  It has a variable called |textStyle| that changes as it iterates over the frames.  Though it's not immediately clear to me that that's always the right frame.  It's also not clear to me if the word breaker will correctly handle multiple SetWordBreak calls at various times in the correct way.  So that would need to be investigated, and fixed if needed.

There are some other fun issues.  For example, I think (although I'm not sure if we still do) we cache text runs and reuse them when we hit the same text with the same style in a different place.  And there are a set of properties that would cause us not to do this.  (I don't think text runs cache line breaking information -- if they did, it would already need to be in this set of properties, I think.)

There are also a set of properties that would cause us to have two text runs rather than one.  See, e.g., BuildTextRunsScanner::ContinueTextRunAcrossFrames.

I don't *think* word-break needs to be in either set of properties, but my memory of this code is a bit rusty, and Jonathan (:jfkthame) would probably have a better idea.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #5)
> There are some other fun issues.  For example, I think (although I'm not
> sure if we still do) we cache text runs and reuse them when we hit the same
> text with the same style in a different place.

We don't cache/re-use the actual text runs; we just cache the individual "shaped words", which we can subsequently use to rapidly create new text runs using the same text/style.

>  And there are a set of
> properties that would cause us not to do this.  (I don't think text runs
> cache line breaking information -- if they did, it would already need to be
> in this set of properties, I think.)
> 
> There are also a set of properties that would cause us to have two text runs
> rather than one.  See, e.g.,
> BuildTextRunsScanner::ContinueTextRunAcrossFrames.
> 
> I don't *think* word-break needs to be in either set of properties, but my
> memory of this code is a bit rusty, and Jonathan (:jfkthame) would probably
> have a better idea.

I think you're right that word-break doesn't need to be considered there.

> It's also not clear to me if the word breaker will correctly handle multiple SetWordBreak calls
> at various times in the correct way.  So that would need to be investigated, and fixed if needed.

Currently, the word breaker stashes the value most recently passed to SetWordBreak, and uses it when it processes a "current word" (a space-delimited run of characters). Which means it's applying a single word-break value to the whole "word"; but if we respect word-break on inline elements, then the value might change -within- a "word", which the breaker doesn't support.

Maybe we could solve this by simply "flushing" the breaker's current word any time the word-break value is changed, though we'd need to think through what the behavior should be at the boundaries.

(Is the spec clear on the expected boundary behaviors? Given

  data:text/html,<div style="width:1px">foo<span style="word-break:break-all">bar</span>quux

there should clearly be breaks each side of the "a" -within- "bar", but what about before the "b" and after the "r"? A quick test here indicates that Chrome and Safari differ on this...)