Bug 92513 – [chromium] Upstream Android changes to WebFrameImpl::selectRange

• Component: WebKit API
• Status: RESOLVED FIXED

[Iain Merrick]

Android-specific changes made to WebFrameImpl methods selectRange and setCaretSelectionAtWindowPoint need to be upstreamed back into WebKit.

See related Chromium bug http://code.google.com/p/chromium/issues/detail?id=138939

[Iain Merrick]

Created attachment 154971 [details]
Patch

[WebKit Review Bot]

Attachment 154971 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Iain Merrick]

Created attachment 155229 [details]
Patch

[Hans Wennborg]

Comment on attachment 155229 [details]
Patch

Just a couple of drive-by style nits; I can't comment on the actual functionality here.


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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463
> +    // Ensure startPosition is before endPosition

nit: finish sentence with period; applies for comments below as well.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1467
> +        endPosition = tmp;

maybe just std::swap(startPosition, endPosition)

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1499
> +    WebCore::EUserTriggered userTriggered = static_cast<WebCore::EUserTriggered>(true);

the cast looks funny
how about "const WebCore::EUserTriggered userTriggered = WebCore::UserTriggered;" ?
or maybe just passing in WebCore::UserTriggered in the function calls where it is used below?

[Iain Merrick]

Comment on attachment 155229 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463
>> +    // Ensure startPosition is before endPosition
> 
> nit: finish sentence with period; applies for comments below as well.

Done

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1467
>> +        endPosition = tmp;
> 
> maybe just std::swap(startPosition, endPosition)

Good call, done.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1499
>> +    WebCore::EUserTriggered userTriggered = static_cast<WebCore::EUserTriggered>(true);
> 
> the cast looks funny
> how about "const WebCore::EUserTriggered userTriggered = WebCore::UserTriggered;" ?
> or maybe just passing in WebCore::UserTriggered in the function calls where it is used below?

Huh, you're right, that is a bit odd. It does seem correct for it always to be true, as this method is specifically intended to convert touch coordinates into a selection range.

I'll just pass WebCore::UserTriggered directly.

[Iain Merrick]

Created attachment 155518 [details]
Patch

[Peter Beverloo]

Please make sure your patches block the meta bug: bug 66687 :). Thanks!

[Peter Beverloo]

Comment on attachment 155518 [details]
Patch

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

Could we write layout or unit tests to verify correct behavior of this code? It's a lot of logic on the WebKit API side..
Do you have an example/explanation of what is broken right now? At least on desktop, selections in content editable elements seems to be clamped to the editable root already.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1501
> +            // New selection is outside input box - move cursor towards selection point.

s/input box/editable element?

> Source/WebKit/chromium/src/WebFrameImpl.h:287
> +    void setCaretSelectionAtWindowPoint(const WebPoint&);

Is this method called from outside WebFrameImpl? Otherwise it should be private.

[Peter Beverloo]

+rniwa, as this is about selection/editing.

[Iain Merrick]

Comment on attachment 155518 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1501
>> +            // New selection is outside input box - move cursor towards selection point.
> 
> s/input box/editable element?

Done

>> Source/WebKit/chromium/src/WebFrameImpl.h:287
>> +    void setCaretSelectionAtWindowPoint(const WebPoint&);
> 
> Is this method called from outside WebFrameImpl? Otherwise it should be private.

Nope, it isn't. Done.

[Iain Merrick]

Created attachment 155547 [details]
Patch

[Adam Barth]

Comment on attachment 155547 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1458
> +    VisiblePosition startPosition(visiblePositionForWindowPoint(start));
> +    VisiblePosition endPosition(visiblePositionForWindowPoint(end));

nit: WebKit generally prefers the assignment form of the constructor:

VisiblePosition startPosition = visiblePositionForWindowPoint(start);

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463
> +    // Ensure startPosition is before endPosition.

This comment just states what the code does.  We generally frown on these sorts of comments.  When we include comments, we prefer comments that explain why the code does something rather than re-stating what the code does.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1474
> +    // Ensure startPosition is not at end of block.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1480
> +    // Ensure endPosition is not at start of block.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1496
> +    if (frame()->selection()->isContentEditable()) {

Prefer early return.

[Adam Barth]

My comments above are just trivialities.

@rniwa: Would you be willing to give this code a real review?

[Ryosuke Niwa]

Comment on attachment 155547 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1460
> +    if (startPosition.isNull() || endPosition.isNull() || startPosition == endPosition)

We never set a collapsed selection by this function? Why?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472
> +    if (frame()->selection()->isContentEditable()) {
> +        startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition);
> +        endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition);
> +        if (startPosition.isNull() || endPosition.isNull())
> +            return;
> +    }

This is done in setSelection.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1479
> +    if (startPosition != endPosition && isEndOfBlock(startPosition)) {
> +        VisiblePosition nextStartPosition(startPosition.next());
> +        if (!nextStartPosition.isNull())
> +            startPosition = nextStartPosition;
> +    }

Why are we doing this?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1535
> +            if (currentCaretRect.x() > point.x) {
> +                // Cursor should move left unless that would result in a line change or would result in the cursor
> +                // moving past the selection point.
> +                movePosition = currentPosition.previous();
> +                shouldMove = frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() > point.x;
> +                moveDirection = DirectionLeft;
> +            } else if (currentCaretRect.x() < point.x) {
> +                // Cursor should move right unless that would result in a line change or would result in the cursor
> +                // moving past the selection point.
> +                movePosition = currentPosition.next();
> +                shouldMove = frame()->view()->contentsToWindow(movePosition.absoluteCaretBounds()).x() < point.x;
> +                moveDirection = DirectionRight;
> +            }
> +
> +            if (shouldMove && inSameLine(currentPosition, movePosition))
> +                frame()->selection()->modify(FrameSelection::AlterationMove, moveDirection, CharacterGranularity, UserTriggered);
> +
> +            shouldMove = false;
> +            if (currentCaretRect.y() > point.y) {
> +                // Cursor should move up unless that would not result in a line change
> +                // as that means it's already at the top.
> +                shouldMove = !inSameLine(currentPosition, previousLinePosition(currentPosition, 0));
> +                moveDirection = DirectionBackward;
> +            } else if (currentCaretRect.y() < point.y) {
> +                // Cursor should move down unless that would not result in a line change
> +                // as that means it's already at the bottom.
> +                shouldMove = !inSameLine(currentPosition, nextLinePosition(currentPosition, 0));
> +                moveDirection = DirectionForward;
> +            }

What? Why do we need all of this code. This clearly doesn't work at all for bidirectional text.
See what EventHandler::handleMousePressEventSingleClick and other functions in EventHandler.cpp do.
We should be re-using the code there.

[Iain Merrick]

Comment on attachment 155547 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1460
>> +    if (startPosition.isNull() || endPosition.isNull() || startPosition == endPosition)
> 
> We never set a collapsed selection by this function? Why?

This method is called when dragging one of the selection handles. We don't allow a collapsed selection because that would cause us to exit selection mode. However, this method is *also* called when we specifically want to set a single insertion point (lines 1452-5). That's not good -- maybe selectRange() is too general and we should have separate methods for those two use cases.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1463
>> +    // Ensure startPosition is before endPosition.
> 
> This comment just states what the code does.  We generally frown on these sorts of comments.  When we include comments, we prefer comments that explain why the code does something rather than re-stating what the code does.

Yeah, we haven't been aggressive enough at policing these in the Android fork. I'll remove all these comments.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1535
>> +            }
> 
> What? Why do we need all of this code. This clearly doesn't work at all for bidirectional text.
> See what EventHandler::handleMousePressEventSingleClick and other functions in EventHandler.cpp do.
> We should be re-using the code there.

I'll refactor all this stuff.

[Iain Merrick]

Thanks for the comments! This is the code we're currently shipping with, so I wanted to start with that patch, but I'm happy to make all the requested changes.

[Adam Barth]

Thanks Iain.

[Iain Merrick]

Comment on attachment 155547 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472
>> +    }
> 
> This is done in setSelection.

I think this is incorrect. Assuming you mean FrameSelection::setSelection, that method doesn't seem to check for editable boundaries (both by code inspection, and by experimentally removing this code from the Android port).

FrameSelection::modifyExtendingRight and similar methods *do* clip at the editable boundaries, but setSelection doesn't. Is that correct, or am I misreading the code?

I don't think setSelection should truncate to the editable boundaries, because then there would be no way to select an entire different part of the page. Is there another suitable method call? Possibly FrameSelection::setSelectedRange?

[Iain Merrick]

Created attachment 156075 [details]
Patch

[Iain Merrick]

I've removed setCaretSelectionAtWindowPoint as that needs heavy rewriting. I'll address that in a later patch.

I've trimmed selectRange down to what I think is the minimal set of changes needed (apart from special handling for caret selections).

[Ryosuke Niwa]

Comment on attachment 155547 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1472
>>> +    }
>> 
>> This is done in setSelection.
> 
> I think this is incorrect. Assuming you mean FrameSelection::setSelection, that method doesn't seem to check for editable boundaries (both by code inspection, and by experimentally removing this code from the Android port).
> 
> FrameSelection::modifyExtendingRight and similar methods *do* clip at the editable boundaries, but setSelection doesn't. Is that correct, or am I misreading the code?
> 
> I don't think setSelection should truncate to the editable boundaries, because then there would be no way to select an entire different part of the page. Is there another suitable method call? Possibly FrameSelection::setSelectedRange?

I misspoke. This is done in VisibleSelection::VisibleSelection.

[Iain Merrick]

(In reply to comment #21)
> I misspoke. This is done in VisibleSelection::VisibleSelection.

Hmm, does that happen via VisibleSelection::validate()? (which the constructors generally end up calling)

Various things happen in there but I don't see any specific logic to check the editable boundaries, and the code behaves differently when we do it manally (as in this patch). Maybe I'm just looking in the wrong place?

[Ryosuke Niwa]

Comment on attachment 156075 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1466
> +    if (frame()->selection()->isContentEditable()) {
> +        startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition);
> +        endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition);
> +        if (startPosition.isNull() || endPosition.isNull())
> +            return;
> +    }

Adjustment around editing boundary is done in VisibleSelection::VisibleSelection > VisibleSelection::validate > VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries.

[Iain Merrick]

(In reply to comment #23)
> (From update of attachment 156075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156075&action=review
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1466
> > +    if (frame()->selection()->isContentEditable()) {
> > +        startPosition = frame()->selection()->selection().visibleStart().honorEditingBoundaryAtOrAfter(startPosition);
> > +        endPosition = frame()->selection()->selection().visibleEnd().honorEditingBoundaryAtOrBefore(endPosition);
> > +        if (startPosition.isNull() || endPosition.isNull())
> > +            return;
> > +    }
> 
> Adjustment around editing boundary is done in VisibleSelection::VisibleSelection > VisibleSelection::validate > VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries.

Aha, doh! Now I see it. Thanks!

I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end.

[Ryosuke Niwa]

(In reply to comment #24)
> Aha, doh! Now I see it. Thanks!
> 
> I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end.

Please don't manually adjust selection end points in WebKit code. That's extremely error prone.

[Iain Merrick]

(In reply to comment #25)
> (In reply to comment #24)
> > Aha, doh! Now I see it. Thanks!
> > 
> > I suspect I just have to be more careful about handling all four of start/end/base/extent, rather than just start and end.
> 
> Please don't manually adjust selection end points in WebKit code. That's extremely error prone.

Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation.

[Ryosuke Niwa]

(In reply to comment #26)
> Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation.

If you're adding code only for Android port, then why are you changing WebFrameImpl::selectRange?

Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B?

[Iain Merrick]

(In reply to comment #27)
> (In reply to comment #26)
> > Sorry, I'll clarify. Reading adjustSelectionToAvoidCrossingEditingBoundaries, it looks like when the selection starts in non-editable text and ends in editable text, the endpoint is adjusted rather than the start point. That's not what the Android port wants in this situation.
> 
> If you're adding code only for Android port, then why are you changing WebFrameImpl::selectRange?

Well, where's a better place to put it? This is part of the WebKit API and it needs to behave sensibly for all touch-based UIs.

> Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B?

No, this call is used when adjusting an existing selection. If points A and B are both inside an editable region, and point A is then moved before the start of the region, what *should* happen is that point A gets pinned at the start. What happens instead is that point B gets pinned to the end of the previous region -- not what the user wants.

Unfortunately the API doesn't include any information about whether the start or end is being adjusted. I suspect we need to either extend the API, or use a heuristic (for example, if one endpoint changes and the other doesn't, that's the endpoint that needs to be clipped against the editing boundaries).

I'll try pushing this logic into FrameSelection and/or Editor, which already have some similar code.

[Ryosuke Niwa]

(In reply to comment #28)
> > Also, are you saying that when a user selects text from point A to point B and they're not in the same editable region, you want point A to be adjusted instead of point B?
> 
> No, this call is used when adjusting an existing selection. If points A and B are both inside an editable region, and point A is then moved before the start of the region, what *should* happen is that point A gets pinned at the start. What happens instead is that point B gets pinned to the end of the previous region -- not what the user wants.

What you need to do is to treat point A as the "extent" of the selection, not the "base".  Selection has this concept of "base" and "extent" (similar to what we expose to JavaScript as "anchor" and "focus") and end points are adjusted to be inside the editable region of the "base". If either point is adjusted, then that end should be treated as the extent.

> Unfortunately the API doesn't include any information about whether the start or end is being adjusted.

Why can't we expose that information? Given what you want to do, it's insane not to expose that information.

[Ryosuke Niwa]

See https://developer.mozilla.org/en-US/docs/DOM/Selection. "base"="anchor" and "extent"="focus" for all practical purposes (they are technically different).

[Iain Merrick]

> > Unfortunately the API doesn't include any information about whether the start or end is being adjusted.
> 
> Why can't we expose that information? Given what you want to do, it's insane not to expose that information.

We already have several redundant-looking APIs in WebFrame:

    virtual void setSelectionToRange(const WebRange&);
    virtual void selectRange(const WebPoint& start, const WebPoint& end);
    virtual void selectRange(const WebRange&);

As there are no comments, the semantics are very unclear. I think we should clean those up before adding even more APIs -- do you agree?

[Ryosuke Niwa]

(In reply to comment #31)
> We already have several redundant-looking APIs in WebFrame:
> 
>     virtual void setSelectionToRange(const WebRange&);
>     virtual void selectRange(const WebPoint& start, const WebPoint& end);
>     virtual void selectRange(const WebRange&);
> 
> As there are no comments, the semantics are very unclear. I think we should clean those up before adding even more APIs -- do you agree?

Definitely. Also, the intent of each function should be clear from function names, not comments.

[Iain Merrick]

FrameSelection::setBase and setExtent don't have the semantics discussed above (honoring the boundaries of the current element if it's editable). Manually testing with the desktop UI confirms this: if I have some editable text selected, shift-clicking in a non-editable part of the page does a deselect-all. That's correct for desktop, not correct for a touch UI.

I'm thinking of extending FrameSelection::SetSelectionOption with a new "HonorEditingBoundaries" flag. How does that sound?

[Ryosuke Niwa]

(In reply to comment #33)
> FrameSelection::setBase and setExtent don't have the semantics discussed above (honoring the boundaries of the current element if it's editable). Manually testing with the desktop UI confirms this: if I have some editable text selected, shift-clicking in a non-editable part of the page does a deselect-all. That's correct for desktop, not correct for a touch UI.

I've never said that you should be using setBase and setExtent.

[Ryosuke Niwa]

Please observe the behavior of selection by a mouse drag. When you start a mouse drag inside an editable region and extent mouse moves out of the editable region, selection extends until the end of the editable region. You can read the code that does this in Source/page/EventHandler.cpp.

[Ryosuke Niwa]

See EventHandler::updateSelectionForMouseDrag at
http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp?124774#L780 for example.

[Iain Merrick]

I guess this gets back to your original comment about reusing that code. Just to be clear, do you want WebFrameImpl::selectRange to call through to EventHandler (either an existing method or a new method)? Or to somewhere else?

[Ryosuke Niwa]

(In reply to comment #37)
> I guess this gets back to your original comment about reusing that code. Just to be clear, do you want WebFrameImpl::selectRange to call through to EventHandler (either an existing method or a new method)? Or to somewhere else?

Yeah, that'll be good. Alternatively, you can extract some code from EventHandler and move it to FrameSelection as well.

[Iain Merrick]

Great, thanks for your patience!

[Iain Merrick]

Created attachment 156931 [details]
Patch

[Iain Merrick]

This code seems to work. Needs a test -- I'm looking at https://bugs.webkit.org/show_bug.cgi?id=93108 now.

[Ryosuke Niwa]

Comment on attachment 156931 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:760
> -static VisiblePosition selectionExtentRespectingEditingBoundary(const VisibleSelection& selection, const LayoutPoint& localPoint, Node* targetNode)
> +VisiblePosition EventHandler::visiblePositionRespectingEditingBoundary(const HitTestResult& hitTestResult)

Why are we renaming this function? This function is about adjusting extent, not either base or extent.

> Source/WebCore/page/EventHandler.cpp:762
> +    LayoutPoint layoutPoint = hitTestResult.localPoint();

Why are we renaming this variable to layoutPoint? That doesn't explain anything.
It's like naming a variable int or boolean.

> Source/WebCore/page/EventHandler.cpp:3714
> +    VisiblePosition startPosition = visiblePositionRespectingEditingBoundary(hitTestStart);
> +    VisiblePosition endPosition = visiblePositionRespectingEditingBoundary(hitTestEnd);

This is not right. Either end point needs to be base, and that's the one we should be adjusting, not both.

> Source/WebCore/page/EventHandler.cpp:3716
> +    VisibleSelection newSelection(startPosition, endPosition);

We need to know which point is base & extent here to create a selection properly.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1461
> +    frame()->eventHandler()->handleSelectRange(IntPoint(start.x, start.y), IntPoint(end.x, end.y));

We should definitely not be calling into eventHandler here since we're not handling an event.

[Iain Merrick]

Comment on attachment 156931 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:760
>> +VisiblePosition EventHandler::visiblePositionRespectingEditingBoundary(const HitTestResult& hitTestResult)
> 
> Why are we renaming this function? This function is about adjusting extent, not either base or extent.

There's nothing specific to extent in this function.

>> Source/WebCore/page/EventHandler.cpp:762
>> +    LayoutPoint layoutPoint = hitTestResult.localPoint();
> 
> Why are we renaming this variable to layoutPoint? That doesn't explain anything.
> It's like naming a variable int or boolean.

Oops, I think I meant localPoint (as that's what the parameter that I removed was called).

I'll switch it back to selectionEndPoint, though.

>> Source/WebCore/page/EventHandler.cpp:3716
>> +    VisibleSelection newSelection(startPosition, endPosition);
> 
> We need to know which point is base & extent here to create a selection properly.

There is no such distinction in the Android text selection model (and I believe touch-based ChromeOS too). There's a selection handle at each end of the selection, and either one can be moved freely.

What I could maybe do is have WebFrameImpl figure out which endpoint is moving, and define that one to be the extent. Does that sound plausible?

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1461
>> +    frame()->eventHandler()->handleSelectRange(IntPoint(start.x, start.y), IntPoint(end.x, end.y));
> 
> We should definitely not be calling into eventHandler here since we're not handling an event.

OK, I'll move the shared bits of logic into FrameSelection.

[Ryosuke Niwa]

(In reply to comment #43)
> >> Source/WebCore/page/EventHandler.cpp:3716
> >> +    VisibleSelection newSelection(startPosition, endPosition);
> > 
> > We need to know which point is base & extent here to create a selection properly.
> 
> There is no such distinction in the Android text selection model (and I believe touch-based ChromeOS too). There's a selection handle at each end of the selection, and either one can be moved freely.
> 
> What I could maybe do is have WebFrameImpl figure out which endpoint is moving, and define that one to be the extent. Does that sound plausible?

Yes, that's exactly what we need here.

[Iain Merrick]

Created attachment 157198 [details]
Patch

[Iain Merrick]

I was trying to do too many things at once, so I'm now aiming for a minimal refactoring:
- Extract EventHandler::selectionExtentRespectingEditingBoundary into VisibleSelection.
- Call that from WebFrameImpl::visiblePositionForWindowPoint.

I have re-added the unit test from https://bugs.webkit.org/show_bug.cgi?id=93108, which got rolled back due to Windows failures. I'll extend it to cover the editable element case and check that it works on all platforms.

I think it would be good to move the targetNode() function into HitTestResult. That would remove some duplicate code, and we could also simplify the parameter list of selectionExtentRespectingEditingBoundary. Not essential for this patch, so I added a FIXME comment.

I have NOT addressed the base/extent confusion that Ryosuke identified. I think that should be done in a separate patch, by an API change (the SelectRange message should specify which endpoint is moving).

[Ryosuke Niwa]

Comment on attachment 157198 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Move selectionExtentRespectingEditingBoundary() into VisibleSelection
> +        https://bugs.webkit.org/show_bug.cgi?id=92513

Please file a new bug and post the patch there.
We don't typically one patch per each bug.

> Source/WebKit/chromium/tests/data/select_range_basic.html:1
> +<span id='target'>some test text for testing</span>

Missing DOCTYPE.

> Source/WebKit/chromium/tests/data/select_range_iframe.html:1
> +<html>

Ditto.

> Source/WebKit/chromium/tests/data/select_range_scroll.html:1
> +<div style='height:3000px'>

Ditto.

[Iain Merrick]

Comment on attachment 157198 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=92513
> 
> Please file a new bug and post the patch there.
> We don't typically one patch per each bug.

This is the same bug! Did you make this comment because the ChangeLog entry is different?

I intended that WebCore/ChangeLog should explain what was happening in WebCore, but to minimize confusion I'll use the same text in WebCore/ChangeLog and WebKit/ChangeLog.

Fixed.

>> Source/WebKit/chromium/tests/data/select_range_basic.html:1
>> +<span id='target'>some test text for testing</span>
> 
> Missing DOCTYPE.

I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>".

If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug).

I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know.

Not changed.

[Peter Beverloo]

(In reply to comment #48)
> 
> >> Source/WebKit/chromium/tests/data/select_range_basic.html:1
> >> +<span id='target'>some test text for testing</span>
> > 
> > Missing DOCTYPE.
> 
> I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>".
> 
> If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug).
> 
> I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know.
> 
> Not changed.

Adding a doctype will mean the test is ran in standards mode, something that generally is a good practice, unless the test will rely on specific quirks behavior. This also implies that the tests will be much easier to port to other browsers, may they be interested, as standards mode is much more interoperable than quirks mode. WebKit has thousands of tests imported from other browsers and official test-suites.

Going forth, I agree with Ryosuke that using standards mode when new tests are being added is the way to go.

[Iain Merrick]

(In reply to comment #49)
> (In reply to comment #48)
> > 
> > >> Source/WebKit/chromium/tests/data/select_range_basic.html:1
> > >> +<span id='target'>some test text for testing</span>
> > > 
> > > Missing DOCTYPE.
> > 
> > I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>".
> > 
> > If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug).
> > 
> > I'm inclined to leave out the doctype, for consistency with the surrounding code. But I can add "<!DOCTYPE html>" to these three files if you really think it's a blocker for this bug -- let me know.
> > 
> > Not changed.
> 
> Adding a doctype will mean the test is ran in standards mode, something that generally is a good practice, unless the test will rely on specific quirks behavior. This also implies that the tests will be much easier to port to other browsers, may they be interested, as standards mode is much more interoperable than quirks mode. WebKit has thousands of tests imported from other browsers and official test-suites.
> 
> Going forth, I agree with Ryosuke that using standards mode when new tests are being added is the way to go.

OK, will fix.

[Iain Merrick]

Created attachment 157465 [details]
Patch

[Iain Merrick]

Work-in-progress patch. No review needed but any comments appreciated.

Doctypes fixed. I made the tests a bit stricter, which revealed an annoying off-by-one error (I assume this is why it failed previously Windows): hitTest() and selectionBounds() aren't round-trip robust. Working on a fix or workaround for that.

VisibleSelection::validate() and EventHandler::selectionExtentRespectingEditingBoundary() also have subtly different endpoint behaviour (VisibleSelection clips your endpoint to the end of the editable element, EventHandler clips it to the start of the next element), so I'm not calling selectionExtentRespectingEditingBoundary refactoring.

I added doctypes to the test pages, and added a new one for editable element selection.

[Iain Merrick]

(In reply to comment #52)
> so I'm not calling selectionExtentRespectingEditingBoundary refactoring.

Er, mangled prose there, sorry. I'm not reusing that method any more, so I also removed the refactoring.

[Iain Merrick]

Hmm, unit tests seem to be using tiny font -- less than 1 pixel square per glyph. That makes it impossible to select text accurately with integer coordinates. Trying to figure out where that font gets set.

[Iain Merrick]

Setting font-size explicitly in the test pages gives *different* selection errors. It consistently misses some chars at the end, or selects too many. Very strange.

[Ryosuke Niwa]

(In reply to comment #48)
> (From update of attachment 157198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157198&action=review
> 
> >> Source/WebCore/ChangeLog:4
> >> +        https://bugs.webkit.org/show_bug.cgi?id=92513
> > 
> > Please file a new bug and post the patch there.
> > We don't typically one patch per each bug.
> 
> This is the same bug! Did you make this comment because the ChangeLog entry is different?

Is this the only patch you're going to post to this bug? (i.e. once this patch is landed, there's nothing else to do?) We try to land exactly one patch per bug.

If you're intending to split the original patch into 2-3 pieces, and then those split pieces should be posted on new bugs that blocks this one.

Also, the text above the bug URL should be the bug title. If you think the current bug title does not reflect what the patch does or the problem you're solving, then you should also update the bug title.

> >> Source/WebKit/chromium/tests/data/select_range_basic.html:1
> >> +<span id='target'>some test text for testing</span>
> > 
> > Missing DOCTYPE.
> 
> I had a look at the other files in Source/WebKit/chromium/tests/data/. Out of 54 .html files, only 6 have doctype tags; and those 6 are just "<!DOCTYPE html>".
> 
> If we think doctypes are important, we should add them to all the test files (but that's outside the scope of this bug).

Yes. We should have DOCTYPE. Otherwise we'll be in quirks mode, which isn't spec'ed anywhere.

[Iain Merrick]

Any idea why WebViewImpl::selectionBounds would give strange results? (much too small, it seems to me) Is there something odd about the rendering context that WebFrameTest sets up?

[Iain Merrick]

(In reply to comment #56)
[...]
> If you're intending to split the original patch into 2-3 pieces, and then those split pieces should be posted on new bugs that blocks this one.
> 
> Also, the text above the bug URL should be the bug title. If you think the current bug title does not reflect what the patch does or the problem you're solving, then you should also update the bug title.

I think there are an indefinite number of improvements that could be made to all of this code, so I'll file separate bugs.

[WebKit Review Bot]

Comment on attachment 157465 [details]
Patch

Attachment 157465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13454953

New failing tests:
WebFrameTest.SelectRange

[WebKit Review Bot]

Created attachment 157518 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

> Any idea why WebViewImpl::selectionBounds would give strange results? (much too small, it seems to me) Is there something odd about the rendering context that WebFrameTest sets up?

I don't know how many WebFrameTest actually render things.  It's possible that things aren't set up in a reasonable way in the testing framework because we tend to use the LayoutTest framework for things related to rendering.  It's possible to change your test into a LayoutTest, but it's unclear to me if that's the right thing to do here.

I get the sense that you're getting a bit frustrated by the back-and-forth with rniwa.  I'll try building your patch and seeing if I can figure out the issue.

[Adam Barth]

@Iain: I'm not sure I understand these tests.  The selectionBounds functions gives you a bounding box around the selection.  If you then set the selection to that bounding box, it's not surprising that you're catching some additional text in the selection.  The bounding box is an upper bound, after all.

[Adam Barth]

Created attachment 157785 [details]
cleaned up patch, but no test

[Adam Barth]

Given that there are currently zero tests for WebFrame::selectRange, I don't think we should block this patch on lacking a test.  It makes me sad, but that's the current state of affairs.

[Adam Barth]

Created attachment 157787 [details]
fixed ChangeLog

[WebKit Review Bot]

Comment on attachment 157787 [details]
fixed ChangeLog

Clearing flags on attachment: 157787

Committed r125329: <http://trac.webkit.org/changeset/125329>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Iain Merrick]

Adam, thanks for following up!

I didn't write that test originally, but the code looks sensible to me, so I'll continue to investigate and see if I can get it working reliably. I think it's still a better approach than a layout test, as that would require adding a SelectRange callback to the test runner.

I'll also open new bugs for the refactoring changes I mentioned, like moving targetNode().