Bug 123782 – [CSS3] Add support for the word-break:keep-all CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[KyungTae Kim]

The word-break property can has 'normal','break-all' or 'keep-all', but 'keep-all' is not supported now. 
http://www.w3.org/TR/2013/WD-css-text-3-20131010/#word-break-property

[KyungTae Kim]

Created attachment 215993 [details]
test case

[KyungTae Kim]

Created attachment 216000 [details]
Patch

[KyungTae Kim]

Created attachment 216006 [details]
Patch

[Timothy Hatcher]

This is being asked for on Twitter. https://twitter.com/Captainmaumau/status/583148521672433665

Can someone review this patch? Does it still apply to TOT?

[Brent Fulgham]

This looks like a good patch. I'm really sad it was sitting here for so long! Let's see if we can find a good review for this.

[Radar WebKit Bug Importer]

<rdar://problem/21264859>

[Myles C. Maxfield]

Comment on attachment 216006 [details]
Patch

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

In general, the approach is good. There are just some very small tweaks I'd make.

> Source/WebCore/rendering/SimpleLineLayout.cpp:276
> +                if (wordEndOffset > lineStartOffset && isBreakable(lineBreakIterator, wordEndOffset, nextBreakable, false, false))

We need to make sure that this new CSS value causes SLL not to be taken

> Source/WebCore/rendering/break_lines.cpp:164
> +        if (keepAllWords)

I'm worried about the performance of this. It seems to me that this if statement should be hoisted into its own loop with an early return.

[Myles C. Maxfield]

(In reply to comment #7)
> Comment on attachment 216006 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=216006&action=review
> 
> In general, the approach is good. There are just some very small tweaks I'd
> make.
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:276
> > +                if (wordEndOffset > lineStartOffset && isBreakable(lineBreakIterator, wordEndOffset, nextBreakable, false, false))
> 
> We need to make sure that this new CSS value causes SLL not to be taken
> 
> > Source/WebCore/rendering/break_lines.cpp:164
> > +        if (keepAllWords)
> 
> I'm worried about the performance of this. It seems to me that this if
> statement should be hoisted into its own loop with an early return.

Do you think you could have an updated patch by Monday? If not, I'll be happy to make these small changes and commit this on your behalf.

Thanks!
Myles

[Myles C. Maxfield]

Comment on attachment 216006 [details]
Patch

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

>> Source/WebCore/rendering/break_lines.cpp:164
>> +        if (keepAllWords)
> 
> I'm worried about the performance of this. It seems to me that this if statement should be hoisted into its own loop with an early return.

Correction: I meant to say that it should be hoisted ::and have:: its own loop and an early return

[Myles C. Maxfield]

Created attachment 254544 [details]
Patch

[Darin Adler]

Comment on attachment 254544 [details]
Patch

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

> Source/WebCore/rendering/break_lines.h:165
> +static inline int nextBreakablePositionKeepingAllWords(const CharacterType* str, unsigned length, unsigned pos)

The static here is incorrect.

Return type should be unsigned, not int.

We use words, not abbreviations, for variable names. Should use names like "character" and "startPosition" rather than "str" and "pos".

> Source/WebCore/rendering/break_lines.h:167
> +    int len = static_cast<int>(length);

Very silly to do this conversion to do something that is entirely suitable to do with unsigned.

> Source/WebCore/rendering/break_lines.h:170
> +        CharacterType ch = str[i];
> +        if (isBreakableSpace<nbspBehavior>(ch))

Local variable just makes code harder to read.

> Source/WebCore/rendering/break_lines.h:176
> +inline int nextBreakablePositionKeepingAllWords(LazyLineBreakIterator& lazyBreakIterator, int pos)

We use words, not abbreviations, for variable names. Should use "startPosition" rather than "pos". Also, I suggest naming this argument "iterator", not "lazyBreakIterator". I know this matches existing functions below but they should eventually be fixed. Return value should be unsigned.

> Source/WebCore/rendering/break_lines.h:184
> +inline int nextBreakablePositionKeepingAllWordsIgnoringNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos)

Ditto.

> Source/WebCore/rendering/style/RenderStyleConstants.h:288
> +// Word Break Values. Matches both WinIE and CSS3

Comment seems to have almost no value. I suggest deleting it entirely.

> LayoutTests/ChangeLog:10
> +        * fast/text/word-break-keep-all.html: Added.
> +        * platform/mac/fast/text/word-break-keep-all-expected.png: Added.
> +        * platform/mac/fast/text/word-break-keep-all-expected.txt: Added.

I think a reference test would be better. Can we please add word-break-keep-all-expected.html with explicit line breaks and avoid the platform/mac thing?

[Darin Adler]

Comment on attachment 254544 [details]
Patch

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

> LayoutTests/fast/text/word-break-keep-all.html:11
> +<p>On the next line, line should breaks between any two Korean syllables (word-break: normal)</p>

Grammatical error: "should break", not "should breaks".

> LayoutTests/fast/text/word-break-keep-all.html:14
> +<p>On the next line, line should breaks only at spaces like English (word-break: keep-all)<p> 

Grammatical error: "should break", not "should breaks".

[Myles C. Maxfield]

Committed r185729: <http://trac.webkit.org/changeset/185729>