Summary: | turn on move caret by word visually for windows platform. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xiaomei Ji <xji> | ||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | amir.aharoni, ap, aroben, dglazkov, enrica, mitz, rniwa, tkent, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 25298 | ||||||||||||||||||||||||
Attachments: |
|
Description
Xiaomei Ji
2011-04-27 16:24:17 PDT
Created attachment 91373 [details]
patch w/ layout test
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.
(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. Yes, that's probably even better. 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? 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. (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. Created attachment 91604 [details]
patch w/ layout test
Using EditingBehavior for platform differences.
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. (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. (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. (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) ? 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). 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. 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.
(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. Created attachment 117720 [details]
patch w/ layout test
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.
Created attachment 117723 [details]
patch w/ layout test
fix style error.
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 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 on attachment 117723 [details]
patch w/ layout test
Doesn't this mean some other editing tests that use modify("move", *, "word") will start failing?
+aroben since this will also affect Apple's Windows port. 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. 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 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 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 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 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? Committed r102252: <http://trac.webkit.org/changeset/102252> |