Bug 49511

tracked as http://crbug.com/46518 in the chromium bug tracker

Created attachment 74704 [details] Patch

Created attachment 75041 [details] Patch

Now with a better test case that covers all the cases.

Comment on attachment 75041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75041&action=review > WebCore/editing/SelectionController.cpp:285 > - m_selection.setBase(start); > - m_selection.setExtent(end); > + if (directionOfEnclosingBlock() == LTR) { > + m_selection.setBase(start); > + m_selection.setExtent(end); > + } else { > + m_selection.setBase(end); > + m_selection.setExtent(start); > + } I don't think we should be doing this for DirectionRight. New code seems to make sense only for DirectionForward. Dan & Xiaomei, could you comment on this? > WebCore/editing/SelectionController.cpp:295 > - m_selection.setBase(end); > - m_selection.setExtent(start); > + if (directionOfEnclosingBlock() == LTR) { > + m_selection.setBase(end); > + m_selection.setExtent(start); > + } else { > + m_selection.setBase(start); > + m_selection.setExtent(end); > + } Ditto. > WebCore/editing/SelectionController.cpp:425 > - pos = VisiblePosition(m_selection.end(), m_selection.affinity()); > + if (directionOfEnclosingBlock() == LTR) > + pos = VisiblePosition(m_selection.end(), m_selection.affinity()); > + else > + pos = VisiblePosition(m_selection.start(), m_selection.affinity()); You could write this as: pos = VisiblePosition(directionOfEnclosingBlock() == LTR ? m_selection.end() : m_selection.start(), m_selection.affinity()); But maybe this is less readable. > LayoutTests/editing/selection/rtl-move-selection-right-left.html:71 > + testSelectionChange(setSelection, "move", "right", 0, undefined); > + testSelectionChange(setSelection, "move", "left", undefined, 0); Why don't we test end & start offsets for these two tests? I think we should.

Comment on attachment 75041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75041&action=review >> WebCore/editing/SelectionController.cpp:285 >> + } > > I don't think we should be doing this for DirectionRight. New code seems to make sense only for DirectionForward. > > Dan & Xiaomei, could you comment on this? Is it the other way around? the new change should only apply for DirectionRight/Left. > WebCore/editing/SelectionController.cpp:426 > else you probably forgot to enclose them into "{......}" > WebCore/editing/SelectionController.cpp:579 > + pos = VisiblePosition(m_selection.end(), m_selection.affinity()); enclose in { }.

Created attachment 78065 [details] Patch

(In reply to comment #7) > Created an attachment (id=78065) [details] > Patch > > LayoutTests/editing/selection/rtl-move-selection-right-left.html:71 > > + testSelectionChange(setSelection, "move", "right", 0, undefined); > > + testSelectionChange(setSelection, "move", "left", undefined, 0); > Why don't we test end & start offsets for these two tests? I think we should. You're right. Fixed. > (From update of attachment 75041 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75041&action=review > > >> WebCore/editing/SelectionController.cpp:285 > >> + } > > > > I don't think we should be doing this for DirectionRight. New code seems to make sense only for DirectionForward. > > > > Dan & Xiaomei, could you comment on this? > > Is it the other way around? the new change should only apply for DirectionRight/Left. Xiaomei is right, it's really just for Right/Left. I've fixed willBeModified to only consider LTR for Left/Right. > > WebCore/editing/SelectionController.cpp:426 > > else > > you probably forgot to enclose them into "{......}" > > > WebCore/editing/SelectionController.cpp:579 > > + pos = VisiblePosition(m_selection.end(), m_selection.affinity()); > > enclose in { }. Looks like a bug in WebKit Style as well! Fixed :)

Comment on attachment 78065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78065&action=review r=me but cq- because I requested minor fixes. > WebCore/editing/SelectionController.cpp:350 > switch (direction) { > case DirectionRight: > + if (directionOfEnclosingBlock() == LTR) { > + m_selection.setBase(start); > + m_selection.setExtent(end); > + } else { > + m_selection.setBase(end); > + m_selection.setExtent(start); > + } > + break; > case DirectionForward: > m_selection.setBase(start); > m_selection.setExtent(end); > break; > + if (directionOfEnclosingBlock() == LTR) { > + m_selection.setBase(end); > + m_selection.setExtent(start); > + } else { > + m_selection.setBase(start); > + m_selection.setExtent(end); > + } > + break; I'm not particularly happy about these duplicates calls to setBase & setExtent. Can we define a boolean variable like baseIsFirst and make this switch statement update it? e.g. move setBase/setExtent out of switch and put that into a separate if-else clauses. If you do that, then you should probably be modifying the code for the case where m_isDirectional is true as well. > LayoutTests/editing/selection/rtl-move-selection-right-left.html:28 > + eventSender.mouseMoveTo(testContainer.offsetLeft + 2, y); I'm worried that +2 will have an undesired effects on some platforms and the test fails. You may want to consider adding some padding to this testContainer and drag from a padding on the left to a padding on the right. > LayoutTests/editing/selection/rtl-move-selection-right-left.html:75 > + window.layoutTestController.dumpAsText(); > + testSelectionChange(dragSelection, "extend", "right", -1, 0); > + testSelectionChange(dragSelection, "extend", "left", 1, 0); As we talked in person, you probably want to comment on what's the expected behavior here. Something in the line of "When text is selection by mouse, there's no directionality and extending to the right should move the left end to the right and extending to the left should move the right end to the left on RTL text when the entire text is selected".

i may be understanding things wrong since i don't know the code very well... but it sound like you are also changing the behavior if extending the selection and not just the case of deselecting by pressing left/right? if that is so i am a bit worried since extending RTL selections already works as is should AFAIK (at least as expected under windows, xji fixed it) umm... am i wrong?

(In reply to comment #10) > i may be understanding things wrong since i don't know the code very well... but it sound like you are also changing the behavior if extending the selection and not just the case of deselecting by pressing left/right? > > if that is so i am a bit worried since extending RTL selections already works as is should AFAIK (at least as expected under windows, xji fixed it) > > umm... am i wrong? This won't affect Windows at all. This change for extending selections only affects non-directional selections, which is to match NSTextView's behavior on Macs. Check out the comment here: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L743

Committed r76312: <http://trac.webkit.org/changeset/76312>

http://trac.webkit.org/changeset/76312 might have broken Qt Linux Release minimal