[Webkit-unassigned] [Bug 57543] -webkit-visual-word does not work on words separated by multiple spaces

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Xiaomei Ji <xji at chromium.org>  2011-05-09 15:50:19 PST ---
(From update of attachment 92380)
View in context: https://bugs.webkit.org/attachment.cgi?id=92380&action=review

>>>> Source/WebCore/editing/visible_units.cpp:1160
>>>> +static VisiblePosition visuallyFirstWordBreakInBoxInsideBlockWithSameDirectionality(const InlineBox* box, const VisiblePosition& wordBreak, int& offsetOfWordBreak) 
>>> 
>>> I'm not sure if "visuallyFirstWordBreak" is descriptive here.  How about something like beforeVisuallyFirstWordInBoxInsideBlockWithSameDirectionality?
>>> 
>>> Also, it may not be worth adding this function at all since it's called at exactly one place.  Can we just put this all in previousWordBreakInBoxInsideBlockWithSameDirectionality?
>> 
>> It actually is the visually first word break in the box (if it is a word break inside the same box).
> 
> But what kind of word break is it?  Is that before a word or after a word?  It's not clear from the function name.

in this case (box and block have the same directionality), it is the position before next word or it could be the position before current word in the case that the boundary of the box is not a word break boundary.
For example: <div>opq <span>abc<b>def </b> hij</span>, when box is "abc", the first visual word break is position before "abc" (position before current word). when box is "def", the first visual word break is position before "hij" (if this position is in the same inlineBox as "def"), which is a position before next word.

We did not expose whether the word break is before a word or after a word in the function names of ....WithSameDirectionality() and ....WithDifferentDirectionality() and their callee.
Inside previousWordBreakInBoxInsideBlockWithSameDirectionality() and nextWordBreakInBoxInsideBlockWithDifferentDirectionality(), there is comment saying where the word break should be, those functions and their callees follow the rule.

If you feel we should put those functions inside their caller, I can do that.

>> Source/WebCore/editing/visible_units.cpp:1162
>> +    VisiblePosition nextWord = nextWordPosition(wordBreak);
> 
> Nit: s/nextWord/positionAfterCurrentWord/

It could be positionAfterCurrentWord or positionAfterNextWord. Maybe change 'wordBreak' to 'visiblePosition" and change 'nextWord' to 'positionAfterWordBreak'.

>> Source/WebCore/editing/visible_units.cpp:1163
>> +    VisiblePosition wordPosition = previousWordPosition(nextWord);
> 
> Nit: s/wordPosition/positionBeforeCurrentWord/

maybe s/wordPosition/positionBeforeWordBreak/?

>>>> Source/WebCore/editing/visible_units.cpp:1186
>>>> +        || (!box->isLeftToRightDirection() && box->prevLeafChild()))) {
>>> 
>>> I'm not following here.  Shouldn't we be checking box->isLeftToRightDirection() && !box->prevLeafChild() instead?
>> 
>> The word break collecting here is actually the rightmost word break for a LTR box or leftmost word break for a RTL box.  Given LTR box as an example, we do not need to save its rightmost word break if it is the visually rightmost box (has no nextLeafChild).
> 
> But your condition says box->isLTR() AND box->nextLeafChild().  Under this condition, the box is not the rightmost box.

We need to collect the rightmost word break if the box is *not* the rightmost box. And we do *not* need to collect the rightmost word break if the box is the rightmost box.

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