Bug 60430 – selectstart is not fired when selection was created by arrow keys

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ryosuke Niwa]

In WebKit, selectstart isn't fired when a range selection is created by arrow keys.

[Ryosuke Niwa]

Created attachment 92685 [details]
demo

[Arko Saha]

Created attachment 106564 [details]
Patch

Send selectStart DOM event if old selection is empty and selection type is AlterationExtend.

[Kaustubh Atrawalkar]

Ryosuke, can u review patch ? Thanks.

[Ryosuke Niwa]

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?

[Arko Saha]

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.

[Ryosuke Niwa]

(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.

[Kaustubh Atrawalkar]

(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.

[Ryosuke Niwa]

(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.

[Kaustubh Atrawalkar]

(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.

[Arko Saha]

Created attachment 106717 [details]
Updated patch

[Ryosuke Niwa]

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).

[Arko Saha]

> 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.

[Arko Saha]

Created attachment 106844 [details]
Updated patch

[Ryosuke Niwa]

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.

[Arko Saha]

Created attachment 106853 [details]
Updated patch

[Kaustubh Atrawalkar]

(In reply to comment #15)
> Created an attachment (id=106853) [details]
> Updated patch

Ryosuke, can you review updated patch?

[Ryosuke Niwa]

Comment on attachment 106853 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106853&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Arko Saha  <[e-mail]> and Kaustubh Atrawalkar  <[e-mail]>

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.

[Kaustubh Atrawalkar]

> 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.
>

[Arko Saha]

> 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.

[Arko Saha]

Created attachment 107027 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 107027 [details]
Updated patch

Clearing flags on attachment: 107027

Committed r94966: <http://trac.webkit.org/changeset/94966>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

(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

[Ryosuke Niwa]

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