Bug 31711

Created attachment 43568 [details] Patch proposed to fix the problem This is the proposed solution, we had to modify the expected result of the fake drag because the previous solution did not considered the first moveTo for the selection which is the same problem. The patch sets m_beganSelectingText in the EventHandler::handleMousePressEventSingleClick, which means we will use that selection object as start, not the one created in the first movement. Could someone knowing the code better than me check the modification, maybe I'm missing a situation when we create that selection that we do not want to use it. I've run the editing tests and all of them have the same result, except the fake drag but it was wrong before because we do not get all the selected text from the click.

Created attachment 43569 [details] Added the Changelog comments

Adding people to the CC so they can comment about the bug.

Created attachment 43584 [details] Modified the test to avoid using the offsetWidth

I can't figure out from bug description what the problem is. When I click in the text and move the mouse, text gets selected, which looks like correct behavior. Could you please explain the issue in more detail?

(In reply to comment #5) > I can't figure out from bug description what the problem is. When I click in > the text and move the mouse, text gets selected, which looks like correct > behavior. Could you please explain the issue in more detail? Checking the code in the example you can see we are moving to a position, pressing the button, moving the mouse again and releasing the button: eventSender.mouseMoveTo(x, y); eventSender.mouseDown(); eventSender.mouseMoveTo(x + target.offsetWidth / 2, y); eventSender.mouseUp(); This code is not going to cause a selection because the button press position is discarded and a new selection is created with the target in the second moveTo: EventHandler.cpp:555 if (!m_beganSelectingText) { m_beganSelectingText = true; newSelection = VisibleSelection(targetPosition); } And in the next line we set the extent to the target again: EventHandler.cpp:561 newSelection.setExtent(targetPosition); If you want to select something we would have to use code like this: eventSender.mouseDown(); eventSender.mouseMoveTo(x, y); eventSender.mouseMoveTo(x + target.offsetWidth / 2, y); eventSender.mouseUp(); The first move sets the initial position of the selection and the second one the target because m_beganSelectingText is true. This basically means that if you click with the mouse and move fast enough to get a motion notify far from the press the selection will start there, not in the button press. It is easy to reproduce with DRT events but in the browser is difficult to reproduce because you get a lot of motion events but if you move the mouse fast enough when clicking and you print the press and motion positions, you could check you will be losing selection range. Hope this helps.

I'm as confused as I was before. Could you provide steps to reproduce this bug? Something like: 1. Open the first attachment in this bug. 2. Click inside the first "Select" word. 3. Without releasing mouse button, move mouse pointer to the right. 4. Release mouse button. Expected results: <...>. Actual results: <...>. For me, actual results for the above steps are that a selection is made.

(In reply to comment #7) > I'm as confused as I was before. Could you provide steps to reproduce this bug? > Something like: > 1. Open the first attachment in this bug. > 2. Click inside the first "Select" word. > 3. Without releasing mouse button, move mouse pointer to the right. > 4. Release mouse button. > Expected results: <...>. > Actual results: <...>. > The steps are these: 1. Execute the first attachment in this bug with the DRT 2. The expected result is selection (check the expected file in the last attachment), something like: selection start: position 0 of child 0 {#text} of child 1 {DIV} of child 1 {BODY} of child 0 {HTML} of document selection end: position 31 of child 0 {#text} of child 1 {DIV} of child 1 {BODY} of child 0 {HTML} of document 3. Actual result is a caret caret: ...

So, this has no user-visible effect in a browser at all?

(In reply to comment #9) > So, this has no user-visible effect in a browser at all? I did not check it in the browser before, because the problem comes from the fake drag test I was implementing in bug 28153. In theory we are losing the selection from the press to the first move event, but usually they are very close so you are still over the same character, and if they move fast users could think it was just their fault not clicking in the right position when moving too fast, it is unlikely it could be easily detected. Anyway, this is a log of one session with epiphany-webkit and slashdot.org, I was selecting moving the mouse fast. Check the m_offset of the VisiblePositions, the first one is newSelection.visibleStart() the second one is targetPosition. Initially the web was loading and apparently the gap between the first position and the second is a little bit bigger, after loading is more difficult to reproduce it. Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $152 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 20, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $153 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 36, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $154 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 17, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $155 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 18, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $156 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 20, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $157 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 21, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $158 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 17, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $159 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 17, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $160 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 11, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $161 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 12, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} Breakpoint 1, WebCore::EventHandler::updateSelectionForMouseDrag (this=0xb59778, targetNode=0x23e9a10, localPoint=...) at WebCore/page/EventHandler.cpp:557 557 newSelection = VisibleSelection(targetPosition); $162 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 12, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM} $163 = {m_deepPosition = {m_anchorNode = {<WTF::FastAllocBase> = {<No data fields>}, m_ptr = 0x23e9a10}, m_offset = 12, m_anchorType = 0, m_isLegacyEditingPosition = true}, m_affinity = WebCore::DOWNSTREAM}

style-queue successfully ran check-webkit-style on attachment 43584 [details] without any errors

Comment on attachment 43584 [details] Modified the test to avoid using the offsetWidth The ChangeLog in this patch has tabs in it. Won't this change behavior when you click without dragging?

> The ChangeLog in this patch has tabs in it. style-queue false negative filed: https://bugs.webkit.org/show_bug.cgi?id=32039

(In reply to comment #12) > (From update of attachment 43584 [details]) > The ChangeLog in this patch has tabs in it. > Oops, thanks for the review I'll fix it. > Won't this change behavior when you click without dragging? Not completely sure, that is why I would like someone with more experience in the handle_event code could review it. My analysis is this: m_beganSelectingText it is assigned in various places but it is used just in two places in the code, the first one is the one we want to modify the behaviour: EventHandler.cpp:555 if (!m_beganSelectingText) { m_beganSelectingText = true; newSelection = VisibleSelection(targetPosition); } And the second one is in the release handler: EventHandler.cpp:611 if (m_mouseDownWasSingleClickInSelection && !m_beganSelectingText #if ENABLE(DRAG_SUPPORT) && m_dragStartPos == event.event().pos() #endif && m_frame->selection()->isRange() && event.event().button() != RightButton) { This case is caused when m_mouseDownWasSingleClickInSelection is true which just can happen if the code where we have added the assignation of m_beganSelectingText was not executed due to the return false after the assignation: EventHandler.cpp:320 if (!extendSelection && m_frame->selection()->contains(vPoint)) { m_mouseDownWasSingleClickInSelection = true; return false; } We added the assignation in the same method after these lines. I hope it clarifies the rationale. I'll upload the new patch with Changelog fixed. Thanks again for the review :).

Created attachment 44131 [details] New patch proposal Fixed ChangeLog style

Patches require both an r+ and a cq+ if you want them committed by the bot. So you'll need to mark this r? as well.

style-queue ran check-webkit-style on attachment 44131 [details] without any errors.

Comment on attachment 44131 [details] New patch proposal This will cause other platforms to fail since it only has gtk results. Does this really need render tree results, or can this be made into a dumpAsText test?

Comment on attachment 44131 [details] New patch proposal Looks like this patch was abandoned. Please answer my question and reflag/repost as necessary.

(In reply to comment #18) > (From update of attachment 44131 [details]) > This will cause other platforms to fail since it only has gtk results. > > Does this really need render tree results, or can this be made into a > dumpAsText test? Sorry Eric, I forgot about this because we added the workaround in the test. All these tests that check if the selection we did is correct can be done with the dumpAsText checking the string selected instead of the positions but in the end we have the same problem because we depend on the font AFAIK. Anyway I think we can review the patch to avoid changing the boolean in that part of the code and just avoid overwriting the position if someone is interested in this modification.

Created attachment 69013 [details] A webpage test case that exposes the bug in an user-facing way. I just ran into this bug in WebKit/Safari and found this online. Seems like a problem with it is the lack of a good user-facing test so I made a webpage to show it. Basically it tries to run a long loop when the user tries to select text in a text box to make this behavior more apparent. Just try to drag and select text in the webpage and you should notice some part of the selection is missing.

This bug needs a new title. What does “fake drag” have to do with it at this point?

*** This bug has been marked as a duplicate of bug 56213 ***