Implement "word-break: keep-all" in CSS3 Text

Implement "word-break: keep-all" in CSS3 Text

This CL contains an implementation on "word-break: keep-all" which
suppresses soft wrap opportunity between letters (mostly in CJK).

http://dev.w3.org/csswg/css-text/#valdef-word-break-keep-all

BUG=141792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194441

[yunsik, 2015-04-20 04:07:30]

Patchset #2 (id:20001) has been deleted

[yunsik, 2015-04-20 04:59:14]

yunsik.jang@lge.com changed reviewers:
+ eae@chromium.org

[yunsik, 2015-04-20 05:01:12]

I implemented "word-break: keep-all" which is not yet supported. please review
this change.

[yunsik, 2015-04-20 05:15:35]

yunsik.jang@lge.com changed reviewers:
+ esprehn@chromium.org

[yunsik, 2015-04-20 16:32:22]

yunsik.jang@lge.com changed reviewers:
+ kojii@chromium.org

[eae, 2015-04-21 11:12:13]

Implementation looks good but I'd like to see a test where keep-all acts
differently than nowrap.

[yunsik, 2015-04-21 13:26:32]

On 2015/04/21 11:12:13, eae wrote:
> Implementation looks good but I'd like to see a test where keep-all acts
> differently than nowrap.

If my guess is right, you probably missed seeing <br> tags in the test
expectation.
Of course, the actual keep-all test case doesn't have one.
I used nowrap to suppress all line-breaking chances and to give *expected*
line-breaking with <br> tags.

[yunsik, 2015-04-22 01:25:29]

I uploaded the patchset #2:

 - removed `nowrap' in text expectation of case #1 to remove dependency on
`nowrap' functionality (and possible confusion on the test objective)
 - added a new mismatching test case to see clearly the difference between
normal and keep-all.

[kojii, 2015-04-22 13:15:34]

Non-owner LGTM. Thank you for working on this.

Checked CSS WG test suites but unfortunately their current tests for this value
look broken, and your tests look good.
http://test.csswg.org/harness/results/css-text-3_dev/grouped/section/5.3/

[eae, 2015-04-22 13:16:06]

LGTM

[yunsik, 2015-04-22 13:31:29]

The CQ bit was checked by yunsik.jang@lge.com

[commit-bot: I haz the power, 2015-04-22 13:31:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/40001

[yunsik, 2015-04-22 15:58:04]

The CQ bit was unchecked by yunsik.jang@lge.com

[yunsik, 2015-04-22 23:34:20]

The CQ bit was checked by yunsik.jang@lge.com to run a CQ dry run

[yunsik, 2015-04-22 23:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1094863007/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-04-22 23:34:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/60001

[yunsik, 2015-04-22 23:35:29]

The CQ bit was unchecked by yunsik.jang@lge.com

[yunsik, 2015-04-22 23:36:16]

The CQ bit was checked by yunsik.jang@lge.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 23:37:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/60001

[yunsik, 2015-04-23 00:39:17]

The CQ bit was unchecked by yunsik.jang@lge.com

[yunsik, 2015-04-23 00:39:29]

Patchset #3 (id:60001) has been deleted

[yunsik, 2015-04-23 06:33:37]

Hello reviewers,

Today I found CQ  got failed due to the layout test of keep-all failure in
mac_blink_rel:

https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/53...

As I see, this is from a behavior of text width calculation on Mac. When a sort
of text chunk which contains shrinkable letters, in this case Japanese `si'
letter, is given to text-run, Mac-port may return more narrower width than sum
of widths, run by one by one.

I don't think this is an error and I'd like to remove the word which make test
be failed. How do you think about this?

[yunsik, 2015-04-23 06:46:32]

The CQ bit was checked by yunsik.jang@lge.com to run a CQ dry run

[yunsik, 2015-04-23 06:46:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1094863007/#ps80001
(title: "Fix layout test failure on mac")

[commit-bot: I haz the power, 2015-04-23 06:46:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/80001

[commit-bot: I haz the power, 2015-04-23 09:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 09:40:17]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2015-04-23 13:36:32]

jshin@chromium.org changed reviewers:
+ jshin@chromium.org

[jungshik at Google, 2015-04-23 13:36:33]

Thank you for the fix.

https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
File LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html
(right):

https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html:10:
这是一些汉字, and some Latin, و
کمی نوشتن
عربی, 그리고 샘플
한글 문장,
日本語の文字.
nit: could you use real characters in UTF-8 (for a better readability) ? In that
case, please add '<meta charset="UTF-8"> to <head>.

[jungshik at Google, 2015-04-23 13:37:34]

On 2015/04/23 13:36:33, Jungshik at google wrote:
> Thank you for the fix.
> 
>
https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
> File LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html
> (right):
> 
>
https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
> LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html:10:
> 这是一些汉字, and some Latin, و
> کمی نوشتن
> عربی, 그리고 샘플
> 한글 문장,
> 日本語の文字.
> nit: could you use real characters in UTF-8 (for a better readability) ? In
that
> case, please add '<meta charset="UTF-8"> to <head>.

In case it's not clear, I meant replacing NCRs (&#xhh....;) with readable
characters in UTF-8.

[jungshik at Google, 2015-04-23 20:34:15]

https://codereview.chromium.org/1094863007/diff/80001/Source/platform/text/Te...
File Source/platform/text/TextBreakIterator.cpp (right):

https://codereview.chromium.org/1094863007/diff/80001/Source/platform/text/Te...
Source/platform/text/TextBreakIterator.cpp:228: static inline bool
isComplexContextDependent(UChar ch)
Please, use  either requiresContextForWordBoundary(UChar32 ch) in 
TextBoundaries.h or WTF::Unicode::hasLineBreakingPropertyComplexContext()
instead of enumerating blocks here (that will get obsolete as the standard moves
on). 

Moreover, you have to use UChar32. 
See nextBreakablePositionBreakAllInternal() and
isUnicodeCategoryLetterOrNumber() above and do something similar to  handle 
non-BMP characters (represented in a pair of UChar's - surrogate pairs in
UTF-16) in nextBreakablePositionKeepAllInternal(). 

There are about 2,200 relatively widely used CJK ideographs in Plane 2 (about
2,000 from Hong Kong and about 200 from JIS X 213) and you can add them (need
not be them, though ) to layout tests

[yunsik, 2015-04-24 07:01:15]

@Jungshik, Thank you for your comments.

I'll upload a new patch which contains:
 - replace custom isComplexContextDependent to
hasLineBreakingPropertyComplexContext
 - remove quite meaningless layout test case #2
 - add some more cases in test case #1

https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
File LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html
(right):

https://codereview.chromium.org/1094863007/diff/80001/LayoutTests/fast/css3-t...
LayoutTests/fast/css3-text/css3-word-break/css3-word-break-keep-all.html:10:
这是一些汉字, and some Latin, و
کمی نوشتن
عربی, 그리고 샘플
한글 문장,
日本語の文字.
On 2015/04/23 13:36:33, Jungshik at google wrote:
> nit: could you use real characters in UTF-8 (for a better readability) ? In
that
> case, please add '<meta charset="UTF-8"> to <head>. 

I don't think it's good idea. And some other layout tests already write unicode
characters in this way.

https://codereview.chromium.org/1094863007/diff/80001/Source/platform/text/Te...
File Source/platform/text/TextBreakIterator.cpp (right):

https://codereview.chromium.org/1094863007/diff/80001/Source/platform/text/Te...
Source/platform/text/TextBreakIterator.cpp:228: static inline bool
isComplexContextDependent(UChar ch)
On 2015/04/23 20:34:15, Jungshik at google wrote:
> Please, use  either requiresContextForWordBoundary(UChar32 ch) in 
> TextBoundaries.h or WTF::Unicode::hasLineBreakingPropertyComplexContext()
> instead of enumerating blocks here (that will get obsolete as the standard
moves
> on). 

Yes you're right. they seem to be better to use. I didn't see that such a
function is already existing. Thanks for your advice.


> 
> Moreover, you have to use UChar32. 
> See nextBreakablePositionBreakAllInternal() and
> isUnicodeCategoryLetterOrNumber() above and do something similar to  handle 
> non-BMP characters (represented in a pair of UChar's - surrogate pairs in
> UTF-16) in nextBreakablePositionKeepAllInternal(). 

Because ICU does not break between surrogates (not only between surrogate
pairs), there would be no breaking opportunity between non-BMP characters. It is
same even in case of word-break: normal.

So, I don't think i should do something for surrogates. 

> 
> There are about 2,200 relatively widely used CJK ideographs in Plane 2 (about
> 2,000 from Hong Kong and about 200 from JIS X 213) and you can add them (need
> not be them, though ) to layout tests

Build bot mac_blink_rel fails in layout tests with weird letter space problem
which I cannot reproduce with my mac. So I'm afraid to add many case of letters
into layout test ;(

[yunsik, 2015-04-24 09:06:58]

The CQ bit was checked by yunsik.jang@lge.com to run a CQ dry run

[yunsik, 2015-04-24 09:06:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1094863007/#ps100001
(title: "Replace custom SA charset detect function to already existing one &
rewrite layout test files")

[commit-bot: I haz the power, 2015-04-24 09:07:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/100001

[commit-bot: I haz the power, 2015-04-24 10:30:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 10:30:36]

Dry run: This issue passed the CQ dry run.

[yunsik, 2015-04-25 16:40:37]

The CQ bit was checked by yunsik.jang@lge.com

[commit-bot: I haz the power, 2015-04-25 16:41:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094863007/100001

[commit-bot: I haz the power, 2015-04-25 17:57:26]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194441