Bug 59652 – turn on move caret by word visually for windows platform.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

the current implementation comply with window's native support.
For example:
"abc def hij", when press ctrl+right-arrow, the caret postions are "|abc...." ,  "abc |def....",  "abc def |hij".

Mac's native text support implements ctrl-arrow as logical operation.
And for the above example, when press ctrl+right-arrow, the caret positions are "|abc....", "abc| def...", "abc def| hij".

[Xiaomei Ji]

Created attachment 91373 [details]
patch w/ layout test

[Alexey Proskuryakov]

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

The preferred way to implement platform differences like this is by adding a client call, so that all behaviors could be tested in layout tests on all platforms. See e.g. how smart copy and paste is configured via EditorClient.

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 91373 [details])
> The preferred way to implement platform differences like this is by adding a client call, so that all behaviors could be tested in layout tests on all platforms. See e.g. how smart copy and paste is configured via EditorClient.

I agree. We can also use editing behavior.

[Alexey Proskuryakov]

Yes, that's probably even better.

[Xiaomei Ji]

If ctrl-arrow moving caret by word in visual order is not desired for Mac, 
we might need to turn it on for chromium-mac later.

Then, EditingBehavior wont fit the need.
And is it preferred to add a client call or introduce a new setting, say MoveCaretByWordBehavior?

[Alexey Proskuryakov]

That sounds like a question to worry about if you actually ever decide to make Chromium/Mac disrespect this platform convention, and not now. Higher configurability levels have a maintenance cost, so it's better to not implement such unless there is interest.

[Xiaomei Ji]

(In reply to comment #6)
> That sounds like a question to worry about if you actually ever decide to make Chromium/Mac disrespect this platform convention, and not now. Higher configurability levels have a maintenance cost, so it's better to not implement such unless there is interest.

Hm... you are right. Let me do EditingBehavior first. If there is need, I will do client callback so each client has full control on whether and on what platform to turn it on.

[Xiaomei Ji]

Created attachment 91604 [details]
patch w/ layout test

Using EditingBehavior for platform differences.

[Alexey Proskuryakov]

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

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

> Source/WebCore/ChangeLog:10
> +        Tests: editing/selection/move-left-right-by-word.html
> +               platform/win/editing/selection/move-left-right-by-word.html

The tests shouldn't be in platform directories now. All platforms should test both behaviors.

> LayoutTests/platform/win/editing/selection/move-left-right-by-word.html:22
> +<script src="../../../../editing/selection/resources/move-left-right-by-word.js"></script>

Please don't split tests into html and js parts. I can see how this can be helpful when there are separate htmls in platform directories, but this can be one file that tests both behaviors.

[Xiaomei Ji]

(In reply to comment #9)
> (From update of attachment 91604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91604&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Tests: editing/selection/move-left-right-by-word.html
> > +               platform/win/editing/selection/move-left-right-by-word.html
> 
> The tests shouldn't be in platform directories now. All platforms should test both behaviors.


The test only tests sel.modify("move", "left"/"right", "word").

We could have only one test file but with different expectations in different platforms.
The reason why we have 2 test files is because I put test expectations inside the test file itself.

Ryosuke suggested to put test expectation inside test file so that if anyone breaks the test, the test reports "FAILED....." to avoid being blindly rebased.
I think it is a good idea, and I am also using the expectation to test word break stops at correct place on every visible position when the caret movement is following visual order.

When you look at the 2 test files, one is passing "visual" to runTest(), and the other is passing "logical" to runTest(). Seems that they are testing different behaviors.
But underlying, they both just test sel.modify("move", "left"/"right", "word").

The "visual" vs. "logical" parameter is just used to guide the direction of the modify as explained below.
Inside the test, on each line of text, I first set the initial selection postion to 0, then move the caret by word in one direction till reach the end, then move it to the opposite direction till end.

If the implementation of sel.modify() is moving caret by word in visual order, 
when initial position is 0, if the text is in LTR context, it needs to move "right" till end, then move "left" till end. If the text is in RTL context, it needs to move "left" till end, then move "right" till end.

If the implementation of sel.modify() is moving caret by word in logical order,
when initial position is 0, we always move "right" (which is equivalent to move "forward') till end, then move "left" (which is equivalent to move "backward") till end.

That is when "visual" vs. "logical" is used (to indicate whether we should move "left" or "right" first, then, move to the other direction).


> 
> > LayoutTests/platform/win/editing/selection/move-left-right-by-word.html:22
> > +<script src="../../../../editing/selection/resources/move-left-right-by-word.js"></script>
> 
> Please don't split tests into html and js parts. I can see how this can be helpful when there are separate htmls in platform directories, but this can be one file that tests both behaviors.

You are right. The js is extracted to be shared.

I realized that I should skip editing/selection/move-left-right-by-word.html in win/ and chromium-win/. Or else, I probably need to put this test under all other platforms.

[Ryosuke Niwa]

(In reply to comment #10)
> I realized that I should skip editing/selection/move-left-right-by-word.html in win/ and chromium-win/. Or else, I probably need to put this test under all other platforms.

No need. Just call layoutTestController.setEditingBehavior.

[Xiaomei Ji]

(In reply to comment #11)
> (In reply to comment #10)
> > I realized that I should skip editing/selection/move-left-right-by-word.html in win/ and chromium-win/. Or else, I probably need to put this test under all other platforms.
> 
> No need. Just call layoutTestController.setEditingBehavior.

does it mean I should have 3 test html files with 3 expectations while the test files and expectations of 'mac' and 'unix' are the same (with the only difference is the setEditingBehavior itself) ?

[Xiaomei Ji]

Created attachment 91704 [details]
patch w/ layout test

add 3 layout tests for 3 editingBehaviors per comment #4.
the layout test and expectation of 'mac' and 'unix' are almost the same (except layoutTestContoller.setEditingBehavior).

[Xiaomei Ji]

Created attachment 91754 [details]
patch w/ layout test

add 3 layout tests for 3 editingBehaviors per comment #11.
the layout test and expectation of 'mac' and 'unix' are almost the same (except layoutTestContoller.setEditingBehavior).

And put those in Skipped list for qt-wk2 and mac-wk2 since layoutTestController.setEditingBehavior is not implemented.

[Xiaomei Ji]

Created attachment 94278 [details]
patch w/ layout test

I will ask Jeremy and Progame to test it.

To avoid regression in pure LTR text in LTR context, I could short-circuit it (check if all the leave boxes in a RootInlineBox are LTR boxes and the containing block is LTR, fallback to move caret by word logically). But I am not sure whether it is worth yet.

[Ryosuke Niwa]

(In reply to comment #15)
> Created an attachment (id=94278) [details]
> patch w/ layout test
> 
> I will ask Jeremy and Progame to test it.
> 
> To avoid regression in pure LTR text in LTR context, I could short-circuit it (check if all the leave boxes in a RootInlineBox are LTR boxes and the containing block is LTR, fallback to move caret by word logically). But I am not sure whether it is worth yet.

Yeah, I don't think that's necessary but we should do some manual testing before landing this patch as you suggested.

[Xiaomei Ji]

Created attachment 117720 [details]
patch w/ layout test

[WebKit Review Bot]

Attachment 117720 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/editing/FrameSelection.cpp:633:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/editing/FrameSelection.cpp:800:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xiaomei Ji]

Created attachment 117723 [details]
patch w/ layout test

fix style error.

[Early Warning System Bot]

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

Attachment 117723 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10728215

[WebKit Review Bot]

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

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

[Ryosuke Niwa]

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

Doesn't this mean some other editing tests that use modify("move", *, "word") will start failing?

[Ryosuke Niwa]

+aroben since this will also affect Apple's Windows port.

[Adam Roben (:aroben)]

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

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

> Source/WebCore/ChangeLog:10
> +        Turn on move caret by word visually for windows platform.
> +        https://bugs.webkit.org/show_bug.cgi?id=59652
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implements platform differences using EditingBehavior.
> +
> +        Test: editing/selection/move-left-right-by-word-mac.html

This is a bit too terse. It would be good to explain what the behavior without this patch is, what the behavior with this patch is, and which behavior (if either) matches other browsers.

[Xiaomei Ji]

Created attachment 117957 [details]
patch w/ layout test

Updated the patch to fix qt-ews, cr-ews error.

(In reply to comment #22)
> (From update of attachment 117723 [details])
> Doesn't this mean some other editing tests that use modify("move", *, "word") will start failing?

we do not seem to have such  tests before.



(In reply to comment #24)
> (From update of attachment 117723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117723&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Turn on move caret by word visually for windows platform.
> > +        https://bugs.webkit.org/show_bug.cgi?id=59652
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Implements platform differences using EditingBehavior.
> > +
> > +        Test: editing/selection/move-left-right-by-word-mac.html
> 
> This is a bit too terse. It would be good to explain what the behavior without this patch is, what the behavior with this patch is, and which behavior (if either) matches other browsers.

Added comments per suggestion.

[WebKit Review Bot]

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

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

New failing tests:
editing/selection/caret-mode-paragraph-keys-navigation.html

[Xiaomei Ji]

Created attachment 118132 [details]
patch w/ layout test

comparing with the r+ patch, this one:
1. fixes compile warning in qt and linux (comment #20 and #21).
2. add detailed comments in ChangLog (comment #24)
3. fixing failing test in cr-linux (comment #26). fixing an initialization bug in cr drt, mark the failing test tests mac editing behavior.

[WebKit Review Bot]

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

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

New failing tests:
fast/dom/Document/CaretRangeFromPoint/caretRangeFromPoint-in-zoom-and-scroll.html
fast/repaint/japanese-rl-selection-repaint.html
fast/text/international/khmer-selection.html
fast/forms/focus-selection-textarea.html
editing/selection/after-line-break.html

[Xiaomei Ji]

Created attachment 118149 [details]
patch w/ layout test

(In reply to comment #27)
> Created an attachment (id=118132) [details]
> patch w/ layout test
> 
> comparing with the r+ patch, this one:
> 1. fixes compile warning in qt and linux (comment #20 and #21).
> 2. add detailed comments in ChangLog (comment #24)
> 3. fixing failing test in cr-linux (comment #26). fixing an initialization bug in cr drt, mark the failing test tests mac editing behavior.

hm... I am not going to change the cr drt (initialize editingBehavior in WebPreferences) to avoid test failure in comment #28. maybe that is intentional?

[Xiaomei Ji]

Committed r102252: <http://trac.webkit.org/changeset/102252>