[Webkit-unassigned] [Bug 85017] enable ctrl-arrow move by word visually in other platforms (besides Windows)

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Xiaomei Ji <xji at chromium.org>  2012-05-01 13:57:20 PST ---
(From update of attachment 139541)
View in context: https://bugs.webkit.org/attachment.cgi?id=139541&action=review

>> Source/WebCore/ChangeLog:8
>> +        Enable ctrl-arrow moves caret by word in visual order in other platforms (besides Windows) that use ICU word
> 
> What do you mean by "besides Windows"? It's probably better to say on non-Windows platforms.

done.

>> Source/WebCore/ChangeLog:9
>> +        break iterator (it is not enabled for WINCE and QT where ICU is not used). For those platforms, ctrl-arrow
> 
> Nit: WinCE and Qt ports.

done.

>> Source/WebCore/ChangeLog:11
>> +        break positions using ctrl-left-arrow from rightmost position are "|abc |def |hij" (same as Windows platform).
> 
> Nit: same as the convention on Windows? What are you referring to on Windows?

Yes, same as the convention on Windows. I was referring to the ctrl-left-arrow in Windows platform that we enabled before. To remove confusion, I deleted the 'same as Windows platform' part.

>> Source/WebCore/editing/EditingBehavior.h:71
>> +    // Based on native behavior, when using ctrl(alt)+arrow to move caret by word, ctrl(alt)+left_arrow moves to the
> 
> Why do we need an underscore between left and arrow?

removed.

>> Source/WebCore/editing/EditingBehavior.h:72
>> +    // beginning of word in all platforms, for example, the word break positions are: "|abc |def |hij |opq".
> 
> It's probably better to say "immediately before the word" instead of "beginning".

done.

>> Source/WebCore/editing/EditingBehavior.h:73
>> +    // But ctrl+right_arrow moves to the beginning of word in Windows in a way that caret should be moved to after the
> 
> Ditto about _ between right and arrow.

done.

>>> Source/WebCore/editing/EditingBehavior.h:77
>>> +    bool shouldSkipSpaceBeforeWord() const { return m_type == EditingWindowsBehavior; }
>> 
>> I'm sorry but I get more confused by the comment. I would just do:
>> // "abc |def |hij |opq" on Windows and "abc| def| hij| opq|" on Mac and Linux when moving forward between word boundaries.
>> shouldSkipSpaceAfterWordOnForwardWordBoundaryMovement()
>> if I were you (note more verbose function name).
> 
> Forward/Backward implies logical. Maybe "shouldSkipSpaceWhenMovingRight"?

done.

>>>> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:103
>>>> +"    abc    def    hij    opq    "[28, 22, 15, 8, 4]
>>> 
>>> Why is the second position 22? Shouldn't it be 25? And then 18, 11, then 4?
>>> It seems odd to use a position between whitespaces like this.
>> 
>> that is what returned in VisiblePosition. (22) and (25) should be equivalent. But I am not sure how our space collapse works.
> 
> This worries me. I'm not saying this is right or wrong, but as a frequent breaker of test expectations, I'd like to understand this weird behavior in the event that this expectation changes with a future patch. Could you help us understand why this happens a little better, then maybe explicitly call it out in the changelog or explain it here and provide a link to this bug in the test?

Let's ignore the node and only talk about the offset. If you create a VisiblePosition passing Position(25), the offset in created VisiblePosition is 22.  If you look at VisiblePosition::init(), at line "m_deepPosition = canonicalPosition(position)", when |position| is Position(25),  m_deepPosition's offset is 22. Position 25 is canonicalized to position 22. I think it is the space collapsing.   "hij&#x20;&#x20;&#x20;&#x20;opq" collapsed to "hij&#x20;opq", position 23, 24, and 25 are all canonicalized to position 22.
I added comments in the test file.

>> LayoutTests/editing/selection/move-by-word-visually-mac.html:-26
>> -            document.getElementById("testMoveByWord").style.display = "none";
> 
> Please explain why you're removing this chunk of code in the change log.

done.

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