WebFrame::selectRange and moveCaret should behave like mouse selection

WebFrame::selectRange should behave like moveCaret

selectRange and moveCaret are used to alter the current selection by a
user dragging an insertion/selection handle on Android and ChromeOS.
There are two expected behaviors that this change fixes. First, if the
current selection is in an editable element, the new selection will be
within the same editable element. Second, the selection should stay
horizontally aligned with the handle.

Currently, selectRange doesn't even consider the second behavior and it
sort of gets the first behavior by restricting the resulting selection
to not cross editable boundaries and assuming that the focus point is
within the current editable element. This is incredibly fragile.

This extracts the logic used for determining selection when dragging the 
mouse, selectionExtentRespectingEditingBoundary, from EventHandler.cpp so 
that it can also be used for selectRange/moveCaret so that all these 
behaviors can be consistent.

To better reflect what these functions do (and that they are really
paired together) this change introduces new names for these two
functions: WebFrame::moveCaretSelection and WebFrame::moveRangeSelection.

NOTRY=true
BUG=232385, 238329, 238334
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151125

[cjhopman, 2013-05-06 21:49:37]

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/pub...
File Source/WebKit/chromium/public/WebFrame.h (right):

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/pub...
Source/WebKit/chromium/public/WebFrame.h:459: virtual void
moveCaretSelectionTowardsWindowPoint(const WebPoint&) = 0;
These two will be removed once chromium is updated to use
moveRangeSelection/moveCaretSelection.

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/WebFrameTest.cpp:2418: TEST_F(WebFrameTest,
DISABLED_PositionForPointTest) {
I don't know how to actually write this test (should be a layout test of some
sort?), but this demonstrates the problem with contenteditable spans. Line 2424
below fails because positionForPoint on a contenteditable span's renderer always
gives offset 0 here (see the crbug).

[cjhopman, 2013-05-06 21:50:18]

leviw: ptal

[leviw_travelin_and_unemployed, 2013-05-06 22:38:47]

Typo in commit message: "current selection is in _and_ editable element"

I don't think this is more broken than visiblePositionForWindowPoint already
was, but I'm not convinced that code was right before. Ping me if you want to
discuss this further.

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebFrameImpl.cpp (right):

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFrameImpl.cpp:1409: VisiblePosition
WebFrameImpl::visiblePositionForWindowPoint(const WebPoint& point, RenderObject*
renderer)
I'd give this a more descriptive name than renderer... something like
rootRenderer perhaps?

https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFrameImpl.cpp:1415: return
renderer->positionForPoint(localPoint);
Your issue is that positionForPoint isn't really meant to be used this way for
inlines. That hit-testing is actually handled by
RenderBlock::positionForPointWithInlineChildren. I believe this code would be
better off letting positionForPoint from the root do the work, then checking to
see if the result is a descendant of the root editable element you want instead.

[cjhopman, 2013-05-14 01:02:54]

So, I've extracted selectionExtentRespectingEditingBoundary from
EventHandler.cpp so that it can be used for these functions. This gets us closer
to handling WebFrame::selectRange like mouse selection but there is still some
way to go. 

Specifically, we probably want to change this so it does setup at 'base' (like
on mouse down) and then extend the selection to 'extent' (like on mouse drag).

Note that this still has the issue with contenteditable spans (since mouse
selection uses logic very similar to that I was using in the first patch). I'm
not sure if this is a real issue or not as it only happens in webkit_unit_tests
(i.e. in content shell on the same page we get the expected positionForPoint
result).

leviw: wdyt?


On 2013/05/06 22:38:47, Levi wrote:
> Typo in commit message: "current selection is in _and_ editable element"
> 
> I don't think this is more broken than visiblePositionForWindowPoint already
> was, but I'm not convinced that code was right before. Ping me if you want to
> discuss this further.
> 
>
https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
> File Source/WebKit/chromium/src/WebFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
> Source/WebKit/chromium/src/WebFrameImpl.cpp:1409: VisiblePosition
> WebFrameImpl::visiblePositionForWindowPoint(const WebPoint& point,
RenderObject*
> renderer)
> I'd give this a more descriptive name than renderer... something like
> rootRenderer perhaps?
> 
>
https://codereview.chromium.org/15017002/diff/3001/Source/WebKit/chromium/src...
> Source/WebKit/chromium/src/WebFrameImpl.cpp:1415: return
> renderer->positionForPoint(localPoint);
> Your issue is that positionForPoint isn't really meant to be used this way for
> inlines. That hit-testing is actually handled by
> RenderBlock::positionForPointWithInlineChildren. I believe this code would be
> better off letting positionForPoint from the root do the work, then checking
to
> see if the result is a descendant of the root editable element you want
instead.

[cjhopman, 2013-05-17 01:11:33]

leviw: ping

[leviw_travelin_and_unemployed, 2013-05-21 19:21:33]

On 2013/05/14 01:02:54, cjhopman wrote:
> Note that this still has the issue with contenteditable spans (since mouse
> selection uses logic very similar to that I was using in the first patch). I'm
> not sure if this is a real issue or not as it only happens in
webkit_unit_tests
> (i.e. in content shell on the same page we get the expected positionForPoint
> result).

I can't explain the different behavior, but suspect this is a real issue. I'll
accept it's not a regression either way and not block this from moving forward.

[leviw_travelin_and_unemployed, 2013-05-21 19:21:40]

https://codereview.chromium.org/15017002/diff/3002/Source/core/editing/Visibl...
File Source/core/editing/VisiblePosition.h (right):

https://codereview.chromium.org/15017002/diff/3002/Source/core/editing/Visibl...
Source/core/editing/VisiblePosition.h:144: VisiblePosition
visiblePositionRespectingEditingBoundary(const VisibleSelection&
currentSelection, const LayoutPoint& localPoint, Node* targetNode);
The dependency on VisibleSelection here is wrong. Make this a member of
VisibleSelection instead.

[cjhopman, 2013-05-22 23:45:10]

https://codereview.chromium.org/15017002/diff/3002/Source/core/editing/Visibl...
File Source/core/editing/VisiblePosition.h (right):

https://codereview.chromium.org/15017002/diff/3002/Source/core/editing/Visibl...
Source/core/editing/VisiblePosition.h:144: VisiblePosition
visiblePositionRespectingEditingBoundary(const VisibleSelection&
currentSelection, const LayoutPoint& localPoint, Node* targetNode);
On 2013/05/21 19:21:40, Levi wrote:
> The dependency on VisibleSelection here is wrong. Make this a member of
> VisibleSelection instead.

Done.

[leviw_travelin_and_unemployed, 2013-05-22 23:50:02]

https://codereview.chromium.org/15017002/diff/33001/Source/core/page/EventHan...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/15017002/diff/33001/Source/core/page/EventHan...
Source/core/page/EventHandler.cpp:32: #include "SVGNames.h"
I don't think you meant to have all these SVG changes in this patch, did you?

[cjhopman, 2013-05-22 23:58:33]

https://codereview.chromium.org/15017002/diff/33001/Source/core/page/EventHan...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/15017002/diff/33001/Source/core/page/EventHan...
Source/core/page/EventHandler.cpp:32: #include "SVGNames.h"
On 2013/05/22 23:50:02, Levi wrote:
> I don't think you meant to have all these SVG changes in this patch, did you?

Looks like I rebased this change from ps#3 to ps#4. The SVG (and some other
changes) you see when comparing vs patchset 3 are from that (but aren't actually
included in this patch -- see the "View" link).

I don't yet know a good (and easy) way to rebase/merge/sync a change and make it
still easy to compare between patchsets.

[leviw_travelin_and_unemployed, 2013-05-23 22:52:18]

Okay. lgtm.

[commit-bot: I haz the power, 2013-05-23 23:27:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/15017002/33001

[commit-bot: I haz the power, 2013-05-23 23:40:07]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[cjhopman, 2013-05-23 23:44:30]

abarth: Source/WebKit/*

[jamesr, 2013-05-23 23:46:45]

lgtm

https://chromiumcodereview.appspot.com/15017002/diff/33001/Source/WebKit/chro...
File Source/WebKit/chromium/public/WebFrame.h (right):

https://chromiumcodereview.appspot.com/15017002/diff/33001/Source/WebKit/chro...
Source/WebKit/chromium/public/WebFrame.h:466: // new selection will be
restricted to the root editable element.
these points are still in Window coordinates, right? if you're losing the
coordinate space in the function name please at least document the space in the
comment

[commit-bot: I haz the power, 2013-05-23 23:58:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/15017002/51002

[commit-bot: I haz the power, 2013-05-24 01:20:48]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-24 01:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/15017002/51002

[commit-bot: I haz the power, 2013-05-24 02:34:12]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-24 16:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/15017002/51002

[commit-bot: I haz the power, 2013-05-24 17:35:14]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-24 19:33:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/15017002/51002

[commit-bot: I haz the power, 2013-05-24 19:33:47]

Change committed as 151125