Bug 56213 – Selection uses first mousemove's localRect instead of that of mousedown

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Currently, even when m_mouseDownMayStartSelect = true, we use the coordinates given by the first mousemouse event for selection / dragging purposes.  So when a user rapidly moves mouse to select text, it could be few or more letters off.

[Simon Fraser (smfr)]

I presume this is why it's so hard to select sometimes!

[Evan Martin]

Created attachment 85509 [details]
layout test that uses dumpAsText and prints PASS/FAIL

This is a better test than the one I included in my mail to webkit-dev.

[Ryosuke Niwa]

(In reply to comment #1)
> I presume this is why it's so hard to select sometimes!

Yes!  I'm so glad that Evan has finally spotted this bug.  The fix should be fairly straight forward.

[Alexey Proskuryakov]

See also: bug 52986 (duplicate?).

[Ryosuke Niwa]

(In reply to comment #4)
> See also: bug 52986 (duplicate?).

Could be a complement. Whereas this bug addresses the issue with mousedown, the bug 52986 seems to address mouseup.  I think we need to be calling updateSelectionForMouseDrag in mouseup as well.

[Ryosuke Niwa]

It seems like dragstart event is using the correct coordinate:
(around L2690 of EventHandler.cpp)
        DragController* dragController = page ? page->dragController() : 0;
        bool startedDrag = dragController && dragController->startDrag(m_frame, dragState().m_dragClipboard.get(), srcOp, event.event(), m_mouseDownPos, dragState().m_dragSrcIsDHTML);

Notice we use m_mouseDownPos here.  We just need to do the same and call updateSelectionForMouseDrag when the selection starts with mousedown's coordinates.

[Ryosuke Niwa]

Created attachment 85561 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #7)
> Created an attachment (id=85561) [details]
> fixes the bug

One thing to consider here is what should happen if script / user modifies contents between the mouse down and the first mouse move.  In my current approach, the hit test is done in the first mouse move so any modifications done between mouse down and mouse move will be taken into account when we do the hit test.  This means that if scripts removes / appends nodes in keydown's event listener, we'll be doing the hit test against the new nodes.

[Evan Martin]

Comment on attachment 85561 [details]
fixes the bug

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

> Source/WebCore/page/EventHandler.cpp:551
> +        HitTestResult result(m_mouseDownPos);

Wow, we already had a variable for it?

> LayoutTests/editing/selection/drag-select-rapidly.html:4
> +<p>This test verifies that selection begins at the mouse-down position. Select rapidly inside the box blow. The first letter should be selected.</p>

Typo: "blow" -> "below".  I'm not sure what "select rapidly" means, is it really possible for someone to reproduce this by hand?

[Ryosuke Niwa]

Comment on attachment 85561 [details]
fixes the bug

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

>> Source/WebCore/page/EventHandler.cpp:551
>> +        HitTestResult result(m_mouseDownPos);
> 
> Wow, we already had a variable for it?

Yes.  We even use it for dragController, etc... This seems to be the only feature not using it.  We definitely need to do a lot of refactoring in this file :(

>> LayoutTests/editing/selection/drag-select-rapidly.html:4
>> +<p>This test verifies that selection begins at the mouse-down position. Select rapidly inside the box blow. The first letter should be selected.</p>
> 
> Typo: "blow" -> "below".  I'm not sure what "select rapidly" means, is it really possible for someone to reproduce this by hand?

Oops, will fix the typo before landing it.  As I wrote earlier, I have been annoyed by this bug so I'm sure you can reproduce it in some cases.  It's just that it's extremely hard to do.

[Alejandro G. Castro]

*** Bug 31711 has been marked as a duplicate of this bug. ***

[Tony Chang]

Comment on attachment 85561 [details]
fixes the bug

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

> Source/WebCore/page/EventHandler.cpp:550
> +        HitTestRequest request(HitTestRequest::Active);

Why is this Active while the dragController code uses ReadOnly?  Is it because you want the selection to update when the mouse exits the window?  Should it be Active | ReadOnly so we don't re-run RenderLayer::updateHoverActiveState?

> Source/WebCore/page/EventHandler.cpp:555
> +        updateSelectionForMouseDrag(result.innerNode(), result.localPoint());
> +    }
>      updateSelectionForMouseDrag(targetNode, event.localPoint());

Will this fire the selection change event twice?  It's probably fine if it does, but maybe we should codify that in a test?  What happens in other browsers?

[Ryosuke Niwa]

(In reply to comment #12)
> > Source/WebCore/page/EventHandler.cpp:555
> > +        updateSelectionForMouseDrag(result.innerNode(), result.localPoint());
> > +    }
> >      updateSelectionForMouseDrag(targetNode, event.localPoint());
> 
> Will this fire the selection change event twice?  It's probably fine if it does, but maybe we should codify that in a test?  What happens in other browsers?

I don't know how to test this behavior on other browsers.  I'll be extremely hard to emulate what eventSender does.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 85561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85561&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:550
> > +        HitTestRequest request(HitTestRequest::Active);
> 
> Why is this Active while the dragController code uses ReadOnly?  Is it because you want the selection to update when the mouse exits the window?  Should it be Active | ReadOnly so we don't re-run RenderLayer::updateHoverActiveState?

Oh, you're right.  I think I should be using ReadOnly here.

[Ryosuke Niwa]

Created attachment 85692 [details]
Fixed per Tony's comment

[Tony Chang]

Comment on attachment 85692 [details]
Fixed per Tony's comment

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

> LayoutTests/platform/mac/editing/selection/fake-drag-expected.txt:8
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 5 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

Ah, it does fire selection changed twice.  That seems harmless . . .

[Ryosuke Niwa]

(In reply to comment #16)
> (From update of attachment 85692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85692&action=review
> 
> > LayoutTests/platform/mac/editing/selection/fake-drag-expected.txt:8
> > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 5 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> 
> Ah, it does fire selection changed twice.  That seems harmless . . .

And it should fire twice since both mousedown and mousemove change selection.

[Darin Adler]

But why should the delegate get called at all if the selection is not changing? This doesn’t look like a change to me:

EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

[Ryosuke Niwa]

(In reply to comment #18)
> But why should the delegate get called at all if the selection is not changing? This doesn’t look like a change to me:
> 
> EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document toDOMRange:range from 4 of #text > DIV > BODY > HTML > #document to 4 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

That's because we don't check whether or not selection had changed in EventHandler::updateSelectionForMouseDrag.  We should fix that in a separate patch though because it isn't directly related to this bug.

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=56324.

[Ryosuke Niwa]

Committed r81053: <http://trac.webkit.org/changeset/81053>

[Ryosuke Niwa]

*** Bug 26788 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 52986 has been marked as a duplicate of this bug. ***