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.
I presume this is why it's so hard to select sometimes!
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.
(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.
See also: bug 52986 (duplicate?).
(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.
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.
Created attachment 85561 [details] fixes the bug
(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.
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?
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.
*** Bug 31711 has been marked as a duplicate of this bug. ***
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?
(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.
(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.
Created attachment 85692 [details] Fixed per Tony's comment
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 . . .
(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.
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
(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.
Filed https://bugs.webkit.org/show_bug.cgi?id=56324.
Committed r81053: <http://trac.webkit.org/changeset/81053>
*** Bug 26788 has been marked as a duplicate of this bug. ***
*** Bug 52986 has been marked as a duplicate of this bug. ***