Bug 19489 - selectstart is fired for every mouse move
Summary: selectstart is fired for every mouse move
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Event Handling (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 60424 61319 61320 61322
  Show dependency treegraph
 
Reported: 2008-06-11 12:34 PDT by Anantha Keesara
Modified: 2011-05-23 16:37 PDT (History)
11 users (show)

See Also:


Attachments
Screenshot (47.52 KB, image/png)
2008-06-11 12:34 PDT, Anantha Keesara
no flags Details
reduction (1.07 KB, text/html)
2008-06-11 13:12 PDT, Anantha Keesara
no flags Details
fixes the bug (19.19 KB, patch)
2011-05-09 22:20 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added the forgotten tests (25.51 KB, patch)
2011-05-10 10:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed a test (25.39 KB, patch)
2011-05-11 04:54 PDT, Ryosuke Niwa
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anantha Keesara 2008-06-11 12:34:05 PDT
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
Comment 1 Anantha Keesara 2008-06-11 12:34:07 PDT
Created attachment 21630 [details]
Screenshot
Comment 2 Anantha Keesara 2008-06-11 13:12:09 PDT
Created attachment 21640 [details]
reduction
Comment 3 Ryosuke Niwa 2011-05-09 22:20:26 PDT
Created attachment 92919 [details]
fixes the bug
Comment 4 Ryosuke Niwa 2011-05-10 08:43:46 PDT
Comment on attachment 92919 [details]
fixes the bug

Oops, this patch is missing some tests.  Will upload a new patch today.
Comment 5 Ryosuke Niwa 2011-05-10 10:25:18 PDT
Created attachment 92973 [details]
Added the forgotten tests
Comment 6 Hajime Morrita 2011-05-11 01:11:21 PDT
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.
Comment 7 Ryosuke Niwa 2011-05-11 01:40:59 PDT
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.
Comment 8 Ryosuke Niwa 2011-05-11 01:42:44 PDT
(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.
Comment 9 Hajime Morrita 2011-05-11 02:00:07 PDT
(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.
Comment 10 Ryosuke Niwa 2011-05-11 04:54:25 PDT
Created attachment 93106 [details]
fixed a test
Comment 11 Ryosuke Niwa 2011-05-13 17:22:36 PDT
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.
Comment 12 Ryosuke Niwa 2011-05-13 17:23:06 PDT
(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.
Comment 13 Ryosuke Niwa 2011-05-16 13:27:36 PDT
Could anyone review this patch?
Comment 14 Ryosuke Niwa 2011-05-20 11:26:42 PDT
Ping reviewers.
Comment 15 Alexey Proskuryakov 2011-05-23 11:00:05 PDT
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.
Comment 16 Ryosuke Niwa 2011-05-23 13:10:14 PDT
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.
Comment 17 Ryosuke Niwa 2011-05-23 13:40:49 PDT
I'm renaming setSelectionIfPossible to updateSelectionForMouseDownDispatchingSelectStart.
Comment 18 Ryosuke Niwa 2011-05-23 13:49:43 PDT
Committed r87096: <http://trac.webkit.org/changeset/87096>
Comment 19 Ryosuke Niwa 2011-05-23 13:53:48 PDT
Thanks for the review, Alexey!