[Webkit-unassigned] [Bug 20677] WebKit wraps between hyphen-minus and numeric characters

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20677


Xianzhu Wang <wangxianzhu at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106301|                            |review?
               Flag|                            |




--- Comment #43 from Xianzhu Wang <wangxianzhu at chromium.org>  2011-09-04 21:15:44 PST ---
Created an attachment (id=106301)
 --> (https://bugs.webkit.org/attachment.cgi?id=106301&action=review)
new patch

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

>> Source/WebCore/rendering/break_lines.cpp:3
>> + * Copyright (C) 2010, 2011 Google Inc. All rights reserved.
> 
> I’m a little confused about the 2010 here. Is there substantial code here that Google published in 2010?

Yes. I forgot to update the copyright in https://bugs.webkit.org/show_bug.cgi?id=37698. I ever used phnixwxz at gmail.com when I had already been a Google employee.

>> Source/WebCore/rendering/break_lines.cpp:89
>> +    { B(1, 1, 1, 1, 1, 1, 1, 1), B(1, 1, 1, 1, 1, 0, 1, 0), 0, B(0, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1) }, // -
> 
> It’s hard for me to review the change in this line because the change log does not state what these 4 changes are or what they are supposed to accomplish.

It's part of a line-breaking matrix. The above change disables line breaking after '-' and before '.' and '0'..'9'. In fact, the above change doesn't affect the result after '-' and before digits because this is hard-coded in shouldBreakAfter().
Added some comment here and ChangeLog.

>> Source/WebCore/rendering/break_lines.cpp:135
>> +            return isASCIIAlphanumeric(lastCh);
> 
> This doesn’t seem right. Why does this handle ASCII alphabetical characters differently from non-ASCII alphabetical characters? Does this code only run if all characters are ASCII? Maybe the ICU code already does this right for non-ASCII, but I can’t tell that from the patch.
> 
> I’d like to see test cases covering the non-ASCII characters to show we have the same kinds of behavior in those cases.

Yes. ICU can handle the case correctly for both ASCII and non-ASCII characters but with lower performance.
This code will be executed if both nextCh and ch are ASCII characters, and is covering the cases of this bug.
I think non-ASCII characters are out of the scope of this bug.

>> Source/WebCore/rendering/break_lines.cpp:173
>> +    UChar lastLastCh = pos > 1 ? str[pos - 2] : 0;
> 
> This now requires more context to correctly break lines. Do the callers always provide enough context in cases where text within the line is split across multiple elements?

I did some test with ICU by reducing the context only from the lastChar. The ICU line-break iterator works differently with the reduced contexts. Many ICU line-breaking rules require even larger contexts.

However, for now WebKit won't pass contexts across element boundaries to line-break iterators. This causes different line-breaking on in-line element boundaries. For example, it may break after the '-' in '1111111-abcdefg', but not in '1111111-<i>abcdefg</i>'. This patch won't change this behavior. Should this be a bug? I searched and found a related bug: https://bugs.webkit.org/show_bug.cgi?id=56269.

>>>> LayoutTests/fast/text/line-breaks-after-hyphen-before-number-expected.txt:24
>>>> +1111111-2222222
>>> 
>>> This output is wrong, no?
>> 
>> The line breaks based on width don’t show up in the DumpRenderTree output, even though the break does happen on screen. The test checks that the result is correct by comparing the heights of the elements.
>> 
>> That’s consistent with the expected behavior. When you convert to text, line breaks based on the width of the layout aren’t considered.
> 
> I think it's quite misleading to dump the text result like this then. If I saw this result, I'd assume that the test is failing.  We should probably hide these text when ran in DRT/TestRunner.

Done.

>> LayoutTests/fast/text/line-breaks-after-hyphen-before-number-expected.txt:30
>> +PASS
> 
> We should probably print PASS/FAIL for each test case so that we can easily diagnose the results when one of these cases fail.

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.