Bug 55552

test case: click in the middle and drag out to the right to see the unexpected selection change.

Created attachment 85744 [details] test case

I've been digging around this a little, and it seems like the problem might be related to the fact that the contentHeight and contentWidth of the input field are (incorrectly?) set to 0 and 1008, respectively. This causes RenderBox::positionForPoint to incorrectly calculate the relative position of the mouse cursor once the mouse drags out of the input field.

Created attachment 87828 [details] Patch

First pass patch; feedback welcome. After a few blind alleys it turned out to be simplest to just modify updateSelectionForMouseDrag to translate the selection end-point into a local point within the editable element, in the case where the mouse had dragged out of it, since the selection can't extend beyond the editable element anyway.

I think this is bug 12593.

I think this is bug 12593.

(In reply to comment #6) > I think this is bug 12593. Could be. The description there is a little vague and it lacks the attachments from the Radar bug. If they're the same, maybe that bug should be duped against this since there's a patch here? I dunno.

Comment on attachment 87828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87828&action=review > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:17 > +} else This else clause needs to have curly brackets. > Source/WebCore/page/EventHandler.cpp:640 > + IntPoint realLocalPoint(localPoint); I don't think realLocalPoint is a descriptive name. What does "real" mean? Also, I think we normally use assignment operator rather than copy constructor when declaring a local variable. So the preferred way of writing this will be: IntPoint realLocalPoint = localPoint. > Source/WebCore/page/EventHandler.cpp:654 > + if (editableElement && !editableElement->contains(targetNode)) { > + targetNode = editableElement; > > - // Don't modify the selection if we're not on a node. > - if (targetPosition.isNull()) > - return; > + FloatPoint absolutePoint = targetRenderer->localToAbsolute(FloatPoint(realLocalPoint)); > + > + targetRenderer = targetNode->renderer(); > + if (!targetRenderer) > + return; > > + realLocalPoint = roundedIntPoint(targetRenderer->absoluteToLocal(absolutePoint)); > + } It's odd that we fix localPoint in updateSelectionForMouseDrag rather than in its call site. We should fix it in handleMouseDraggedEvent instead. > Source/WebCore/page/EventHandler.cpp:658 > + VisiblePosition targetPosition(targetRenderer->positionForPoint(realLocalPoint)); Ditto about copy constructor.

Comment on attachment 87828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87828&action=review r- because you're missing LayoutTests/ChangeLog > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:14 > + eventSender.mouseDown(); > + x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5; > + eventSender.mouseMoveTo(x, y); > + eventSender.mouseUp(); I believe you need to add leapForward between mouseDown and mouseUp or otherwise this test becomes flaky. > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:27 > + return el.isContentEditable ? window.getSelection().baseOffset : el.selectionStart; Please do not use arbitrary abbreviations like "el". > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:15 > +To test manually, drag from the middle of the editable div to the > +right, into the non-floated text. The selection should go to the end > +of the input element and not jump to the beginning. Are you trying to fit everything in 80 columns? No need to do that here. > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:19 > +<p id="result"> > +</p> > +<pre id="console"></pre> Can't we have just one pre/p? It seems silly to have two separate elements for automated / manual test. > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:20 > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> We don't normally specify type attribute. > LayoutTests/editing/selection/select-out-of-floated-input.html:22 > +<p id="result"> > +</p> > +<pre id="console"></pre> > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> Ditto. > LayoutTests/editing/selection/select-out-of-floated-textarea.html:22 > +<p id="result"> > +</p> > +<pre id="console"></pre> > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> Ditto.

Created attachment 88024 [details] Patch

(In reply to comment #9) > (From update of attachment 87828 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87828&action=review > > > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:17 > > +} else > > This else clause needs to have curly brackets. Done. > > Source/WebCore/page/EventHandler.cpp:640 > > + IntPoint realLocalPoint(localPoint); > > I don't think realLocalPoint is a descriptive name. What does "real" mean? Also, I think we normally use assignment operator rather than copy constructor when declaring a local variable. So the preferred way of writing this will be: > IntPoint realLocalPoint = localPoint. Renamed to finalPoint, and used assignment. > > Source/WebCore/page/EventHandler.cpp:654 > > + if (editableElement && !editableElement->contains(targetNode)) { > > + targetNode = editableElement; > > > > - // Don't modify the selection if we're not on a node. > > - if (targetPosition.isNull()) > > - return; > > + FloatPoint absolutePoint = targetRenderer->localToAbsolute(FloatPoint(realLocalPoint)); > > + > > + targetRenderer = targetNode->renderer(); > > + if (!targetRenderer) > > + return; > > > > + realLocalPoint = roundedIntPoint(targetRenderer->absoluteToLocal(absolutePoint)); > > + } > > It's odd that we fix localPoint in updateSelectionForMouseDrag rather than in its call site. We should fix it in handleMouseDraggedEvent instead. I don't understand why... the point only needs to be re-interpreted in the case where a selection is being updated and the mouse has been dragged outside of the editable element in which it began. To me it seems like a good fit to do this in the updateSelectionForMouseDrag function. Also, handleMouseDraggedEvent has two branches which call updateSelectionForMouseDrag, with two different points as arguments, so I'd need to modify both of them. > > Source/WebCore/page/EventHandler.cpp:658 > > + VisiblePosition targetPosition(targetRenderer->positionForPoint(realLocalPoint)); > > Ditto about copy constructor. Done. (In reply to comment #10) > (From update of attachment 87828 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87828&action=review > > r- because you're missing LayoutTests/ChangeLog > > > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:14 > > + eventSender.mouseDown(); > > + x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5; > > + eventSender.mouseMoveTo(x, y); > > + eventSender.mouseUp(); > > I believe you need to add leapForward between mouseDown and mouseUp or otherwise this test becomes flaky. Why is that? I haven't observed any flaky behaviour in running it. > > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:27 > > + return el.isContentEditable ? window.getSelection().baseOffset : el.selectionStart; > > Please do not use arbitrary abbreviations like "el". Ok. > > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:15 > > +To test manually, drag from the middle of the editable div to the > > +right, into the non-floated text. The selection should go to the end > > +of the input element and not jump to the beginning. > > Are you trying to fit everything in 80 columns? No need to do that here. I find it easier to read wrapped text than text that flows off the page; is there a preferable width to wrap it to? > > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:19 > > +<p id="result"> > > +</p> > > +<pre id="console"></pre> > > Can't we have just one pre/p? It seems silly to have two separate elements for automated / manual test. I think this simplifies separating out the success/fail handling and the logging. The result div is used in the manual test as well (although obviously it's not 100% reliable if you don't follow the written instructions). > > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:20 > > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> > > We don't normally specify type attribute. Ok. > > LayoutTests/editing/selection/select-out-of-floated-input.html:22 > > +<p id="result"> > > +</p> > > +<pre id="console"></pre> > > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> > > Ditto. Done. > > LayoutTests/editing/selection/select-out-of-floated-textarea.html:22 > > +<p id="result"> > > +</p> > > +<pre id="console"></pre> > > +<script type="text/javascript" src="resources/select-out-of-floated-editable.js"></script> > > Ditto. Done.

(In reply to comment #12) > > > Source/WebCore/page/EventHandler.cpp:654 > > It's odd that we fix localPoint in updateSelectionForMouseDrag rather than in its call site. We should fix it in handleMouseDraggedEvent instead. > > I don't understand why... the point only needs to be re-interpreted in the case where a selection is being updated and the mouse has been dragged outside of the editable element in which it began. To me it seems like a good fit to do this in the updateSelectionForMouseDrag function. Also, handleMouseDraggedEvent has two branches which call updateSelectionForMouseDrag, with two different points as arguments, so I'd need to modify both of them. The problem is that updateSelectionForMouseDrag explicitly takes a node and localPoint. It's a really bad API to let a function take a node and a local point and then modify the node & local point inside the function. So much of editing code has been hacked around like this and it really reduces the readability and maintainability of code because when I see the calls to updateSelectionForMouseDrag, I'd assume that selection is updated with respect to the node and local rect I passed in. On my second thought, I think the best way to resolve this is to change the signature of updateSelectionForMouseDrag to MouseEventWithHitTestResults(HitTestResult). That way, we're not explicitly passing a node & localRect so it doesn't look magical. > > > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:14 > > > + eventSender.mouseDown(); > > > + x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5; > > > + eventSender.mouseMoveTo(x, y); > > > + eventSender.mouseUp(); > > > > I believe you need to add leapForward between mouseDown and mouseUp or otherwise this test becomes flaky. > > Why is that? I haven't observed any flaky behaviour in running it. You may not observe flakiness locally but some bots might. Sadly, many existing tests do this and are flaky on some bots :( > > > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:15 > > > +To test manually, drag from the middle of the editable div to the > > > +right, into the non-floated text. The selection should go to the end > > > +of the input element and not jump to the beginning. > > > > Are you trying to fit everything in 80 columns? No need to do that here. > > I find it easier to read wrapped text than text that flows off the page; is there a preferable width to wrap it to? Historically, I don't think we ever wrap text inside HTML files (except maybe at the end of sentence). > > > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:19 > > > +<p id="result"> > > > +</p> > > > +<pre id="console"></pre> > > > > Can't we have just one pre/p? It seems silly to have two separate elements for automated / manual test. > > I think this simplifies separating out the success/fail handling and the logging. The result div is used in the manual test as well (although obviously it's not 100% reliable if you don't follow the written instructions). Huh... it seems odd to have these two elements when only either one is used. Can we auto-generate them instead then? By the way, this bug might be a duplicate of the bug 52986.

Comment on attachment 88024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88024&action=review > Source/WebCore/page/EventHandler.cpp:655 > - VisiblePosition targetPosition(targetRenderer->positionForPoint(localPoint)); > + IntPoint finalPoint = localPoint; > + > + VisibleSelection newSelection = m_frame->selection()->selection(); > + Element* editableElement = newSelection.rootEditableElement(); > + if (editableElement && !editableElement->contains(targetNode)) { > + targetNode = editableElement; > > - // Don't modify the selection if we're not on a node. > - if (targetPosition.isNull()) > - return; > + FloatPoint absolutePoint = targetRenderer->localToAbsolute(FloatPoint(finalPoint)); > + > + targetRenderer = targetNode->renderer(); > + if (!targetRenderer) > + return; > > + finalPoint = roundedIntPoint(targetRenderer->absoluteToLocal(absolutePoint)); > + } > + Please extract this code as a function.

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

Created attachment 88192 [details] test for the bug 52986 Hi Alice, Here's DRT test for the bug 52986. Please add it to your patch.

When I was writing this test, I realized that the bug 52986 does not reproduce when there's non-editable content above or below the editable div. I'd greatly appreciate it if you could figure out why that's the case.

(In reply to comment #17) > When I was writing this test, I realized that the bug 52986 does not reproduce when there's non-editable content above or below the editable div. I'd greatly appreciate it if you could figure out why that's the case. This bug doesn't look like a duplicate to me. Although, it's not clear to me what's causing bug 52986. Have you confirmed it's fixed by Alice's patch? We shouldn't dupe this until we're sure it's the same root cause.

Comment on attachment 88024 [details] Patch Removing this from the review queue per the above comment and till bug 57923 gets committed.

(In reply to comment #19) > (From update of attachment 88024 [details]) > Removing this from the review queue per the above comment and till bug 57923 gets committed. I'm not sure that it's a duplicate, but both Ryosuke and Noel have verified that it's fixed by this change.

(In reply to comment #18) > (In reply to comment #17) > > When I was writing this test, I realized that the bug 52986 does not reproduce when there's non-editable content above or below the editable div. I'd greatly appreciate it if you could figure out why that's the case. > > This bug doesn't look like a duplicate to me. Although, it's not clear to me what's causing bug 52986. Have you confirmed it's fixed by Alice's patch? We shouldn't dupe this until we're sure it's the same root cause. It is a duplicate. We're not passing a node inside editable region to the SelectionController. The reason we can't reproduce this bug when there are content above or blow the editable region is probably due to the canonicalization.

(In reply to comment #21) > It is a duplicate. We're not passing a node inside editable region to the SelectionController. The reason we can't reproduce this bug when there are content above or blow the editable region is probably due to the canonicalization. Or rather how we fix up positions in SelectionController when the root editable element doesn't match.

Created attachment 88970 [details] Patch

Comment on attachment 88970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88970&action=review Okay, I like the patch in general but there are few nits I want you to address before landing this patch. I'd say r- for now given that you're not a committer yet. > LayoutTests/ChangeLog:38 > + > +2011-04-03 Alice Boxhall <aboxhall@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Text selection changes unexpectedly when dragging out of the &lt;input&gt; > + https://bugs.webkit.org/show_bug.cgi?id=55552 > + > + Tests that dragging outside of a contenteditable, input or textarea selects to the end of the > + element, rather than jumping back to the beginning. > + > + * editing/selection/resources/select-out-of-floated-editable.js: Added. > + * editing/selection/select-out-of-floated-contenteditable-expected.txt: Added. > + * editing/selection/select-out-of-floated-contenteditable.html: Added. > + * editing/selection/select-out-of-floated-input-expected.txt: Added. > + * editing/selection/select-out-of-floated-input.html: Added. > + * editing/selection/select-out-of-floated-textarea-expected.txt: Added. > + * editing/selection/select-out-of-floated-textarea.html: Added. > + You have two change log entires. Please fix. > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:12 > + x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5; You need to call leap forward here. > LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:37 > + : floatedEditable.value; I don't think we normally align after tertiary operator like this. Please do: var inputText = floatedEditable.isContentEditable ? floatedEditable.textContent : floatedEditable.value; > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:5 > +<style> > + div { float: left; } > + p { padding-top: 1em; } > +</style> I don't know why we need to specify these as style rules. p already has some margins and you can specify float: left in the inline style declaration. > LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:19 > +<p id="result"> > +</p> > +<pre id="console"></pre> Nit: It seems odd that you have <p> and </p> on a separate line even though it's empty and right beneath it, you have <pre></pre>. > Source/WebCore/ChangeLog:15 > + (WebCore::targetPositionForSelectionEndpoint): When dragging from an editable element, check that > + the endpoint is not outside the element. If it is, translate the point into a local point within the > + editable element. Nit: I don't think we intend in change log like this. Please align the start of each line to (. Also, it's odd that line break exists between an article and the noun that immediately follows it. We usually truncate a line at more natural places between "within" and "the" or between "local point" and "within" > Source/WebCore/page/EventHandler.cpp:634 > + // If selecting out of an editable element, translate the endpoint into a local point within the > + // editable element, to avoid confusing behaviour which can occur when dragging into a node which is > + // earlier in the DOM. Nit: this comment is too verbose. We can either omit the comment entirely or simply state that we're respecting editing boundary. We certainly don't need to say that we're translating the endpoint to a local point within the editable element because that's self-evident from the code. "avoid confusing behavior when dragging out of an editable element" will be better but we need to be a little more specific than "confusing".

Comment on attachment 88970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88970&action=review >> LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:37 >> + : floatedEditable.value; > > I don't think we normally align after tertiary operator like this. Please do: > var inputText = floatedEditable.isContentEditable ? floatedEditable.textContent > : floatedEditable.value; I have actually seen this style. However, I think breaking after the '?', or not breaking at all, might be best.

Created attachment 89157 [details] Patch

Comment on attachment 89157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89157&action=review > Source/WebCore/page/EventHandler.cpp:633 > + // Avoid selection changing direction when dragging out of an editable element into > + // an element which comes earlier in the DOM. Mn... this is the description of the bug you're solving but I don't think that'll necessarily address the all cases where we reach this code path. i.e. I think you're solving more general problem here than what comment explains and that's kind of misleading.

(In reply to comment #27) > (From update of attachment 89157 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89157&action=review > > > Source/WebCore/page/EventHandler.cpp:633 > > + // Avoid selection changing direction when dragging out of an editable element into > > + // an element which comes earlier in the DOM. > > Mn... this is the description of the bug you're solving but I don't think that'll necessarily address the all cases where we reach this code path. i.e. I think you're solving more general problem here than what comment explains and that's kind of misleading. True, although I don't think it suffices to say "respect editing boundaries" either, since that happens elsewhere. Will have a think -- happy to take the comment off altogether, but I think it's worth clarifying why this is necessary here.

(In reply to comment #28) > (In reply to comment #27) > > (From update of attachment 89157 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89157&action=review > > > > > Source/WebCore/page/EventHandler.cpp:633 > > > + // Avoid selection changing direction when dragging out of an editable element into > > > + // an element which comes earlier in the DOM. > > > > Mn... this is the description of the bug you're solving but I don't think that'll necessarily address the all cases where we reach this code path. i.e. I think you're solving more general problem here than what comment explains and that's kind of misleading. > > True, although I don't think it suffices to say "respect editing boundaries" either, since that happens elsewhere. Will have a think -- happy to take the comment off altogether, but I think it's worth clarifying why this is necessary here. How about: // When dragging out of an editable element, the selection cannot cross the editing boundary, // so it doesn't make sense to calculate a selection endpoint outside the editable element.

Comment on attachment 89157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89157&action=review >>>> Source/WebCore/page/EventHandler.cpp:633 >>>> + // an element which comes earlier in the DOM. >>> >>> Mn... this is the description of the bug you're solving but I don't think that'll necessarily address the all cases where we reach this code path. i.e. I think you're solving more general problem here than what comment explains and that's kind of misleading. >> >> True, although I don't think it suffices to say "respect editing boundaries" either, since that happens elsewhere. Will have a think -- happy to take the comment off altogether, but I think it's worth clarifying why this is necessary here. > > How about: > // When dragging out of an editable element, the selection cannot cross the editing boundary, > // so it doesn't make sense to calculate a selection endpoint outside the editable element. okay, that explains why we don't use the old end point but doesn't explain why we use editable root as new end point. I'd say something along the line of "use the root editable element as the end point when dragging out of the element because selection cannot cross the editing boundary." To be more explicitly, I say something like working around issues in adjustSelectionToAvoidCrossingEditingBoundaries but then you need to start talking about how it affects selection, etc... and it'll be too verbose again.

(In reply to comment #30) > To be more explicitly, I say something like working around issues in adjustSelectionToAvoidCrossingEditingBoundaries but then you need to start talking about how it affects selection, etc... and it'll be too verbose again. The bug this patch was originally for isn't about adjustSelectionToAvoidCrossingEditingBoundaries. It's that there are cases where we set the selection at the start of the container node when there are floats involved instead of at the end. You get similarly weird selections if you ignore editing, but in the absence of editing, there's nothing better we can do (and in fact we match Firefox). Alice's last comments seems fine to me.

(In reply to comment #31) > The bug this patch was originally for isn't about adjustSelectionToAvoidCrossingEditingBoundaries. It's that there are cases where we set the selection at the start of the container node when there are floats involved instead of at the end. You get similarly weird selections if you ignore editing, but in the absence of editing, there's nothing better we can do (and in fact we match Firefox). Sure, but I don't think it's important what this bug or patch was for because the comment is added to the code and not to the change log entry. Having said that, I'm fine with Alice's last comment as well (in the sense that I don't mind r+ing it) although I still feel like we can improve it more.

Created attachment 89322 [details] Patch

Comment on attachment 89322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89322&action=review > Source/WebCore/page/EventHandler.cpp:638 > + FloatPoint absolutePoint = targetNode->renderer()->localToAbsolute(FloatPoint(selectionEndPoint)); > + > + selectionEndPoint = roundedIntPoint(selectionEndNode->renderer()->absoluteToLocal(absolutePoint)); > + } > + > + return selectionEndNode->renderer()->positionForPoint(selectionEndPoint); Oops, these nodes may not have renderers.

Created attachment 89526 [details] Patch

(In reply to comment #34) > (From update of attachment 89322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89322&action=review > > > Source/WebCore/page/EventHandler.cpp:638 > > + FloatPoint absolutePoint = targetNode->renderer()->localToAbsolute(FloatPoint(selectionEndPoint)); > > + > > + selectionEndPoint = roundedIntPoint(selectionEndNode->renderer()->absoluteToLocal(absolutePoint)); > > + } > > + > > + return selectionEndNode->renderer()->positionForPoint(selectionEndPoint); > > Oops, these nodes may not have renderers. Fixed. (In reply to comment #32) > (In reply to comment #31) > > The bug this patch was originally for isn't about adjustSelectionToAvoidCrossingEditingBoundaries. It's that there are cases where we set the selection at the start of the container node when there are floats involved instead of at the end. You get similarly weird selections if you ignore editing, but in the absence of editing, there's nothing better we can do (and in fact we match Firefox). > > Sure, but I don't think it's important what this bug or patch was for because the comment is added to the code and not to the change log entry. Having said that, I'm fine with Alice's last comment as well (in the sense that I don't mind r+ing it) although I still feel like we can improve it more. Comment removed.

Comment on attachment 89526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89526&action=review > Source/WebCore/page/EventHandler.cpp:641 > + return selectionEndNode->renderer()->positionForPoint(selectionEndPoint); You need to check renderer here as well. > Source/WebCore/page/EventHandler.cpp:658 > if (!targetRenderer) > return; You should remove this early exit.

Created attachment 89527 [details] Patch

(In reply to comment #37) > (From update of attachment 89526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89526&action=review > > > Source/WebCore/page/EventHandler.cpp:641 > > + return selectionEndNode->renderer()->positionForPoint(selectionEndPoint); > > You need to check renderer here as well. Done. > > Source/WebCore/page/EventHandler.cpp:658 > > if (!targetRenderer) > > return; > > You should remove this early exit. Done (and removed that local variable too, as it wasn't used anywhere).

Comment on attachment 89527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89527&action=review > Source/WebCore/page/EventHandler.cpp:660 > + VisibleSelection newSelection = m_frame->selection()->selection(); > + VisiblePosition targetPosition = selectionExtentRespectingEditingBoundary(newSelection, hitTestResult.localPoint(), target); You can just pass m_frame->selection()->selection() here since you're not modifying newSelection. That'll avoid copying VisibleSelection needlessly when we're bailing out early. Please make this change and I'll cq+ it.

Created attachment 89672 [details] Patch

Comment on attachment 89672 [details] Patch Clearing flags on attachment: 89672 Committed r83967: <http://trac.webkit.org/changeset/83967>

All reviewed patches have been landed. Closing bug.

This caused somewhat strange change of chromium expectations to certain other selection tests: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&showExpectations=true&group=%40ToT%20-%20chromium.org&tests=editing%2Fselection%2Fselect-from-textfield-outwards.html%2Cfast%2Fforms%2Finput-text-drag-down.html Does anyone have an idea whether this is safe to rebaseline?

(In reply to comment #44) > This caused somewhat strange change of chromium expectations to certain other selection tests: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&showExpectations=true&group=%40ToT%20-%20chromium.org&tests=editing%2Fselection%2Fselect-from-textfield-outwards.html%2Cfast%2Fforms%2Finput-text-drag-down.html > > Does anyone have an idea whether this is safe to rebaseline? From a quick look at the failures and what the tests are doing, I can't tell if this is fixing a bug or causing one. :) Alice, mind taking a closer look?

Also see https://bugs.webkit.org/show_bug.cgi?id=55552. Looking at the chromium result, the test has regressed. But I'm now sure if it's a bug in WebKit or a bug in the tests. Will take a look later.

I meant to say see https://bugs.webkit.org/show_bug.cgi?id=58664.

(In reply to comment #46) > Also see https://bugs.webkit.org/show_bug.cgi?id=55552. > > Looking at the chromium result, the test has regressed. But I'm now sure if it's a bug in WebKit or a bug in the tests. Will take a look later. I'm without a workstation till Monday, as our team is moving desks over the weekend. Will take a look on Monday when things are up and running again.