Bug 195361 – A single leading space is not considered as a word break even when word-break: break-all is set

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Javier Fernandez]

Created attachment 363743 [details]
Test case to reproduce the issue

Steps to reproduce the problem:

1- Load the attached test case

ACTUAL RESULT

The text line is wrapped after the leading white-space (actual-result.png)

EXPECTED RESULT 

The word should be broken, like in the attached figure (expected-result.png)

This expected result is based on the discussion held with the CSS WG regarding this issue:

https://github.com/w3c/csswg-drafts/issues/2907#issuecomment-404944849

[Javier Fernandez]

Created attachment 363744 [details]
Actual result

[Javier Fernandez]

Created attachment 363745 [details]
Expected result

[Javier Fernandez]

BTW, the expected behavior is the one that Chrome, Edge and Firefox have for this test case.

[Javier Fernandez]

Created attachment 363898 [details]
Patch

[EWS Watchlist]

Comment on attachment 363898 [details]
Patch

Attachment 363898 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11416854

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
editing/deleting/delete-block-table.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363908 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363898 [details]
Patch

Attachment 363898 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11417153

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
editing/deleting/delete-block-table.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363909 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363898 [details]
Patch

Attachment 363898 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11417300

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
editing/deleting/delete-block-table.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363914 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363898 [details]
Patch

Attachment 363898 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11417298

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
editing/deleting/delete-block-table.html
fast/inline/trailing-floats-inline-crash.html
fast/inline/crash-when-inline-box-has-invalid-float.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363915 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363898 [details]
Patch

Attachment 363898 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11419608

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
editing/deleting/delete-block-table.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363950 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Javier Fernandez]

Created attachment 364012 [details]
Patch

[EWS Watchlist]

Comment on attachment 364012 [details]
Patch

Attachment 364012 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11426650

New failing tests:
compositing/video/video-clip-change-src.html

[EWS Watchlist]

Created attachment 364016 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 364049 [details]
Patch

[EWS Watchlist]

Comment on attachment 364049 [details]
Patch

Attachment 364049 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11430399

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 364062 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 364087 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 364087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364087&action=review

> Source/WebCore/rendering/line/BreakingContext.h:847
> -        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || style.whiteSpace() != WhiteSpace::PreWrap)) {
> +        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || m_atStart || style.whiteSpace() != WhiteSpace::PreWrap)) {

This seems to also affect `line-break: break-word`. That doesn't seem right.
`line-break: break-word` should be treated like `line-break: normal`:
https://drafts.csswg.org/css-text-3/#valdef-word-break-break-word

We need a test for this case.

> LayoutTests/ChangeLog:11
> +          - word-break/word-break-break-all-010.html: This test pass now thanks to this change.

Doesn't seem like you've removed the relevant line.

> LayoutTests/ChangeLog:12
> +          - text-transform-capitalize-026.html: This test is marked as flacky in all platforms.

Why is this test now flaky?
How could the result of text layout be different each time we load it?

> LayoutTests/ChangeLog:14
> +          - text-transform-capitalize-026.html: Removed this entry since it was marked as flacky for all platforms.

Doesn't seem like you made this change.

[Darin Adler]

Comment on attachment 364087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364087&action=review

>> Source/WebCore/rendering/line/BreakingContext.h:847
>> +        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || m_atStart || style.whiteSpace() != WhiteSpace::PreWrap)) {
> 
> This seems to also affect `line-break: break-word`. That doesn't seem right.
> `line-break: break-word` should be treated like `line-break: normal`:
> https://drafts.csswg.org/css-text-3/#valdef-word-break-break-word
> 
> We need a test for this case.

review- because of this comment

[Javier Fernandez]

(In reply to Ryosuke Niwa from comment #22)
> Comment on attachment 364087 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364087&action=review
> 
> > Source/WebCore/rendering/line/BreakingContext.h:847
> > -        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || style.whiteSpace() != WhiteSpace::PreWrap)) {
> > +        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || m_atStart || style.whiteSpace() != WhiteSpace::PreWrap)) {
> 
> This seems to also affect `line-break: break-word`. That doesn't seem right.
> `line-break: break-word` should be treated like `line-break: normal`:
> https://drafts.csswg.org/css-text-3/#valdef-word-break-break-word

Umm, I guess you meant "word-break: break-word', right ? The 'line-break' property doesn't have such 'break-word' value, according to the last draft of the spec: 

https://drafts.csswg.org/css-text-3/#propdef-line-break

As a matter of fact, 'word-break: break-word' seems to be deprecated, as it's indicated in the note you linked before. It's not the intention of this patch to change that behavior. 

However, a collateral, and desirable, effect of this patch is to change the behavior of 'overflow-wrap: break-word', which is already supported by several of the WPT tests; I've udated the test suite recently to cover the cases discussed in the git hub issue [1] mentioned before.

https://github.com/w3c/csswg-drafts/issues/2907#issuecomment-404944849



> 
> We need a test for this case.
> 

Could you clarify the unexpected behavior change that the patch is causing ? I'm not sure whether providing tests for the 'word-break: break-word' conversion to 'word-break: normal' makes sense in this patch, since it's not part of the goals I had in mind. What do you think about doing so in a follow up patch ? 

This is a, kind of, blocker bug for #177327, which is what I'm what I'm focus on now.



> > LayoutTests/ChangeLog:11
> > +          - word-break/word-break-break-all-010.html: This test pass now thanks to this change.
> 
> Doesn't seem like you've removed the relevant line.
> 

The word-break/word-break-break-all-010.html was precisely defined to cover this issue. It's indeed failing for GTK and WPE, and i could verify it fails in Safari on mobile. I'm not sure why it passes in the mac bots, though. I'll try to figure it out. 


> > LayoutTests/ChangeLog:12
> > +          - text-transform-capitalize-026.html: This test is marked as flacky in all platforms.
> 
> Why is this test now flaky?
> How could the result of text layout be different each time we load it?
> 

Well, the test was flaky before my patch, sorry for the noise. Actually, these tests were imported originally in #r232057 and marked as flay (all of them but the one I'm changing now, 026) in #r232456 as part of some gardening tasks. It's been marked as PASS for GTK and WPE later, s I guess is clear it's as flaky as the rest of the text-transform-capitalize-xxx.html tests.

Anyway, that change is not related to the patch so I'll do it as part of a gardening tasks as soon as possible.

[Ryosuke Niwa]

(In reply to Javier Fernandez from comment #24)
> (In reply to Ryosuke Niwa from comment #22)
> > Comment on attachment 364087 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=364087&action=review
> > 
> > > Source/WebCore/rendering/line/BreakingContext.h:847
> > > -        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || style.whiteSpace() != WhiteSpace::PreWrap)) {
> > > +        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || m_atStart || style.whiteSpace() != WhiteSpace::PreWrap)) {
> > 
> > This seems to also affect `line-break: break-word`. That doesn't seem right.
> > `line-break: break-word` should be treated like `line-break: normal`:
> > https://drafts.csswg.org/css-text-3/#valdef-word-break-break-word
> 
> Umm, I guess you meant "word-break: break-word', right ?

Oops, yes.

> > 
> > We need a test for this case.
> > 
> 
> Could you clarify the unexpected behavior change that the patch is causing ?
> I'm not sure whether providing tests for the 'word-break: break-word'
> conversion to 'word-break: normal' makes sense in this patch, since it's not
> part of the goals I had in mind. What do you think about doing so in a
> follow up patch ?

It does not. However, this patch nonetheless affects the behavior of `word-break: break-word`. That behavioral change needs to be accompanied with a test regardless of whether this property value is deprecated in the spec or not.

> > > LayoutTests/ChangeLog:11
> > > +          - word-break/word-break-break-all-010.html: This test pass now thanks to this change.
> > 
> > Doesn't seem like you've removed the relevant line.
> > 
> 
> The word-break/word-break-break-all-010.html was precisely defined to cover
> this issue. It's indeed failing for GTK and WPE, and i could verify it fails
> in Safari on mobile. I'm not sure why it passes in the mac bots, though.
> I'll try to figure it out.

Okay. Please mention relevant observations like that in the change log.

> > > LayoutTests/ChangeLog:12
> > > +          - text-transform-capitalize-026.html: This test is marked as flacky in all platforms.
> > 
> > Why is this test now flaky?
> > How could the result of text layout be different each time we load it?
> > 
> 
> Well, the test was flaky before my patch, sorry for the noise. Actually,
> these tests were imported originally in #r232057 and marked as flay (all of
> them but the one I'm changing now, 026) in #r232456 as part of some
> gardening tasks. It's been marked as PASS for GTK and WPE later, s I guess
> is clear it's as flaky as the rest of the text-transform-capitalize-xxx.html
> tests.

Okay, that DOES indeed seems like a case. But then we should probably be making this test expectation change in a separate patch / commit.

[Javier Fernandez]

Created attachment 364486 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 364486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364486&action=review

> Source/WebCore/rendering/line/BreakingContext.h:823
> +        if (m_atStart && m_currentCharacterIsSpace && !previousCharacterIsSpace)
> +            breakWords = false;

This isn't right. breakWords is always false hereafter for every character.
We also need a new test for this given this change's bug didn't get caught by any existing test.
r-

[Javier Fernandez]

Comment on attachment 364087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364087&action=review

>>>>> Source/WebCore/rendering/line/BreakingContext.h:847
>>>>> +        if ((breakAll || breakWords) && !midWordBreak && (!m_currentCharacterIsSpace || m_atStart || style.whiteSpace() != WhiteSpace::PreWrap)) {
>>>> 
>>>> This seems to also affect `line-break: break-word`. That doesn't seem right.
>>>> `line-break: break-word` should be treated like `line-break: normal`:
>>>> https://drafts.csswg.org/css-text-3/#valdef-word-break-break-word
>>>> 
>>>> We need a test for this case.
>>> 
>>> review- because of this comment
>> 
>> Umm, I guess you meant "word-break: break-word', right ? The 'line-break' property doesn't have such 'break-word' value, according to the last draft of the spec: 
>> 
>> https://drafts.csswg.org/css-text-3/#propdef-line-break
>> 
>> As a matter of fact, 'word-break: break-word' seems to be deprecated, as it's indicated in the note you linked before. It's not the intention of this patch to change that behavior. 
>> 
>> However, a collateral, and desirable, effect of this patch is to change the behavior of 'overflow-wrap: break-word', which is already supported by several of the WPT tests; I've udated the test suite recently to cover the cases discussed in the git hub issue [1] mentioned before.
>> 
>> https://github.com/w3c/csswg-drafts/issues/2907#issuecomment-404944849
> 
> Oops, yes.

Note that the change in line 822 tries to neutralize this one for the case of breakWords being true, which is the case of overflow-wrap: break-words or word-break: break-words (deprecated)

>>>> LayoutTests/ChangeLog:11
>>>> +          - word-break/word-break-break-all-010.html: This test pass now thanks to this change.
>>> 
>>> Doesn't seem like you've removed the relevant line.
>> 
>> The word-break/word-break-break-all-010.html was precisely defined to cover this issue. It's indeed failing for GTK and WPE, and i could verify it fails in Safari on mobile. I'm not sure why it passes in the mac bots, though. I'll try to figure it out.
> 
> Okay. Please mention relevant observations like that in the change log.

I've updated the WebCore/ChangeLog with additional information about which tests are used to cover the new behavior introduced by this change.

>>>> LayoutTests/ChangeLog:12
>>>> +          - text-transform-capitalize-026.html: This test is marked as flacky in all platforms.
>>> 
>>> Why is this test now flaky?
>>> How could the result of text layout be different each time we load it?
>> 
>> Well, the test was flaky before my patch, sorry for the noise. Actually, these tests were imported originally in #r232057 and marked as flay (all of them but the one I'm changing now, 026) in #r232456 as part of some gardening tasks. It's been marked as PASS for GTK and WPE later, s I guess is clear it's as flaky as the rest of the text-transform-capitalize-xxx.html tests.
>> 
>> Anyway, that change is not related to the patch so I'll do it as part of a gardening tasks as soon as possible.
> 
> Okay, that DOES indeed seems like a case. But then we should probably be making this test expectation change in a separate patch / commit.

I've been analyzing the patch for a while already and I don't see the behavior change you mention. As a matter of fact, changes in line 822 of in this patch are precisely to preserve the current behavior of both, overflow-wrap: break-word and word-break: break-word properties. Even more, there are already several tests, which pass before this change, to ensure this patch doesn't change such behavior.

I hope I'm not missing a case where this patch may have affect the above mentioned properties, but I couldn't find any yet.

[Javier Fernandez]

Comment on attachment 364486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364486&action=review

>> Source/WebCore/rendering/line/BreakingContext.h:823
>> +            breakWords = false;
> 
> This isn't right. breakWords is always false hereafter for every character.
> We also need a new test for this given this change's bug didn't get caught by any existing test.
> r-

Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).

[Ryosuke Niwa]

Comment on attachment 364486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364486&action=review

>>> Source/WebCore/rendering/line/BreakingContext.h:823
>>> +            breakWords = false;
>> 
>> This isn't right. breakWords is always false hereafter for every character.
>> We also need a new test for this given this change's bug didn't get caught by any existing test.
>> r-
> 
> Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).

The trouble here is that we can't assume that we've encountered a word breaking just because m_currentCharacterIsSpace is a space.
It's possible that we can still fire more content into the current line but we would have to break a word later in the line.
Setting breakWords to false here would mean that all words that appear in this line after seeing the leading space would not be
considered as a line breaking opportunity as far as I can tell.

[Javier Fernandez]

(In reply to Ryosuke Niwa from comment #30)
> Comment on attachment 364486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364486&action=review
> 
> >>> Source/WebCore/rendering/line/BreakingContext.h:823
> >>> +            breakWords = false;
> >> 
> >> This isn't right. breakWords is always false hereafter for every character.
> >> We also need a new test for this given this change's bug didn't get caught by any existing test.
> >> r-
> > 
> > Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).
> 
> The trouble here is that we can't assume that we've encountered a word
> breaking just because m_currentCharacterIsSpace is a space.
> It's possible that we can still fire more content into the current line but
> we would have to break a word later in the line.
> Setting breakWords to false here would mean that all words that appear in
> this line after seeing the leading space would not be
> considered as a line breaking opportunity as far as I can tell.

I think any other further white-space will be indeed considered as a breaking opportunity. What this code would do is to prevent the word to be broken, either breaking at the leading white-space or at any further space, which I think makes sense. Do you agree on this ? 

I think we already have tests covering that case, but I'll check it out again and provide them otherwise.

[Ryosuke Niwa]

(In reply to Javier Fernandez from comment #31)
> (In reply to Ryosuke Niwa from comment #30)
> > Comment on attachment 364486 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=364486&action=review
> > 
> > >>> Source/WebCore/rendering/line/BreakingContext.h:823
> > >>> +            breakWords = false;
> > >> 
> > >> This isn't right. breakWords is always false hereafter for every character.
> > >> We also need a new test for this given this change's bug didn't get caught by any existing test.
> > >> r-
> > > 
> > > Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).
> > 
> > The trouble here is that we can't assume that we've encountered a word
> > breaking just because m_currentCharacterIsSpace is a space.
> > It's possible that we can still fire more content into the current line but
> > we would have to break a word later in the line.
> > Setting breakWords to false here would mean that all words that appear in
> > this line after seeing the leading space would not be
> > considered as a line breaking opportunity as far as I can tell.
> 
> I think any other further white-space will be indeed considered as a
> breaking opportunity.

Obviously. But the point of `word-break: break-word` is to allow word-breaking for non-space characters. In effect, your code change would break this feature whenever there is a leading whitespace.

> What this code would do is to prevent the word to be
> broken, either breaking at the leading white-space or at any further space,
> which I think makes sense. Do you agree on this ? 

No, that's not sufficient. We need to break words for line breaking purposes when `word-break: break-word` is set.

[Javier Fernandez]

Created attachment 364522 [details]
pre-wrap-bug-3.html

[Javier Fernandez]

Created attachment 364523 [details]
pre-wrap-bug-4.html

[Javier Fernandez]

Comment on attachment 364522 [details]
pre-wrap-bug-3.html

break-word with single leading space

[Javier Fernandez]

Comment on attachment 364523 [details]
pre-wrap-bug-4.html

break-word without leading space, but with a space opportunity before the word breaking point.

[Javier Fernandez]

Comment on attachment 364486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364486&action=review

>>>>>> Source/WebCore/rendering/line/BreakingContext.h:823
>>>>>> +            breakWords = false;
>>>>> 
>>>>> This isn't right. breakWords is always false hereafter for every character.
>>>>> We also need a new test for this given this change's bug didn't get caught by any existing test.
>>>>> r-
>>>> 
>>>> Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).
>>> 
>>> The trouble here is that we can't assume that we've encountered a word breaking just because m_currentCharacterIsSpace is a space.
>>> It's possible that we can still fire more content into the current line but we would have to break a word later in the line.
>>> Setting breakWords to false here would mean that all words that appear in this line after seeing the leading space would not be
>>> considered as a line breaking opportunity as far as I can tell.
>> 
>> I think any other further white-space will be indeed considered as a breaking opportunity. What this code would do is to prevent the word to be broken, either breaking at the leading white-space or at any further space, which I think makes sense. Do you agree on this ? 
>> 
>> I think we already have tests covering that case, but I'll check it out again and provide them otherwise.
> 
> Obviously. But the point of `word-break: break-word` is to allow word-breaking for non-space characters. In effect, your code change would break this feature whenever there is a leading whitespace.

That's not what WebKit currently implements for 'break-word' actually, that's why I think there is no behavior change in my code. Either for the case of a single leading white-space (see pre-wrap-bug-3.html) or without it (see pre-wrap-bug-4.html) WebKit avois breaking the word if there is a previous breaking opportunity available (similar behavior of overflow-wrap: break-word). Chrome has this behavior too, so that Firefox and Edge (but for different reasons, since neither of these implements word-break: break-word). 

I understand that the behavior you describe for word-break: break-word would make sense, and it'd be indeed a difference with overflow-wrap: break-word, but since the spec changed and now this property is deprecated (it's specified that it should behave as 'normal' while is not removed), I'm not sure we should define a test verifying the behavior you suggest. 

Additionally, if that's the behavior WebKit should implement, it's a bug already present before this patch, which I don't think it'd make sense to fix anyway because it wouldn't be spec compliant and it wont be an interoperable behavior either.

[Javier Fernandez]

Comment on attachment 364486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364486&action=review

>>>>>>> Source/WebCore/rendering/line/BreakingContext.h:823
>>>>>>> +            breakWords = false;
>>>>>> 
>>>>>> This isn't right. breakWords is always false hereafter for every character.
>>>>>> We also need a new test for this given this change's bug didn't get caught by any existing test.
>>>>>> r-
>>>>> 
>>>>> Why is that wrong ? That variable is set before the loop, based on the value of overflow-wrap and word-break and it's never changed while evaluating the text line. It's basically the same logic as in line L1013, where breakWords is set to false once a breaking opportunity has been found; also being false hereafter for every character. It's true that it only happens if autoWrap is enabled, which I'm not considering in my patch (maybe something I should have).
>>>> 
>>>> The trouble here is that we can't assume that we've encountered a word breaking just because m_currentCharacterIsSpace is a space.
>>>> It's possible that we can still fire more content into the current line but we would have to break a word later in the line.
>>>> Setting breakWords to false here would mean that all words that appear in this line after seeing the leading space would not be
>>>> considered as a line breaking opportunity as far as I can tell.
>>> 
>>> I think any other further white-space will be indeed considered as a breaking opportunity. What this code would do is to prevent the word to be broken, either breaking at the leading white-space or at any further space, which I think makes sense. Do you agree on this ? 
>>> 
>>> I think we already have tests covering that case, but I'll check it out again and provide them otherwise.
>> 
>> Obviously. But the point of `word-break: break-word` is to allow word-breaking for non-space characters. In effect, your code change would break this feature whenever there is a leading whitespace.
> 
> That's not what WebKit currently implements for 'break-word' actually, that's why I think there is no behavior change in my code. Either for the case of a single leading white-space (see pre-wrap-bug-3.html) or without it (see pre-wrap-bug-4.html) WebKit avois breaking the word if there is a previous breaking opportunity available (similar behavior of overflow-wrap: break-word). Chrome has this behavior too, so that Firefox and Edge (but for different reasons, since neither of these implements word-break: break-word). 
> 
> I understand that the behavior you describe for word-break: break-word would make sense, and it'd be indeed a difference with overflow-wrap: break-word, but since the spec changed and now this property is deprecated (it's specified that it should behave as 'normal' while is not removed), I'm not sure we should define a test verifying the behavior you suggest. 
> 
> Additionally, if that's the behavior WebKit should implement, it's a bug already present before this patch, which I don't think it'd make sense to fix anyway because it wouldn't be spec compliant and it wont be an interoperable behavior either.

BTW, these issues has been already discussed here https://github.com/w3c/csswg-drafts/issues/2907, just to give some context for the discussion.

[Javier Fernandez]

Created attachment 364951 [details]
Patch

[Javier Fernandez]

Created attachment 364979 [details]
Test case to reproduce the issue

It seems that the bug is not present in the SimpleLineLayout codepath, so adding line-break:strict to avoid it, forcing the use of the BreakingContext class.

[Javier Fernandez]

Hi @rniwa, could you please have another look to the tests cases I defined to verify currently implemented break-word's behavior ? I added also additional WPT to ensure the patch don't regress on such, presumably correct, behavior.

The patch for the code is the same you already review, bit perhaps you can review it again, with the discussion [1] I had with the CSS WG in mind. 

[1]| https://github.com/w3c/csswg-drafts/issues/2907,

[Ryosuke Niwa]

Comment on attachment 364951 [details]
Patch

Sure, given what you described, the proposed code change seems reasonable to me.

[Javier Fernandez]

Created attachment 365862 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365862 [details]
Patch

Clearing flags on attachment: 365862

Committed r243437: <https://trac.webkit.org/changeset/243437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49216242>