Bug 59652 - turn on move caret by word visually for windows platform.
Summary: turn on move caret by word visually for windows platform.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2011-04-27 16:24 PDT by Xiaomei Ji
Modified: 2011-12-07 11:09 PST (History)
9 users (show)

See Also:


Attachments
patch w/ layout test (69.80 KB, patch)
2011-04-27 16:33 PDT, Xiaomei Ji
ap: review-
Details | Formatted Diff | Diff
patch w/ layout test (95.48 KB, patch)
2011-04-28 17:35 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (102.78 KB, patch)
2011-04-29 11:10 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (104.28 KB, patch)
2011-04-29 15:33 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (128.26 KB, patch)
2011-05-20 15:24 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (25.58 KB, patch)
2011-12-02 17:54 PST, Xiaomei Ji
no flags 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-
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-
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-
Details | Formatted Diff | Diff
patch w/ layout test (27.50 KB, patch)
2011-12-06 17:14 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-04-27 16:24:17 PDT
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 Xiaomei Ji 2011-04-27 16:33:17 PDT
Created attachment 91373 [details]
patch w/ layout test
Comment 2 Alexey Proskuryakov 2011-04-27 21:21:23 PDT
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.
Comment 3 Ryosuke Niwa 2011-04-27 21:34:29 PDT
(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.
Comment 4 Alexey Proskuryakov 2011-04-27 22:00:42 PDT
Yes, that's probably even better.
Comment 5 Xiaomei Ji 2011-04-28 12:24:45 PDT
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 Alexey Proskuryakov 2011-04-28 14:16:56 PDT
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 Xiaomei Ji 2011-04-28 14:30:35 PDT
(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 Xiaomei Ji 2011-04-28 17:35:43 PDT
Created attachment 91604 [details]
patch w/ layout test

Using EditingBehavior for platform differences.
Comment 9 Alexey Proskuryakov 2011-04-28 17:41:53 PDT
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.
Comment 10 Xiaomei Ji 2011-04-28 22:16:39 PDT
(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.
Comment 11 Ryosuke Niwa 2011-04-28 22:22:52 PDT
(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 Xiaomei Ji 2011-04-28 22:47:43 PDT
(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 Xiaomei Ji 2011-04-29 11:10:40 PDT
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).
Comment 14 Xiaomei Ji 2011-04-29 15:33:23 PDT
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.
Comment 15 Xiaomei Ji 2011-05-20 15:24:04 PDT
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.
Comment 16 Ryosuke Niwa 2011-05-20 15:29:26 PDT
(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.
Comment 17 Xiaomei Ji 2011-12-02 17:54:56 PST
Created attachment 117720 [details]
patch w/ layout test
Comment 18 WebKit Review Bot 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 Xiaomei Ji 2011-12-02 18:01:27 PST
Created attachment 117723 [details]
patch w/ layout test

fix style error.
Comment 20 Early Warning System Bot 2011-12-02 18:28:16 PST
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
Comment 21 WebKit Review Bot 2011-12-02 18:46:06 PST
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
Comment 22 Ryosuke Niwa 2011-12-02 20:26:57 PST
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?
Comment 23 Ryosuke Niwa 2011-12-02 20:27:34 PST
+aroben since this will also affect Apple's Windows port.
Comment 24 Adam Roben (:aroben) 2011-12-05 06:59:26 PST
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.
Comment 25 Xiaomei Ji 2011-12-05 16:54:26 PST
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.
Comment 26 WebKit Review Bot 2011-12-05 18:10:15 PST
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
Comment 27 Xiaomei Ji 2011-12-06 16:00:37 PST
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.
Comment 28 WebKit Review Bot 2011-12-06 16:45:18 PST
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
Comment 29 Xiaomei Ji 2011-12-06 17:14:03 PST
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?
Comment 30 Xiaomei Ji 2011-12-07 11:09:46 PST
Committed r102252: <http://trac.webkit.org/changeset/102252>