Bug 60430

Summary: selectstart is not fired when selection was created by arrow keys
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: arko, japhet, kaustubh.ra, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60424    
Attachments:
Description Flags
demo
none
Patch
rniwa: review-
Updated patch
rniwa: review-
Updated patch
rniwa: review-
Updated patch
rniwa: review+, rniwa: commit-queue-
Updated patch none

Description Ryosuke Niwa 2011-05-06 21:46:32 PDT
In WebKit, selectstart isn't fired when a range selection is created by arrow keys.
Comment 1 Ryosuke Niwa 2011-05-06 21:46:49 PDT
Created attachment 92685 [details]
demo
Comment 2 Arko Saha 2011-09-07 03:25:54 PDT
Created attachment 106564 [details]
Patch

Send selectStart DOM event if old selection is empty and selection type is AlterationExtend.
Comment 3 Kaustubh Atrawalkar 2011-09-07 03:26:57 PDT
Ryosuke, can u review patch ? Thanks.
Comment 4 Ryosuke Niwa 2011-09-07 03:40:11 PDT
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?
Comment 5 Arko Saha 2011-09-07 05:15:36 PDT
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.
Comment 6 Ryosuke Niwa 2011-09-07 09:15:04 PDT
(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.
Comment 7 Kaustubh Atrawalkar 2011-09-07 10:30:52 PDT
(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.
Comment 8 Ryosuke Niwa 2011-09-07 11:04:49 PDT
(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.
Comment 9 Kaustubh Atrawalkar 2011-09-07 20:09:03 PDT
(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.
Comment 10 Arko Saha 2011-09-08 02:46:32 PDT
Created attachment 106717 [details]
Updated patch
Comment 11 Ryosuke Niwa 2011-09-08 09:43:26 PDT
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).
Comment 12 Arko Saha 2011-09-08 23:52:33 PDT
> 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.
Comment 13 Arko Saha 2011-09-08 23:53:43 PDT
Created attachment 106844 [details]
Updated patch
Comment 14 Ryosuke Niwa 2011-09-09 01:03:05 PDT
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.
Comment 15 Arko Saha 2011-09-09 03:14:26 PDT
Created attachment 106853 [details]
Updated patch
Comment 16 Kaustubh Atrawalkar 2011-09-11 21:07:04 PDT
(In reply to comment #15)
> Created an attachment (id=106853) [details]
> Updated patch

Ryosuke, can you review updated patch?
Comment 17 Ryosuke Niwa 2011-09-11 22:14:41 PDT
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.
Comment 18 Kaustubh Atrawalkar 2011-09-11 23:08:18 PDT
> 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.
>
Comment 19 Arko Saha 2011-09-12 00:47:33 PDT
> 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.
Comment 20 Arko Saha 2011-09-12 00:48:32 PDT
Created attachment 107027 [details]
Updated patch
Comment 21 WebKit Review Bot 2011-09-12 10:54:54 PDT
Comment on attachment 107027 [details]
Updated patch

Clearing flags on attachment: 107027

Committed r94966: <http://trac.webkit.org/changeset/94966>
Comment 22 WebKit Review Bot 2011-09-12 10:54:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Nate Chapin 2011-09-12 12:59:26 PDT
(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
Comment 24 Ryosuke Niwa 2011-09-12 14:06:35 PDT
Fixed the test for Mac in http://trac.webkit.org/changeset/94977.