Bug 85017 – enable ctrl-arrow move by word visually in other platforms (besides Windows)

• Component: HTML Editing
• Status: RESOLVED FIXED

[Xiaomei Ji]

enable ctrl-arrow move by word visually in other platforms (besides Windows)

[Xiaomei Ji]

Created attachment 139101 [details]
patch w/ layout test

[WebKit Review Bot]

Comment on attachment 139101 [details]
patch w/ layout test

Attachment 139101 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12559263

New failing tests:
fast/images/gif-large-checkerboard.html
editing/selection/move-by-word-visually-mac.html

[WebKit Review Bot]

Created attachment 139223 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexey Proskuryakov]

What "other platforms" does this change? If you're changing Mac, then why?

[Xiaomei Ji]

The current WIP patch enables moving by word visually in all other platforms that use ICU break iterator for word break, including Mac,

I meant to ask around whether this behavior is desired for each platform.
If not, I can only enable for chromium.

mitz, ap, enrica,
do you think alt-arrow moving word visually is desired in Mac, or the current behavior is desired and should be kept untouched?

The change is desirable for Mac. Thanks!

[Xiaomei Ji]

Created attachment 139304 [details]
patch w/ layout test

set font-family and font-size for multi-line test so that the render result is the same for Mac and cr-linux.

[Ryosuke Niwa]

Comment on attachment 139304 [details]
patch w/ layout test

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

> Source/WebCore/ChangeLog:7
> +

You need to explain what kind of changes we're making in this patch. r- due to lack of explanation in the change log.

> Source/WebCore/editing/EditingBehavior.h:71
> +    // Based on native behavior, when using ctrl(alt)+arrow to move word, ctrl(alt)+left_arrow moves to begin of word

"to move word" sounds awkward. "to move between words" maybe?
Nit: to the beginning of word.

> Source/WebCore/editing/EditingBehavior.h:74
> +    // But ctrl+right_arrow moves to the begin of word in Windows in a way that it eats space to the next word.
> +    // For example, the word break positions are: "abc |def |hij |opq".

Instead of saying "it eats space", can we just say that the caret should be after the whitespace?

> Source/WebCore/editing/EditingBehavior.h:76
> +    // while ctrl(alt)+right_arrow moves to the end of word in Mac and Linux in a way that it does not eat space to the
> +    // next word. For example, the word break positions are "abc| def| hij| opq|".

Ditto. We should say before whitespace.

> Source/WebCore/editing/FrameSelection.cpp:644
> +#if !OS(WINCE) && !PLATFORM(QT)

Why are we disabling this feature on Qt? Is that because ICU is not available on Qt? This should be documented in the change log.

> Source/WebCore/editing/FrameSelection.cpp:648
> +        bool eatSpaceToNextWord = false;
> +        if (m_frame && m_frame->editor()->behavior().shouldEatSpaceToNextWord())
> +            eatSpaceToNextWord = true;

You can just do:
bool eatSpaceToNextWord = m_frame && m_frame->editor()->behavior().shouldEatSpaceToNextWord().

> Source/WebCore/editing/visible_units.cpp:371
> +    bool eatSpaceToNextWord)

I would call this variable skipsSpaceBeforeWord.

> Source/WebCore/editing/visible_units.cpp:425
> +            || (!eatSpaceToNextWord && direction == MoveLeft && box->direction() == LTR)
> +            || (!eatSpaceToNextWord && direction == MoveRight && box->direction() == RTL)) {

Can we define booleans for "(direction == MoveLeft && box->direction() == LTR) || (direction == MoveRight && box->direction() == RTL)" as well?
Maybe movingBackward?

> Source/WebCore/editing/visible_units.h:46
> +VisiblePosition rightWordPosition(const VisiblePosition&, bool);
> +VisiblePosition leftWordPosition(const VisiblePosition&, bool);

You need variable name here because it's not obvious what these two boolean arguments should be.

> LayoutTests/ChangeLog:7
> +

Please explain why expectations have changed.

[Xiaomei Ji]

Comment on attachment 139304 [details]
patch w/ layout test

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> You need to explain what kind of changes we're making in this patch. r- due to lack of explanation in the change log.

done.

>> Source/WebCore/editing/EditingBehavior.h:71
>> +    // Based on native behavior, when using ctrl(alt)+arrow to move word, ctrl(alt)+left_arrow moves to begin of word
> 
> "to move word" sounds awkward. "to move between words" maybe?
> Nit: to the beginning of word.

changed to "to move caret by word". 
changed to "to the beginning of word".

>> Source/WebCore/editing/EditingBehavior.h:74
>> +    // For example, the word break positions are: "abc |def |hij |opq".
> 
> Instead of saying "it eats space", can we just say that the caret should be after the whitespace?

done.

>> Source/WebCore/editing/EditingBehavior.h:76
>> +    // next word. For example, the word break positions are "abc| def| hij| opq|".
> 
> Ditto. We should say before whitespace.

done.

>> Source/WebCore/editing/FrameSelection.cpp:644
>> +#if !OS(WINCE) && !PLATFORM(QT)
> 
> Why are we disabling this feature on Qt? Is that because ICU is not available on Qt? This should be documented in the change log.

done.

>> Source/WebCore/editing/FrameSelection.cpp:648
>> +            eatSpaceToNextWord = true;
> 
> You can just do:
> bool eatSpaceToNextWord = m_frame && m_frame->editor()->behavior().shouldEatSpaceToNextWord().

done.

>> Source/WebCore/editing/visible_units.cpp:371
>> +    bool eatSpaceToNextWord)
> 
> I would call this variable skipsSpaceBeforeWord.

done.

>> Source/WebCore/editing/visible_units.cpp:425
>> +            || (!eatSpaceToNextWord && direction == MoveRight && box->direction() == RTL)) {
> 
> Can we define booleans for "(direction == MoveLeft && box->direction() == LTR) || (direction == MoveRight && box->direction() == RTL)" as well?
> Maybe movingBackward?

done.

>> Source/WebCore/editing/visible_units.h:46
>> +VisiblePosition leftWordPosition(const VisiblePosition&, bool);
> 
> You need variable name here because it's not obvious what these two boolean arguments should be.

done.

>> LayoutTests/ChangeLog:7
>> +
> 
> Please explain why expectations have changed.

done.

[Xiaomei Ji]

Created attachment 139541 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 139541 [details]
patch w/ layout test

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.

> 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.

> 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?

> 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?

> 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".

> 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.

> Source/WebCore/editing/EditingBehavior.h:77
> +    // white space. For example, the word break positions are: "abc |def |hij |opq".
> +    // while ctrl(alt)+right_arrow moves to the end of word in Mac and Linux in a way that caret should be moved to
> +    // before the white space. For example, the word break positions are "abc| def| hij| opq|".
> +    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).

> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:90
> +" abc def AAA AAA hij AAA AAA uvw xyz "[1, 5, 8, 12, 16, 20, 24, 28, 32, 36], <DIV>[0], "AAA kj AAA mn opq AAA AAA"[3, 6, 10, 13, 17, 21, 25]    FAIL expected: [" abc def AAA AAA hij AAA AAA uvw xyz "[ 1,  5,  8,  12,  16,  20,  24,  28,  32,  36, ]"AAA kj AAA mn opq AAA AAA"[ 3,  6,  10,  13,  17,  21,  25]

Why is this case failing? Offsets do match here.

> 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.

> LayoutTests/editing/selection/move-by-word-visually-mac.html:-26
> -        var tests = document.getElementsByClassName("test_move_by_word");
> -        var sel = getSelection();
> -        for (var i = 0; i < tests.length; ++i) {
> -            if (tests[i].getAttribute("dir") == 'ltr')
> -                log("Test " + (i + 1) + ", LTR:\n");
> -            else
> -                log("Test " + (i + 1) + ", RTL:\n");
> -            sel.setPosition(tests[i], 0);
> -            moveByWord(sel, tests[i], "right");
> -            moveByWord(sel, tests[i], "left");
> -        }
> -        if (document.getElementById("testMoveByWord"))
> -            document.getElementById("testMoveByWord").style.display = "none";

Please explain why you're removing this chunk of code in the change log.

[Xiaomei Ji]

Comment on attachment 139541 [details]
patch w/ layout test

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

>> 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"?

>> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:90
>> +" abc def AAA AAA hij AAA AAA uvw xyz "[1, 5, 8, 12, 16, 20, 24, 28, 32, 36], <DIV>[0], "AAA kj AAA mn opq AAA AAA"[3, 6, 10, 13, 17, 21, 25]    FAIL expected: [" abc def AAA AAA hij AAA AAA uvw xyz "[ 1,  5,  8,  12,  16,  20,  24,  28,  32,  36, ]"AAA kj AAA mn opq AAA AAA"[ 3,  6,  10,  13,  17,  21,  25]
> 
> Why is this case failing? Offsets do match here.

when moving left, it stops at the first empty line (<div><br><div>) whose position is <div>[0].

>> 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.

[Levi Weintraub]

Comment on attachment 139541 [details]
patch w/ layout test

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

>>> 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?

[Xiaomei Ji]

Comment on attachment 139541 [details]
patch w/ layout test

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    opq" collapsed to "hij 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.

[Xiaomei Ji]

Created attachment 139671 [details]
patch w/ layout test

Thanks for the review! Comments addressed.

[Ryosuke Niwa]

Comment on attachment 139671 [details]
patch w/ layout test

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

> Source/WebCore/editing/FrameSelection.cpp:645
> +#if !OS(WINCE) && !PLATFORM(QT)
> +        // Visual word movement relies on isWordTextBreak which is not implemented in WinCE and QT.

Should we define a new macro like USE(WORD_TEXT_BREAK) ?

[Xiaomei Ji]

Comment on attachment 139671 [details]
patch w/ layout test

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

>> Source/WebCore/editing/FrameSelection.cpp:645
>> +        // Visual word movement relies on isWordTextBreak which is not implemented in WinCE and QT.
> 
> Should we define a new macro like USE(WORD_TEXT_BREAK) ?

which is "!OS(WINCE) && !PLATFORM(QT)"?  I feel it might not be direct/clear.

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 139671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139671&action=review
> 
> >> Source/WebCore/editing/FrameSelection.cpp:645
> >> +        // Visual word movement relies on isWordTextBreak which is not implemented in WinCE and QT.
> > 
> > Should we define a new macro like USE(WORD_TEXT_BREAK) ?
> 
> which is "!OS(WINCE) && !PLATFORM(QT)"?  I feel it might not be direct/clear.

Apparently we already have this. Just use USE(ICU_UNICODE) instead.

[Xiaomei Ji]

Created attachment 139689 [details]
patch w/ layout test

address comment #16 (adding macro DEFINED_IS_WORD_TEXT_BREAK).

[Ryosuke Niwa]

Comment on attachment 139689 [details]
patch w/ layout test

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

> Source/WebCore/editing/FrameSelection.cpp:74
> +// Visual word movement relies on isWordTextBreak which is not implemented in WinCE and QT.
> +// https://bugs.webkit.org/show_bug.cgi?id=81136.
> +#define DEFINED_IS_WORD_TEXT_BREAK (!OS(WINCE) && !PLATFORM(QT))

Ugh... please don't define a macro here. Just use USE(ICU_UNICODE) instead.

[Xiaomei Ji]

Created attachment 139707 [details]
patch w/ layout test

using USE(ICU_UNICODE).

[WebKit Review Bot]

Comment on attachment 139707 [details]
patch w/ layout test

Clearing flags on attachment: 139707

Committed r115788: <http://trac.webkit.org/changeset/115788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 65773 has been marked as a duplicate of this bug. ***