Bug 105189 – Add function to move caret selection towards a point

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Hopman]

Add function to move caret selection towards a point

[Chris Hopman]

Created attachment 179761 [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.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." 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]
Source/WebKit/chromium/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 13 in 7 files


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

[Chris Hopman]

Created attachment 179763 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 179763 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1393
> +void WebFrameImpl::moveCaretSelectionTowardsWindowPoint(const WebPoint& point)

Can you provide more details on how this function will be used by Chromium?

I'm also having a hard time making sense of the definition of this new
function.  It seems highly specific to your particular use case.  I
wonder if there isn't a better decomposition of this into more general
purpose methods.

[Chris Hopman]

Sure. On Android, in an editable area, you can have an insertion handle. The user can drag this around to move where the caret selection is in the editable area. When those movements are within the editable area, the expected behavior is simple, move the caret to where its dragged. When a user drags the insertion handle outside of the editable area, the expected behavior is that the caret stays inside the editable area and vertically/horizontally aligned with the event position (unlike, for example, a range selection), and the edit box should scroll (slowly) towards the event position. Other touch-based(/touch-supporting) platforms would also likely want similar behavior.

When I originally wrote this, I tried to decompose it into:
1. Move the selection to the closest horizontal/vertically aligned position in the visible editable area.
2. If needed, move the selection 1 more line/character towards the point (i.e. scroll one line/character).
I did not find either of those behaviors already available, and that direction didn't work out as well as the way in this patch (quite possibly due to my inexperience in WebKit).

That being said, any suggestions for better approaches would be appreciated.

https://codereview.chromium.org/11593011/ is a chromium change that uses this new function. (Though that change is rather trivial other than the tests).

[Ryosuke Niwa]

Comment on attachment 179763 [details]
Patch

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

It appears to me that this whole idea of moving caret towards a point is flawed in the presence of vertical writing mode bidirectional text.

> Source/WebCore/ChangeLog:3
> +        Fix an issue where page scale factor was not taken into account

Why is this different from the bug title?

[Ryosuke Niwa]

Comment on attachment 179763 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1398
> +    VisiblePosition movePosition = frame()->selection()->selection().visibleStart();

This should be visibleBase();

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1401
> +    IntRect editableRect = editable->renderer()->enclosingLayer()->absoluteBoundingBox();

This is not right. You need to obtain the bounding box of the roo editable element.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1405
> +    bool movingUp = point.y < windowMovePosition.y();

This is wrong when we're in a vertical writing mode. r- because of this.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1407
> +        VisiblePosition newMovePosition = movingUp ? previousLinePosition(movePosition, 0) : nextLinePosition(movePosition, 0);

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1411
> +        if (movingUp ? point.y > windowNewMovePosition.y() : point.y < windowNewMovePosition.y())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1414
> +        if (movingUp ? editableRect.y() > windowNewMovePosition.y(): editableRect.y() + editableRect.height() < windowNewMovePosition.y())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1419
> +    bool movingLeft = point.x < windowMovePosition.x();

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1421
> +        VisiblePosition newMovePosition = movingLeft ? movePosition.left(true) : movePosition.right(true);

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1425
> +        if (movingLeft ? point.x > windowNewMovePosition.x() : point.x < windowNewMovePosition.x())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1428
> +        if (movingLeft ? editableRect.x() > windowNewMovePosition.x(): editableRect.x() + editableRect.width() < windowNewMovePosition.x())

Ditto.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1432
> +    frame()->selection()->moveTo(movePosition, UserTriggered);

You need to call FrameSelection::shouldChangeSelection before calling moveTo.
I'd suggest adding this entire function to FrameSelection instead.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1434
> +    if (viewImpl()->client())
> +        viewImpl()->client()->didChangeSelection(true);

Why are we directly calling back to didChangeSelection? moveTo calls FrameSelection::setSelection, which in turn calls Editor::respondToChangedSelection and then EditorClient::respondToChangedSelection.

[Chris Hopman]

Created attachment 180067 [details]
Patch

[Chris Hopman]

With some help from eseidel and rniwa, this is now a much better approach.

I am unsure about the point conversion, there is a decent chance that I got that wrong.

[Simon Fraser (smfr)]

Comment on attachment 180067 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2108
> +        LayoutRect unscaledRect = rect;
> +        Frame* frame = renderer()->frame();
> +        if (frame)
> +            unscaledRect.scale(1 / frame->page()->pageScaleFactor());
> +        LayoutRect exposeRect = LayoutRect(unscaledRect.x() + scrollXOffset(), unscaledRect.y() + scrollYOffset(), unscaledRect.width(), unscaledRect.height());

Please do this in a separate patch with its own testcase.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1399
> +    Element* editable = frame()->selection()->rootEditableElement();
> +    IntPoint contentsPoint = frame()->view()->windowToContents(IntPoint(point));
> +    float pageScaleFactor = frame()->page()->pageScaleFactor();
> +    contentsPoint.scale(1 / pageScaleFactor, 1 / pageScaleFactor);
> +    LayoutPoint localPoint(editable->renderer()->absoluteToLocal(contentsPoint));
> +    VisiblePosition position = editable->renderer()->positionForPoint(localPoint);
> +    frame()->selection()->moveTo(position, UserTriggered);

YOu should test this in a subframe with page scale applied. I'm not sure whether windowToContents() on a subframe applies the page scale. Also, if you're in a subframe, pageScale doesn't apply. frameScaleFactor() is more useful here.

It also looks like you assume windowToContents() gives you an "absolute" point in that frame (relative to the top left of the document, even when scrolled), since you then call absoluteToLocal() on it.

A good test here would be a zoomed page with both the root and the subframe scrolled. Ideally you'd also have CSS transforms on an ancestor of the frame, and inside the frame.

[Chris Hopman]

Created attachment 180193 [details]
Patch

[Eric Seidel (no email)]

Does chromium draw the insertion handles, or are they from teh OS?  I'm still surprised this needs special handling, separate from just exetending the selection with a pointing device beyond the bounds of the text area would.

[Chris Hopman]

(In reply to comment #13)
> Does chromium draw the insertion handles, or are they from teh OS?  I'm still surprised this needs special handling, separate from just exetending the selection with a pointing device beyond the bounds of the text area would.

Chromium draws them. I would be happy to reuse existing behavior, but I was unable to find it. For selection handles (i.e. range selections) we are using the behavior of extending selections (like dragging with a mouse). The insertion handle moves a caret selection and I could not find any such existing behavior.

[Eric Seidel (no email)]

rniwa would probably know best, if you can catch him (either via bugmail or #webkit).

[Eric Seidel (no email)]

A caret is just a collapsed selection.  So I would expect them to go through the same code paths.

On a desktop you move the caret with the arrow keys, but I doubt we have special handling for arrow keys + text fields, rather I suspect the arrow keys just move the selection and the selection controller causes the text area to scroll?

[Ryosuke Niwa]

(In reply to comment #16)
> A caret is just a collapsed selection.  So I would expect them to go through the same code paths.

Right. EventHandler handles both cases.

> On a desktop you move the caret with the arrow keys, but I doubt we have special handling for arrow keys + text fields, rather I suspect the arrow keys just move the selection and the selection controller causes the text area to scroll?

We don’t have a special case for arrow keys in textarea although we have a special code for editable regions (since that’s the only place where collapsed selection is rendered). FrameSelection::setSelection talks to RenderView to scroll relevant layers to "reveal" the selection.

[Eric Seidel (no email)]

(In reply to comment #14)
> (In reply to comment #13)
> Chromium draws them. I would be happy to reuse existing behavior, but I was unable to find it. For selection handles (i.e. range selections) we are using the behavior of extending selections (like dragging with a mouse). The insertion handle moves a caret selection and I could not find any such existing behavior.

I suspect this is the core confusion.  For other ports, WebKit itself draws all the input UI, like the cursor, etc.  In this case, Chromium is drawing it's own selection UI (in addition to WebKits) and thus needs a hook to tell WebKit to update it's part of things.   As Ryosuke points out, if this were in WebKit, most of the code would just be in EventHandler.cpp (the drawing code likely in RenderThemeChromiumAndroid?).

I'm not sure this patch is wrong.  Just slightly atypical for WebKit's previous editing design.

Again, rniwa has been in this code much more recently than I, so I can correct any errors I may have made above. :)

[Ryosuke Niwa]

(In reply to comment #18)
>
> I suspect this is the core confusion.  For other ports, WebKit itself draws all the input UI, like the cursor, etc.  In this case, Chromium is drawing it's own selection UI (in addition to WebKits) and thus needs a hook to tell WebKit to update it's part of things.   As Ryosuke points out, if this were in WebKit, most of the code would just be in EventHandler.cpp (the drawing code likely in RenderThemeChromiumAndroid?).

It’s drawn by WebCore even. See CaretBase::paintCaret in FrameSelection.cpp.

> I'm not sure this patch is wrong.  Just slightly atypical for WebKit's previous editing design.

I concur. This "feels wrong".

[Chris Hopman]

(In reply to comment #16)
> A caret is just a collapsed selection.  So I would expect them to go through the same code paths.
> 
> On a desktop you move the caret with the arrow keys, but I doubt we have special handling for arrow keys + text fields, rather I suspect the arrow keys just move the selection and the selection controller causes the text area to scroll?

But, my understanding (admittedly, little), is that when expanding a range selection you have a reference for what editable area the range needs to stay in. At least for our selection handles we end up calling selectRange(base, extent) where base is the fixed side (i.e. in the editable area) of the range and extent is where it is dragged to. This then ensures that the selection stays within the base's root editable element. Then, for modifying the selection with arrow keys (also home/end/etc), these use FrameSelection::modify which only moves one step in a particular granularity.

[Chris Hopman]

(In reply to comment #18)
> (In reply to comment #14)
> > (In reply to comment #13)
> > Chromium draws them. I would be happy to reuse existing behavior, but I was unable to find it. For selection handles (i.e. range selections) we are using the behavior of extending selections (like dragging with a mouse). The insertion handle moves a caret selection and I could not find any such existing behavior.
> 
> I suspect this is the core confusion.  For other ports, WebKit itself draws all the input UI, like the cursor, etc.  In this case, Chromium is drawing it's own selection UI (in addition to WebKits) and thus needs a hook to tell WebKit to update it's part of things.   As Ryosuke points out, if this were in WebKit, most of the code would just be in EventHandler.cpp (the drawing code likely in RenderThemeChromiumAndroid?).
> 
Yes this is adding a hook similar to the existing WebFrameImpl::selectRange. The behavior is similar to how I would expect the following to work:
  selectRange(frame()->selection()->selection().visibleBase(), point);
  selectRange(frame()->selection()->selection().visibleExtent(), frame()->selection()->selection().visibleExtent()); // not sure if .visibleExtent() is correct here

I don't really like doing two select ranges when the first one is really only used to constrain the point to being in the root editable element of the current selection. Also, ::selectRange doesn't appear to honor the platform editing behavior, which is required (though I could be wrong, or maybe it's unintentional).

[Chris Hopman]

(In reply to comment #19)
> (In reply to comment #18)
> > I'm not sure this patch is wrong.  Just slightly atypical for WebKit's previous editing design.
> 
> I concur. This "feels wrong".

Hm, ok, I have a proposal:

What's really wanted is a WebFrame::selectRange() that (1) honors the platform editing behavior, and (2) is restricted to the root editable element of the current selection.

So, what if I change selectRange to do (1) and make it optionally do (2)?

[Chris Hopman]

(In reply to comment #18)
> (In reply to comment #14)
> > (In reply to comment #13)
> > Chromium draws them. I would be happy to reuse existing behavior, but I was unable to find it. For selection handles (i.e. range selections) we are using the behavior of extending selections (like dragging with a mouse). The insertion handle moves a caret selection and I could not find any such existing behavior.
> 
> I suspect this is the core confusion.  For other ports, WebKit itself draws all the input UI, like the cursor, etc.  In this case, Chromium is drawing it's own selection UI (in addition to WebKits) and thus needs a hook to tell WebKit to update it's part of things.   As Ryosuke points out, if this were in WebKit, most of the code would just be in EventHandler.cpp (the drawing code likely in RenderThemeChromiumAndroid?).
> 
> I'm not sure this patch is wrong.  Just slightly atypical for WebKit's previous editing design.
> 
> Again, rniwa has been in this code much more recently than I, so I can correct any errors I may have made above. :)

Looking through the history of these functions... a WebFrame::moveCaret() function used to exist. It was merged into selectRange with https://bugs.webkit.org/show_bug.cgi?id=96508. Since selectRange is not the same behavior as moveCaret (I have a local patch that extends selectRange to support the moveCaret behavior, but it is not any simpler than this patch), and there is precedent for such a moveCaret hook, I think this approach should be fine (though I do prefer the ::moveCaret() function name to the name in my last patch).

eseidel, rniwa: What do you think?

[Eric Seidel (no email)]

Comment on attachment 180193 [details]
Patch

I suspect this is uneeded.  But I don't want to block you any more than we have, and I don't think we're making further design progress on this bug.

[Ryosuke Niwa]

Comment on attachment 180193 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1395
> +    LayoutPoint localPoint(editable->convertFromPage(contentsPoint));

We should be using = here.

[WebKit Review Bot]

Comment on attachment 180193 [details]
Patch

Clearing flags on attachment: 180193

Committed r138893: <http://trac.webkit.org/changeset/138893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.