RESOLVED FIXED 19489
selectstart is fired for every mouse move
https://bugs.webkit.org/show_bug.cgi?id=19489
Summary selectstart is fired for every mouse move
Anantha Keesara
Reported 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
Attachments
Screenshot (47.52 KB, image/png)
2008-06-11 12:34 PDT, Anantha Keesara
no flags
reduction (1.07 KB, text/html)
2008-06-11 13:12 PDT, Anantha Keesara
no flags
fixes the bug (19.19 KB, patch)
2011-05-09 22:20 PDT, Ryosuke Niwa
no flags
Added the forgotten tests (25.51 KB, patch)
2011-05-10 10:25 PDT, Ryosuke Niwa
no flags
fixed a test (25.39 KB, patch)
2011-05-11 04:54 PDT, Ryosuke Niwa
ap: review+
Anantha Keesara
Comment 1 2008-06-11 12:34:07 PDT
Created attachment 21630 [details] Screenshot
Anantha Keesara
Comment 2 2008-06-11 13:12:09 PDT
Created attachment 21640 [details] reduction
Ryosuke Niwa
Comment 3 2011-05-09 22:20:26 PDT
Created attachment 92919 [details] fixes the bug
Ryosuke Niwa
Comment 4 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.
Ryosuke Niwa
Comment 5 2011-05-10 10:25:18 PDT
Created attachment 92973 [details] Added the forgotten tests
Hajime Morrita
Comment 6 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.
Ryosuke Niwa
Comment 7 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.
Ryosuke Niwa
Comment 8 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.
Hajime Morrita
Comment 9 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.
Ryosuke Niwa
Comment 10 2011-05-11 04:54:25 PDT
Created attachment 93106 [details] fixed a test
Ryosuke Niwa
Comment 11 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.
Ryosuke Niwa
Comment 12 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.
Ryosuke Niwa
Comment 13 2011-05-16 13:27:36 PDT
Could anyone review this patch?
Ryosuke Niwa
Comment 14 2011-05-20 11:26:42 PDT
Ping reviewers.
Alexey Proskuryakov
Comment 15 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.
Ryosuke Niwa
Comment 16 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.
Ryosuke Niwa
Comment 17 2011-05-23 13:40:49 PDT
I'm renaming setSelectionIfPossible to updateSelectionForMouseDownDispatchingSelectStart.
Ryosuke Niwa
Comment 18 2011-05-23 13:49:43 PDT
Ryosuke Niwa
Comment 19 2011-05-23 13:53:48 PDT
Thanks for the review, Alexey!
Note You need to log in before you can comment on or make changes to this bug.