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".
Created attachment 160944 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
rniwa is the ideal person to review this patch, but he's on vacation.
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.
Adam, did you manage to find anyone?
Nope. I'll take a look on Tuesday. (Today is a national holiday in the US.)
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?
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
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.
Created attachment 162521 [details] Patch
Comment on attachment 162521 [details] Patch Setting cq+.
Comment on attachment 162521 [details] Patch Clearing flags on attachment: 162521 Committed r127776: <http://trac.webkit.org/changeset/127776>
All reviewed patches have been landed. Closing bug.