Bug 60430

Created attachment 92685 [details] demo

Created attachment 106564 [details] Patch Send selectStart DOM event if old selection is empty and selection type is AlterationExtend.

Ryosuke, can u review patch ? Thanks.

Comment on attachment 106564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106564&action=review > Source/WebCore/editing/FrameSelection.cpp:886 > + if (oldSelection.isCaret()) { > + if (!(dispatchedSelectStart = dispatchSelectStart())) > + break; > + } This is too late. m_selection.m_isDirectional has already been modified at this point. r- because of this. You should probably do this in the if-statement at the beginning of this function. > Source/WebCore/editing/FrameSelection.cpp:1923 > + if (isContentEditable()) { > + root = highestEditableRoot(m_selection.start()); > + if (Node* shadowRoot = m_selection.nonBoundaryShadowTreeRootNode()) > + selectStartTarget = shadowRoot->shadowAncestorNode(); > + else > + selectStartTarget = root.get(); > + } else { > + root = m_selection.nonBoundaryShadowTreeRootNode(); > + if (root) > + selectStartTarget = root->shadowAncestorNode(); > + else { > + root = document->documentElement(); > + selectStartTarget = document->body(); > + } > + } > + if (!root || !selectStartTarget) > + return true; Why do we need all this code?

Hi Ryosuke, Thanks for the review. > > This is too late. m_selection.m_isDirectional has already been modified at > this point. r- because of this. You should probably do this in the if- > statement at the beginning of this function. We can do this at the beginning of this function. But in IMHO if we keep this in the if-statement at the beginning, selectStart might not execute if modify() function called by JS. > Why do we need all this code? Here we need to find out selectStart target if the element is content editable, Similar as in case of selectAll(), how the selectStart event is fired.

(In reply to comment #5) > > This is too late. m_selection.m_isDirectional has already been modified at > > this point. r- because of this. You should probably do this in the if- > > statement at the beginning of this function. > > We can do this at the beginning of this function. But in IMHO if we keep this in the if-statement at the beginning, selectStart might not execute if modify() function called by JS. I thought selectstart doesn't fire for programatically set selection anyway. I don't think we currently fire selectstart when a collapsed selection is replaced by a range selection for example, and this behavior matches that of MSIE. You should definitely check MSIE's behavior though. > > Why do we need all this code? > > Here we need to find out selectStart target if the element is content editable, Similar as in case of selectAll(), how the selectStart event is fired. We don't do this elsewhere for firing selectstart so I'd rather not add such a code for this specific case. If this is a bug, then we should fix all those places where we fire selectstart at once.

(In reply to comment #6) > I thought selectstart doesn't fire for programatically set selection anyway. I don't think we currently fire selectstart when a collapsed selection is replaced by a range selection for example, and this behavior matches that of MSIE. You should definitely check MSIE's behavior though. > IE sends out selectstart when a range selection is created. IE fires selectstart only when a user starts extending the selection. I think we should match with IE specs and send out selectStart only in user triggered modify calls for selection. We will change and put that inside the first if statement. > > > Why do we need all this code? > > > > Here we need to find out selectStart target if the element is content editable, Similar as in case of selectAll(), how the selectStart event is fired. > > We don't do this elsewhere for firing selectstart so I'd rather not add such a code for this specific case. If this is a bug, then we should fix all those places where we fire selectstart at once. I think this is being done for selectAll case right now. So we added the code to retrieve the selectStartTarget node where we need to dispatch the event. In case of text area it might be shadow node. So we need to take the highest editable node as target. Please suggest and correct us if anything missing.

(In reply to comment #7) > > We don't do this elsewhere for firing selectstart so I'd rather not add such a code for this specific case. If this is a bug, then we should fix all those places where we fire selectstart at once. > > I think this is being done for selectAll case right now. So we added the code to retrieve the selectStartTarget node where we need to dispatch the event. In case of text area it might be shadow node. So we need to take the highest editable node as target. Please suggest and correct us if anything missing. But then it appears that we have a bug in other code paths that fire selectstart event. See EventHandler.cpp. If anything, adding such code should be done in a separate patch.

(In reply to comment #8) > > But then it appears that we have a bug in other code paths that fire selectstart event. See EventHandler.cpp. If anything, adding such code should be done in a separate patch. Ok. That seems to be fair. Let's make that code path for firing selectStart visit again and get that in seperate patch.

Created attachment 106717 [details] Updated patch

Comment on attachment 106717 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=106717&action=review > Source/WebCore/editing/FrameSelection.cpp:821 > + if (alter == AlterationExtend && m_selection.isCaret() && !dispatchSelectStart()) You should check that new selection is range. You can do that by using trialFrameSelection below. > Source/WebCore/editing/FrameSelection.cpp:1896 > + Node* selectStartTarget = m_frame->document()->getSelection()->focusNode(); This seems like an unnecessary indirection. I'd just do extent().containerNode(). > Source/WebCore/editing/FrameSelection.h:247 > + bool dispatchSelectStart(); > + This method should be private for now. > LayoutTests/fast/events/selectstart-by-arrow-keys.html:11 > +if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > +} No curly brackets around one line statement. > LayoutTests/fast/events/selectstart-by-arrow-keys.html:16 > +div.addEventListener('selectstart', function (event) { passed = true; }); We should probably ensure that selectstart event is fired exactly once (not twice or three times).

> You should check that new selection is range. You can do that by using trialFrameSelection below. > Done. > This seems like an unnecessary indirection. I'd just do extent().containerNode(). > Incorporated this comment. > This method should be private for now. > Right, moved this method to private. > No curly brackets around one line statement. > Removed curly brackets. > We should probably ensure that selectstart event is fired exactly once (not twice or three times). I have modified the test case to ensure that selectstart event is fired exactly once.

Created attachment 106844 [details] Updated patch

Comment on attachment 106844 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=106844&action=review > Source/WebCore/editing/FrameSelection.cpp:825 > + VisibleSelection trialVisibleSelection = trialFrameSelection.selection(); > + bool change = shouldChangeSelection(trialVisibleSelection); Why do we need to make a copy of VisibleSelection here? > LayoutTests/fast/events/selectstart-by-arrow-keys.html:13 > +div.addEventListener('selectstart', function (event) { selectStartCount++; }); We also need a test for preventing the default action. > LayoutTests/fast/events/selectstart-by-arrow-keys.html:58 > + document.write('PASS.'); Nit: wrong indentation. Also maybe get rid of . at the end? > LayoutTests/fast/events/selectstart-by-arrow-keys.html:59 > + document.writeln('<br />'); Don't think we should use XML-style br.

Created attachment 106853 [details] Updated patch

(In reply to comment #15) > Created an attachment (id=106853) [details] > Updated patch Ryosuke, can you review updated patch?

Comment on attachment 106853 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=106853&action=review > Source/WebCore/ChangeLog:1 > +2011-09-09 Arko Saha <arko@motorola.com> and Kaustubh Atrawalkar <kaustubh@motorola.com> We don't normally list two authors like this. If you're like to live a credit, then please mention it in the description below (after/before "Fire selectstart when..."). > LayoutTests/fast/events/selectstart-by-arrow-keys-prevent-default.html:17 > +eventSender.keyDown("rightArrow", ["shiftKey"]); Because this line will throw an exception outside of DRT, you should consider wrapping it in if and explain how you'd test manually on a browser. > LayoutTests/fast/events/selectstart-by-arrow-keys.html:19 > +eventSender.keyDown("rightArrow"); > +logResult('Check (Right arrow)', 0); Ditto about wrapping these in if and explaining how to test manually. If manual test is not possible, you can also say so to clarify.

> We don't normally list two authors like this. If you're like to live a credit, then please mention it in the description below (after/before "Fire selectstart when..."). > We listed that referring to various commits in WebKit like this - http://trac.webkit.org/changeset/82570 > > LayoutTests/fast/events/selectstart-by-arrow-keys-prevent-default.html:17 > > +eventSender.keyDown("rightArrow", ["shiftKey"]); > > Because this line will throw an exception outside of DRT, you should consider wrapping it in if and explain how you'd test manually on a browser. Ok. Will make it. >

> We don't normally list two authors like this. If you're like to live a credit, then please mention it in the description below (after/before "Fire selectstart when..."). Done. > Because this line will throw an exception outside of DRT, you should consider wrapping it in if and explain how you'd test manually on a browser. Wrapped DRT specific code in if condition. Also added how to test it manually.

Created attachment 107027 [details] Updated patch

Comment on attachment 107027 [details] Updated patch Clearing flags on attachment: 107027 Committed r94966: <http://trac.webkit.org/changeset/94966>

All reviewed patches have been landed. Closing bug.

(In reply to comment #21) > (From update of attachment 107027 [details]) > Clearing flags on attachment: 107027 > > Committed r94966: <http://trac.webkit.org/changeset/94966> FYI, selectstart-by-arrow-keys.html is failing on mac (both safari and chormium). See, e.g., http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fevents%2Fselectstart-by-arrow-keys.html&showExpectations=true

Fixed the test for Mac in http://trac.webkit.org/changeset/94977.