Bug 91661 – [Chromium] Support scrolling and zooming to focused input elements

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Peter Beverloo]

When an input element gets focused, the element should be positioned in a convenient position and size on the user's screen for editing.

Downstream CL is:
https://gerrit-int.chromium.org/#change,15078

[Adam Barth]

Assigned for investigation.

[Adam Barth]

Looks like this covers these functions from WebView.h:

virtual void undoScrollFocusedNode() = 0;
virtual void zoomToFindInPageRect(const WebRect&) = 0;

[Adam Barth]

This is going to require a bit of cleanup downstream first.

[Adam Barth]

zoomToFindInPageRect is in Bug 91801.  Looking at undoScrollFocusedNode now.

[Adam Barth]

undoScrollFocusedNode depends on 757bb01d84e3817ec56f1d2945744245d1a43962 to make any sense.

[Adam Barth]

Created attachment 153608 [details]
work in progress

[Adam Barth]

This patch as a simplified version of scrollFocusedNodeIntoRect.  We can upstream the full version in a future patch.

@aelias: Would you be willing to take a look at this patch?  In particular, I'm not super confident in my simplification of scrollFocusedNodeIntoRect.  Thanks!

[Alexandre Elias]

Sorry, why do we want to upstream a simplified version?  Every line in there is there for a reason.  If it's blocked on some dependencies, we should upstream those dependencies first.

[Adam Barth]

I'm just trying to break things down in to smaller pieces.  That way we can iterate towards success instead of having to do things in large chunks.

In this particular case, the milestone we're aiming for is API compatibility.  In some sense, we could upstream stub implementations of these functions, but this patch takes a slightly bigger chunk than that.  :)

[Alexandre Elias]

I understand API compatibility is a useful goal.  In that case, I would actually prefer a pure stub implementation.  Then there is no expectation that it works at all, and it's not confusing during merges.

[Adam Barth]

Ok.  Thanks!

[Adam Barth]

Created attachment 153616 [details]
Patch

[Adam Barth]

Note: The implementation of undoScrollFocusedNode in this patch is complete.  It just doesn't do much without the changes to scrollFocusedNodeIntoRect.

[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.

[Alexandre Elias]

LGTM, thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 153616 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:201
> +static const int minReadableCaretHeight = 18;

what is this line for?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2464
> +void WebViewImpl::undoScrollFocusedNode()

I wish we had a term that embodied "scrolling and scaling" the viewport to
bring an element into view.  Maybe we could just refer to restoring the
viewport or something like that.  It might be easier to think about what
the transition into the scrolled and scaled state is called.

> Source/WebKit/chromium/src/WebViewImpl.h:815
> +    WebPoint m_undoScrollFocusedNodeScrollOffset;

how do these fields get set?

[Adam Barth]

(In reply to comment #16)
> (From update of attachment 153616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153616&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:201
> > +static const int minReadableCaretHeight = 18;
> 
> what is this line for?

A previous iteration of the patch.  Removed.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2464
> > +void WebViewImpl::undoScrollFocusedNode()
> 
> I wish we had a term that embodied "scrolling and scaling" the viewport to
> bring an element into view.  Maybe we could just refer to restoring the
> viewport or something like that.  It might be easier to think about what
> the transition into the scrolled and scaled state is called.

I'm open to naming suggestions.

> > Source/WebKit/chromium/src/WebViewImpl.h:815
> > +    WebPoint m_undoScrollFocusedNodeScrollOffset;
> 
> how do these fields get set?

They get set by the version of scrollFocusedNodeIntoRect on the chromium-android branch that zooms in on the focused node.  An earlier version of this patch contained a simplified version of this function that set these variables but aelias asked that we not upstream a simplified version of that function.

Another alternative is to upstream all the changes to scrollFocusedNodeIntoRect in this patch.  The changes are just a bit hairy.  :)

[Adam Barth]

Created attachment 153618 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #17)
> They get set by the version of scrollFocusedNodeIntoRect on the chromium-android branch that zooms in on the focused node.  An earlier version of this patch contained a simplified version of this function that set these variables but aelias asked that we not upstream a simplified version of that function.

I see, so we could go with:

   scrollFocusedNodeIntoRect
   undoScrollFocusedNodeIntoRect

The two functions would then be very clearly paired.

Adding "scale":

   scrollAndScaleFocusedNodeIntoRect
   undoScrollAndScaleFocusedNodeIntoRect

More of a mouthful:

   scrollAndScaleViewportToBringFocusedNodeIntoRect
   undoScrollAndScaleShootMeNow

Other ideas:

   adjustViewportTo{Highlight,Feature}FocusedNode
   restoreViewport

[Adam Barth]

> I see, so we could go with:
> 
>    scrollFocusedNodeIntoRect
>    undoScrollFocusedNodeIntoRect
> 
> The two functions would then be very clearly paired.

Yes.  It's a bit unclear from those names what happens if the user scrolls/zooms in between.  Perhaps we should make the embedder manage the state explicitly?

pushScrollAndScaleState();
scrollFocusedNodeIntoRect(...);
...
popScrollAndScaleState();

>    adjustViewportTo{Highlight,Feature}FocusedNode
>    restoreViewport

Highlight isn't the best term to use because there's another feature that draws a highlight around nodes (e.g., as in Bug 84936).

[Adam Barth]

It occurs to me that push/pop could leak, maybe we should have a save/restore interface that only keeps one history entry:

saveScrollAndScaleState();
scrollFocusedNodeIntoRect(...);
...
restoreScrollAndScaleState();

[Alexandre Elias]

The use case for the undo is for when a user accidentally taps a form field, then presses the hardware back button to dismiss the on-screen-keyboard, we wanted to also return to the previous page scale level.  I think save/restore makes a lot of sense as we don't require a stack.

The term that we used in the past in Android to refer to a page scale action is "magnify".  Essentially a synonym for "zoom" but not as overloaded :).  I don't know if it's clear enough in a codebase that doesn't commonly use the word, though.  "scrollAndScale" might be the best option.

[Darin Fisher (:fishd, Google)]

save/restoreScrollAndScaleState SGTM

[Darin Fisher (:fishd, Google)]

may as well also be verbose with: scrollAndScaleFocusedNodeIntoRect

[Adam Barth]

Comment on attachment 153618 [details]
Patch

Will do.

[Adam Barth]

Created attachment 153857 [details]
Patch

[Adam Barth]

Any advice on writing a test is appreciated.

[Alexandre Elias]

There's helpers for creating WebViewImpl inside the WebFrameTest.cpp, that should work well to test this.

[Adam Barth]

(In reply to comment #28)
> There's helpers for creating WebViewImpl inside the WebFrameTest.cpp, that should work well to test this.

Creating the WebViewImpl isn't the issue.  It's the fact that startPageScaleAnimation is asynchronous.

[Alexandre Elias]

In the downstream version of startPageScaleAnimation(), the method is synchronous   if durationSec == 0.  One option would be to upstream that and have animateScrollFocusedNodeSec parametrizable by test.  This approach could also be handy for adding more tests to double-tap zoom.

[Adam Barth]

Ok.  I'm happy to up the priority of upstreaming that change and to write a test for this patch once that lands.

[Alexandre Elias]

Yusuf is working on it in https://bugs.webkit.org/show_bug.cgi?id=90316 .  He told me he should be have time to restart work on it within a week or two.

[Darin Fisher (:fishd, Google)]

Comment on attachment 153857 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:201
> +static const double scrollFocusedNodeAnimationDurationInSeconds = 0.2;

nit: since this is used with restoreScrollAndScaleState, it may be used in cases
that don't involve the focused node.  maybe call it scrollAndScaleAnimationDurationInSeconds?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2730
> +#if ENABLE(GESTURE_EVENTS)

nit: not sure why this is protected by GESTURE_EVENTS feature guard, but that's ok.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2743
> +    m_savedScrollOffset = WebSize();

... = IntSize();

> Source/WebKit/chromium/src/WebViewImpl.h:724
> +    WebSize m_savedScrollOffset;

nit: use IntSize instead.  better to use WebCore types in the implementation.

[Adam Barth]

Comment on attachment 153857 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2730
>> +#if ENABLE(GESTURE_EVENTS)
> 
> nit: not sure why this is protected by GESTURE_EVENTS feature guard, but that's ok.

startPageScaleAnimation only exists when ENABLE(GESTURE_EVENTS) is true.  The #else block gives the non-animated implementation.

[Adam Barth]

(In reply to comment #33)
> (From update of attachment 153857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153857&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:201
> > +static const double scrollFocusedNodeAnimationDurationInSeconds = 0.2;
> 
> nit: since this is used with restoreScrollAndScaleState, it may be used in cases
> that don't involve the focused node.  maybe call it scrollAndScaleAnimationDurationInSeconds?

Done.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2743
> > +    m_savedScrollOffset = WebSize();
> 
> ... = IntSize();

Fixed.

> > Source/WebKit/chromium/src/WebViewImpl.h:724
> > +    WebSize m_savedScrollOffset;
> 
> nit: use IntSize instead.  better to use WebCore types in the implementation.

Thanks.  I missed the implicit conversion from WebSize to IntSize.  Using IntSize makes all this code much prettier.

[Adam Barth]

Created attachment 154172 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154172 [details]
Patch for landing

Clearing flags on attachment: 154172

Committed r123554: <http://trac.webkit.org/changeset/123554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.