enable ctrl-arrow move by word visually in other platforms (besides Windows)
Created attachment 139101 [details] patch w/ layout test
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
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
What "other platforms" does this change? If you're changing Mac, then why?
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?
The change is desirable for Mac. Thanks!
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 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 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.
Created attachment 139541 [details] patch w/ layout test
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 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 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 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    opq" collapsed to "hij 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.
Created attachment 139671 [details] patch w/ layout test Thanks for the review! Comments addressed.
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 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.
(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.
Created attachment 139689 [details] patch w/ layout test address comment #16 (adding macro DEFINED_IS_WORD_TEXT_BREAK).
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.
Created attachment 139707 [details] patch w/ layout test using USE(ICU_UNICODE).
Comment on attachment 139707 [details] patch w/ layout test Clearing flags on attachment: 139707 Committed r115788: <http://trac.webkit.org/changeset/115788>
All reviewed patches have been landed. Closing bug.
*** Bug 65773 has been marked as a duplicate of this bug. ***