WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r87096
: <
http://trac.webkit.org/changeset/87096
>
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.
Top of Page
Format For Printing
XML
Clone This Bug