Bug 85017

Summary: enable ctrl-arrow move by word visually in other platforms (besides Windows)
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, enrica, leviw, mitz, rniwa, vanlam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 25298    
Attachments:
Description Flags
patch w/ layout test
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
patch w/ layout test
rniwa: review-
patch w/ layout test
none
patch w/ layout test
rniwa: review+
patch w/ layout test
none
patch w/ layout test none

Description Xiaomei Ji 2012-04-26 16:54:18 PDT
enable ctrl-arrow move by word visually in other platforms (besides Windows)
Comment 1 Xiaomei Ji 2012-04-26 17:15:16 PDT
Created attachment 139101 [details]
patch w/ layout test
Comment 2 WebKit Review Bot 2012-04-27 10:12:30 PDT
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
Comment 3 WebKit Review Bot 2012-04-27 10:12:36 PDT
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
Comment 4 Alexey Proskuryakov 2012-04-27 10:44:35 PDT
What "other platforms" does this change? If you're changing Mac, then why?
Comment 5 Xiaomei Ji 2012-04-27 11:29:02 PDT
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?
Comment 6 mitz 2012-04-27 11:31:03 PDT
The change is desirable for Mac. Thanks!
Comment 7 Xiaomei Ji 2012-04-27 16:35:35 PDT
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.
Comment 8 Ryosuke Niwa 2012-04-29 15:38:21 PDT
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.
Comment 9 Xiaomei Ji 2012-04-30 15:13:10 PDT
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.
Comment 10 Xiaomei Ji 2012-04-30 16:09:15 PDT
Created attachment 139541 [details]
patch w/ layout test
Comment 11 Ryosuke Niwa 2012-04-30 17:12:44 PDT
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.
Comment 12 Xiaomei Ji 2012-04-30 17:24:44 PDT
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.
Comment 13 Levi Weintraub 2012-04-30 17:39:07 PDT
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?
Comment 14 Xiaomei Ji 2012-05-01 13:57:20 PDT
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&#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.
Comment 15 Xiaomei Ji 2012-05-01 14:01:50 PDT
Created attachment 139671 [details]
patch w/ layout test

Thanks for the review! Comments addressed.
Comment 16 Ryosuke Niwa 2012-05-01 14:25:49 PDT
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) ?
Comment 17 Xiaomei Ji 2012-05-01 14:30:21 PDT
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.
Comment 18 Ryosuke Niwa 2012-05-01 14:37:42 PDT
(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.
Comment 19 Xiaomei Ji 2012-05-01 14:54:39 PDT
Created attachment 139689 [details]
patch w/ layout test

address comment #16 (adding macro DEFINED_IS_WORD_TEXT_BREAK).
Comment 20 Ryosuke Niwa 2012-05-01 15:00:25 PDT
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.
Comment 21 Xiaomei Ji 2012-05-01 16:28:14 PDT
Created attachment 139707 [details]
patch w/ layout test

using USE(ICU_UNICODE).
Comment 22 WebKit Review Bot 2012-05-01 20:40:46 PDT
Comment on attachment 139707 [details]
patch w/ layout test

Clearing flags on attachment: 139707

Committed r115788: <http://trac.webkit.org/changeset/115788>
Comment 23 WebKit Review Bot 2012-05-01 20:41:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryosuke Niwa 2012-05-01 23:40:52 PDT
*** Bug 65773 has been marked as a duplicate of this bug. ***