Bug 93998 – [chromium] More Android changes to WebFrameImpl::selectRange

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Iain Merrick]

Followup to https://bugs.webkit.org/show_bug.cgi?id=92513

WebFrameImpl::selectRange() is still missing some functionality needed on the Android platform:

1. When dragging the selection handles, we shouldn't be able to collapse the selection; it should always be at least one character wide.

2. But we also need a way of explicitly switching to a collapsed selection (that is, an insertion caret rather than a highlighted selection). Currently the Android code uses the convention of calling selectRange with start == end, but this is fragile because the user could do so accidentally (thus failing requirement 1, above).

I think an API change might be useful:

- The editing code distinguishes between base and extent, not just start and end. To get the correct behaviour when the selection is editable, I had to clumsily set the endpoints one at a time.

- The unit tests in WebFrameTest call selectionBounds(), then call selectRange() with the same values, expecting to end up with the same selection range. I think that's a reasonable expectation, but it doesn't actually work in practice! There's an off-by-one pixel error at the lower-right of the selection range; and there may be other reasons why selectionBounds() and selectRange() aren't round-trip robust.

On Android, there are draggable handles around the selection range, and typically only one handle at a time is moved. So maybe the API could be updated to allow one endpoint to be moved, the other remaining fixed. That doesn't directly fix the off-by-one thing, but you would at least be able to say "move the start handle to here, leave the end handle where it is".

[Iain Merrick]

Created attachment 160944 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

rniwa is the ideal person to review this patch, but he's on vacation.

[Adam Barth]

I did a first pass, and it looked reasonable to me, but I'm not an editing expert. Let me see if I can find someone more expert in this area to take a look.

[Iain Merrick]

Adam, did you manage to find anyone?

[Adam Barth]

Nope.  I'll take a look on Tuesday.  (Today is a national holiday in the US.)

[Adam Barth]

Comment on attachment 160944 [details]
Patch

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

I'd really like rniwa to take a look at this patch when he gets back from vacation.  Based on the discussion in the previous bug, this patch looks plausible to me, but I'm far from an expert here.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:-1482
> -    if (frame()->selection()->shouldChangeSelection(newSelection))

Looks like we lost this call.  Is this no longer needed?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1486
> +    if (selection.isNone())
> +        return false;

Should this ASSERT?  Is it an error to call this function without a current selection?

[WebKit Review Bot]

Comment on attachment 160944 [details]
Patch

Rejecting attachment 160944 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
romium/src/WebFrameImpl.cpp
patching file Source/WebKit/chromium/src/WebFrameImpl.h
patching file Source/WebKit/chromium/tests/WebFrameTest.cpp
Hunk #6 FAILED at 1066.
1 out of 6 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej
patching file Source/WebKit/chromium/tests/data/text_selection.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13764349

[Iain Merrick]

Comment on attachment 160944 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:-1482
>> -    if (frame()->selection()->shouldChangeSelection(newSelection))
> 
> Looks like we lost this call.  Is this no longer needed?

Oops, good catch. setNonDirectionalSelectionIfNeeded() calls that internally, but setSelection() doesn't. I'll put it back in.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1486
>> +        return false;
> 
> Should this ASSERT?  Is it an error to call this function without a current selection?

I thought it best to return a bool result code, for consistency with the existing selectWordAroundCaret.

This is going to be called from way over in Java-land, so it seems good to be defensive about the exact editing state, rather than just asserting that it's correct. For example, a Javascript hook might clear the selection concurrently with the Java UI code.

[Iain Merrick]

Created attachment 162521 [details]
Patch

[Peter Beverloo]

Comment on attachment 162521 [details]
Patch

Setting cq+.

[WebKit Review Bot]

Comment on attachment 162521 [details]
Patch

Clearing flags on attachment: 162521

Committed r127776: <http://trac.webkit.org/changeset/127776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.