Bug 19489 – selectstart is fired for every mouse move

• Component: UI Events
• Status: RESOLVED FIXED

[Anantha Keesara]

I. Steps:
 -----------
 
 1. Launch Safari
 
 2. Launch the testcase
 
 3. Select text by left_click and dragging the mouse.
 
 
 
 II. Issue:
 -----------------
 body.onselectstart should be fired only once for each selection. It should be fired (once) as soon as the user starts selection.
 
 However, Safari fires it many times for every charecter selected during selection.
 
 
 
 
 III. Other browsers:
 -----------------------
 IE7 : Ok
 FF3 : doesn't support it 
 Opera 9.27: doesn't support it 
 
 
 IV. Safari nightly tested: version 3.1.1(525.17 )- r34278.
 
 Not working properly on Safari.
 
 
 
 V. Safari screenshot : Avalible

[Anantha Keesara]

Created attachment 21630 [details]
Screenshot

[Anantha Keesara]

Created attachment 21640 [details]
reduction

[Ryosuke Niwa]

Created attachment 92919 [details]
fixes the bug

[Ryosuke Niwa]

Comment on attachment 92919 [details]
fixes the bug

Oops, this patch is missing some tests.  Will upload a new patch today.

[Ryosuke Niwa]

Created attachment 92973 [details]
Added the forgotten tests

[Hajime Morrita]

Comment on attachment 92973 [details]
Added the forgotten tests

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

In my understanding, old code didn't fire the event for double and triple click, but new one does. Is that right?
If so, we could separate patch to, one for fixing this bug, another for handle double and triple click case.

> LayoutTests/fast/events/selectstart-by-double-triple-clicks-expected.txt:11
> +"

Are these failures intentional?

> Source/WebCore/page/EventHandler.cpp:279
> +}

It looks a bit weird that shouldSomething()  has a side effect (dispatching event), I know some existing codebase does similar though.
I personally don't like give canXxx side-effect, but at least it seems a existing convention.

[Ryosuke Niwa]

Comment on attachment 92973 [details]
Added the forgotten tests

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

>> LayoutTests/fast/events/selectstart-by-double-triple-clicks-expected.txt:11
>> +"
> 
> Are these failures intentional?

No. This is odd.

>> Source/WebCore/page/EventHandler.cpp:279
>> +}
> 
> It looks a bit weird that shouldSomething()  has a side effect (dispatching event), I know some existing codebase does similar though.
> I personally don't like give canXxx side-effect, but at least it seems a existing convention.

This is a convention used throughout WebKit.

[Ryosuke Niwa]

(In reply to comment #6)
> In my understanding, old code didn't fire the event for double and triple click, but new one does. Is that right?

It did fire in canMouseDownStartSelect.

> If so, we could separate patch to, one for fixing this bug, another for handle double and triple click case.

So no, we cannot split this patch into multiple pieces as it would introduce a regression.

[Hajime Morrita]

(In reply to comment #8)
> (In reply to comment #6)
> > In my understanding, old code didn't fire the event for double and triple click, but new one does. Is that right?
> 
> It did fire in canMouseDownStartSelect.
> 
> > If so, we could separate patch to, one for fixing this bug, another for handle double and triple click case.
> 
> So no, we cannot split this patch into multiple pieces as it would introduce a regression.
Oh I see. Thank you for the explanation.

[Ryosuke Niwa]

Created attachment 93106 [details]
fixed a test

[Ryosuke Niwa]

Eric, could you review the new patch again?  The one you r+ was totally out of date due to a bug being fixed and datagrid being removed.

[Ryosuke Niwa]

(In reply to comment #11)
> Eric, could you review the new patch again?  The one you r+ was totally out of date due to a bug being fixed and datagrid being removed.

Oops, please ignore me.  This was a comment for the bug 53564.

[Ryosuke Niwa]

Could anyone review this patch?

[Ryosuke Niwa]

Ping reviewers.

[Alexey Proskuryakov]

Comment on attachment 93106 [details]
fixed a test

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

I'm going to say r+, since this fixes the bug, and the code has never been great to start with - but I think that the code becomes somewhat more confusing after this patch.

> LayoutTests/fast/events/selectstart-by-double-triple-clicks.html:29
> +    document.write('This test requires eventSender');

A person opening the test in browser will not necessarily know what eventSender is.

> LayoutTests/fast/events/selectstart-by-drag.html:5
> +</p><span style='font-size: 50px; padding: 10px;' contenteditable>hello

I see that some of the tests use contenteditable, and some don't. Is there some system behind this, or just randomness to slightly increase coverage? FWIW, I don't have a problem with the latter.

> Source/WebCore/ChangeLog:24
> +        Test: fast/events/selectstart-by-double-triple-clicks.html
> +              fast/events/selectstart-by-drag.html
> +              fast/events/selectstart-by-single-click-with-shift.html

A quick Web search reveals that in IE, the event is also dispatched when "changing the selection by script (through the selection object or with the select method)". Do we implement this, do we want this, and do we already test for this?

> Source/WebCore/page/EventHandler.cpp:273
> +static inline bool shouldStartSelection(Node* node)

This name seems very misleading for a function that dispatches a DOM event.

Besides hiding the fact that arbitrary changes can happen here, it's more like a "can" or "may" function - yet there is already Node::canStartSelection().

> Source/WebCore/page/EventHandler.cpp:291
> +    if (newSelection.isRange())
> +        m_selectionInitiationState = ExtendedSelection;
> +    else {
> +        granularity = CharacterGranularity;
> +        m_selectionInitiationState = PlacedCaret;
> +    }

The logic here is surprising in two ways:
1. If I extend the selection first by moving the mouse, and then move it back to original point, EventHandler apparently returns to PlacedCaret state. If the code doesn't care about state transitions, why does it differentiate between ExtendedSelection and PlacedCaret in m_selectionInitiationState? We could just look at current selection for the answer.
2. Is PlacedCaret also used when there is no caret (i.e. in non-editable text)?

> Source/WebCore/page/EventHandler.cpp:313
> -        setNonDirectionalSelectionIfNeeded(m_frame->selection(), newSelection, granularity);
> +        if (newSelection.isRange() && result.event().clickCount() == 2 && m_frame->editor()->isSelectTrailingWhitespaceEnabled()) 
> +            newSelection.appendTrailingWhitespace();
> +
> +        setSelectionIfPossible(innerNode, newSelection, WordGranularity);

Previously, the code used either character or word granularity, but now it always uses word. Why is that OK?

> Source/WebCore/page/EventHandler.cpp:331
> -        setNonDirectionalSelectionIfNeeded(m_frame->selection(), newSelection, granularity);
> +        setSelectionIfPossible(innerNode, newSelection, WordGranularity);

Ditto.

> Source/WebCore/page/EventHandler.cpp:369
> +    return setSelectionIfPossible(innerNode, newSelection, ParagraphGranularity);

Again, this used to do CharacterGranularity sometimes.

[Ryosuke Niwa]

Comment on attachment 93106 [details]
fixed a test

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

>> Source/WebCore/page/EventHandler.cpp:273
>> +static inline bool shouldStartSelection(Node* node)
> 
> This name seems very misleading for a function that dispatches a DOM event.
> 
> Besides hiding the fact that arbitrary changes can happen here, it's more like a "can" or "may" function - yet there is already Node::canStartSelection().

Will rename to dispatchSelectStart.

>> Source/WebCore/page/EventHandler.cpp:291
>> +    }
> 
> The logic here is surprising in two ways:
> 1. If I extend the selection first by moving the mouse, and then move it back to original point, EventHandler apparently returns to PlacedCaret state. If the code doesn't care about state transitions, why does it differentiate between ExtendedSelection and PlacedCaret in m_selectionInitiationState? We could just look at current selection for the answer.
> 2. Is PlacedCaret also used when there is no caret (i.e. in non-editable text)?

1. setSelectionIfPossible is NOT used for mouse drag, so this isn't a problem.  Maybe we'll need to give a better name here?
2. Yes. We do set collapsed selection in non-editable region despite it being invisible to users.  I'm not sure if that's a bug or not.

>> Source/WebCore/page/EventHandler.cpp:313
>> +        setSelectionIfPossible(innerNode, newSelection, WordGranularity);
> 
> Previously, the code used either character or word granularity, but now it always uses word. Why is that OK?

The code use to use CharacterGranularity whenever newSelection is not range and setSelectionIfPossible does that.

[Ryosuke Niwa]

I'm renaming setSelectionIfPossible to updateSelectionForMouseDownDispatchingSelectStart.

[Ryosuke Niwa]

Committed r87096: <http://trac.webkit.org/changeset/87096>

[Ryosuke Niwa]

Thanks for the review, Alexey!