Bug 59652 - turn on move caret by word visually for windows platform.
: turn on move caret by word visually for windows platform.
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 25298
  Show dependency treegraph
 
Reported: 2011-04-27 16:24 PST by
Modified: 2011-12-07 11:09 PST (History)


Attachments
patch w/ layout test (69.80 KB, patch)
2011-04-27 16:33 PST, Xiaomei Ji
ap: review-
Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (95.48 KB, patch)
2011-04-28 17:35 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (102.78 KB, patch)
2011-04-29 11:10 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (104.28 KB, patch)
2011-04-29 15:33 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (128.26 KB, patch)
2011-05-20 15:24 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (25.58 KB, patch)
2011-12-02 17:54 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (25.58 KB, patch)
2011-12-02 18:01 PST, Xiaomei Ji
rniwa: review+
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (26.80 KB, patch)
2011-12-05 16:54 PST, Xiaomei Ji
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (28.62 KB, patch)
2011-12-06 16:00 PST, Xiaomei Ji
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (27.50 KB, patch)
2011-12-06 17:14 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-27 16:24:17 PST
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".
------- Comment #1 From 2011-04-27 16:33:17 PST -------
Created an attachment (id=91373) [details]
patch w/ layout test
------- Comment #2 From 2011-04-27 21:21:23 PST -------
(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.
------- Comment #3 From 2011-04-27 21:34:29 PST -------
(In reply to comment #2)
> (From update of attachment 91373 [details] [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.
------- Comment #4 From 2011-04-27 22:00:42 PST -------
Yes, that's probably even better.
------- Comment #5 From 2011-04-28 12:24:45 PST -------
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?
------- Comment #6 From 2011-04-28 14:16:56 PST -------
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.
------- Comment #7 From 2011-04-28 14:30:35 PST -------
(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.
------- Comment #8 From 2011-04-28 17:35:43 PST -------
Created an attachment (id=91604) [details]
patch w/ layout test

Using EditingBehavior for platform differences.
------- Comment #9 From 2011-04-28 17:41:53 PST -------
(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.

> 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.
------- Comment #10 From 2011-04-28 22:16:39 PST -------
(In reply to comment #9)
> (From update of attachment 91604 [details] [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.
------- Comment #11 From 2011-04-28 22:22:52 PST -------
(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.
------- Comment #12 From 2011-04-28 22:47:43 PST -------
(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) ?
------- Comment #13 From 2011-04-29 11:10:40 PST -------
Created an attachment (id=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).
------- Comment #14 From 2011-04-29 15:33:23 PST -------
Created an attachment (id=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.
------- Comment #15 From 2011-05-20 15:24:04 PST -------
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.
------- Comment #16 From 2011-05-20 15:29:26 PST -------
(In reply to comment #15)
> Created an attachment (id=94278) [details] [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.
------- Comment #17 From 2011-12-02 17:54:56 PST -------
Created an attachment (id=117720) [details]
patch w/ layout test
------- Comment #18 From 2011-12-02 17:58:22 PST -------
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.
------- Comment #19 From 2011-12-02 18:01:27 PST -------
Created an attachment (id=117723) [details]
patch w/ layout test

fix style error.
------- Comment #20 From 2011-12-02 18:28:16 PST -------
(From update of attachment 117723 [details])
Attachment 117723 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10728215
------- Comment #21 From 2011-12-02 18:46:06 PST -------
(From update of attachment 117723 [details])
Attachment 117723 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10734166
------- Comment #22 From 2011-12-02 20:26:57 PST -------
(From update of attachment 117723 [details])
Doesn't this mean some other editing tests that use modify("move", *, "word") will start failing?
------- Comment #23 From 2011-12-02 20:27:34 PST -------
+aroben since this will also affect Apple's Windows port.
------- Comment #24 From 2011-12-05 06:59:26 PST -------
(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.
------- Comment #25 From 2011-12-05 16:54:26 PST -------
Created an attachment (id=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] [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] [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.
------- Comment #26 From 2011-12-05 18:10:15 PST -------
(From update of attachment 117957 [details])
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
------- Comment #27 From 2011-12-06 16:00:37 PST -------
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.
------- Comment #28 From 2011-12-06 16:45:18 PST -------
(From update of attachment 118132 [details])
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
------- Comment #29 From 2011-12-06 17:14:03 PST -------
Created an attachment (id=118149) [details]
patch w/ layout test

(In reply to comment #27)
> Created an attachment (id=118132) [details] [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?
------- Comment #30 From 2011-12-07 11:09:46 PST -------
Committed r102252: <http://trac.webkit.org/changeset/102252>