Bug 55552 – Text selection changes unexpectedly when dragging out of the <input>

• Component: Text
• Status: RESOLVED FIXED

[Peter Kasting]

Visit http://www.shacknews.com/search?type=4 .  Type anything into the "Search for:" box.  Now click down in the middle of your text, drag left to select the beginning, and keep dragging out of the <input> and into the page to the left.  Also try moving your mouse up and down at various x coordinates.

I expect the selection to be pretty stable, not unexpectedly reverse itself half the time.

I could swear I filed this already, but I can't find it.  In any case this happens on a lot of sites and drive me bananas.  Please tell me someone has time to look into it :(

[noel gordon]

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

[noel gordon]

Created attachment 85744 [details]
test case

[Alice Boxhall]

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.

[Alice Boxhall]

Created attachment 87828 [details]
Patch

[Alice Boxhall]

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.

[Peter Kasting]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Alice Boxhall]

Created attachment 88024 [details]
Patch

[Alice Boxhall]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ojan Vafai]

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

[Ojan Vafai]

Comment on attachment 88024 [details]
Patch

Removing this from the review queue per the above comment and till bug 57923 gets committed.

[Alice Boxhall]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Alice Boxhall]

Created attachment 88970 [details]
Patch

[Ryosuke Niwa]

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
> +
> [phone]  Alice Boxhall  <[e-mail]>
> +
> +        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".

[Peter Kasting]

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.

[Alice Boxhall]

Created attachment 89157 [details]
Patch

[Ryosuke Niwa]

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.

[Alice Boxhall]

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

[Alice Boxhall]

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

[Ryosuke Niwa]

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.

[Ojan Vafai]

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

[Ryosuke Niwa]

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

[Alice Boxhall]

Created attachment 89322 [details]
Patch

[Ryosuke Niwa]

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.

[Alice Boxhall]

Created attachment 89526 [details]
Patch

[Alice Boxhall]

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

[Ryosuke Niwa]

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.

[Alice Boxhall]

Created attachment 89527 [details]
Patch

[Alice Boxhall]

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

[Ryosuke Niwa]

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.

[Alice Boxhall]

Created attachment 89672 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 89672 [details]
Patch

Clearing flags on attachment: 89672

Committed r83967: <http://trac.webkit.org/changeset/83967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrey Kosyakov]

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?

[Ojan Vafai]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Alice Boxhall]

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