Bug 19489

Created attachment 21630 [details] Screenshot

Created attachment 21640 [details] reduction

Created attachment 92919 [details] fixes the bug

Comment on attachment 92919 [details] fixes the bug Oops, this patch is missing some tests. Will upload a new patch today.

Created attachment 92973 [details] Added the forgotten tests

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 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.

(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.

(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.

Created attachment 93106 [details] fixed a test

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.

(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.

Could anyone review this patch?

Ping reviewers.

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 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.

I'm renaming setSelectionIfPossible to updateSelectionForMouseDownDispatchingSelectStart.

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

Thanks for the review, Alexey!