Bug 180789 – Support Autoscrolling in contenteditable for WK2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Support Autoscrolling in contenteditable for WK2

[Megan Gardner]

Created attachment 329517 [details]
Patch

[Megan Gardner]

<rdar://problem/19005092>

[Megan Gardner]

Created attachment 329527 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 329527 [details]
Patch

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

Looks good but r- for cleanup and test improvement.

> Source/WebCore/page/EventHandler.h:341
> +    IntPoint m_targetAutoscrollPosition;

In what coordinate system relative to which element?

> Source/WebCore/page/EventHandler.h:342
> +    bool m_isAutoscrollActive { false };

"active" is ambiguous; it could be "is able to autoscroll" or "is autoscrolling now". I would call this m_isAutoscrolling.

Or it seems to correspond to the state of the autoscroll timer. Maybe you don't need this.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:695
> +- (void)cancelAutoscroll

blank line between methods please.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1389
> +        m_page->mainFrame().eventHandler().startTextAutoscroll(m_assistedNode->renderer(), position);

What if m_assistedNode->renderer() is null?

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:23
> +            var tapPointX = targetRect.x+targetRect.width / 2;
> +            var tapPointY = targetRect.y+targetRect.height / 2;

Spaces around +

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:30
> +                    setTimeout(function(){ // wait a spell while the keyboard comes up

We have functions to detect when the keyboard animation is finished.

[Megan Gardner]

Created attachment 329542 [details]
Patch

[Wenson Hsieh]

Comment on attachment 329542 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:690
> +// @protocol UITextAutoscrolling

Nit - we should usually indicate this using `#pragma mark - UITextAutoscrolling`

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:703
> +    // used to scroll selection on keyboard up; we already scroll to visible

Nit - inline comments like these should begin with a capital letter and end with a period.

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:22
> +            var tapPointX = targetRect.x+targetRect.width / 2;

Nit - spaces around the +

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:23
> +            var tapPointY = targetRect.y+targetRect.height / 2;

Ditto.

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:34
> +                                    testRunner.runUIScript(continueTouchAndDragFromPointToPoint(dragX, tapPointY, dragX+5, tapPointY), function() {

Ditto (spaces around +)

> LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:35
> +                                        if (scrollBox.scrollLeft > 0 )

Nit - extra space after the 0.

> LayoutTests/fast/events/touch/ios/resources/basic-gestures.js:173
> +        uiController.uiScriptComplete();

Nit - looks like there's an extra indent here?

[Simon Fraser (smfr)]

Comment on attachment 329542 [details]
Patch

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

> Source/WebCore/page/EventHandler.h:339
> +    WEBCORE_EXPORT void startTextAutoscroll(RenderObject* renderer, const FloatPoint& position);

position -> positionInWindow

> Source/WebCore/page/EventHandler.h:342
> +    bool m_isAutoscrolling { false };

My comment about not needing this and using the timer state still stands.

> Source/WebCore/page/ios/EventHandlerIOS.mm:563
> +void EventHandler::startTextAutoscroll(RenderObject* renderer, const FloatPoint& position)

position -> positionInWindow

> Source/WebKit/UIProcess/WebPageProxy.h:573
> +    void startAutoscrollAtPosition(const WebCore::FloatPoint&);

Name the parameter positionInWindow

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:721
> +void WebPageProxy::startAutoscrollAtPosition(const WebCore::FloatPoint& position)

Name the parameter positionInWindow

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:102
> +    StartAutoscrollAtPosition(WebCore::FloatPoint position)

Name the parameter positionInWindow

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1386
> +void WebPage::startAutoscrollAtPosition(const WebCore::FloatPoint& position)

Name the parameter positionInWindow

[Megan Gardner]

The m_isAutoscrolling is used in a call that the timer uses to determine if it should still be updating, so using the timer state instead would be problematic.

[Megan Gardner]

https://trac.webkit.org/r226067

[Radar WebKit Bug Importer]

<rdar://problem/36112755>

[Megan Gardner]

*** Bug 148061 has been marked as a duplicate of this bug. ***

[Tim Horton]

Comment on attachment 329542 [details]
Patch

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

>> Source/WebCore/page/EventHandler.h:342
>> +    bool m_isAutoscrolling { false };
> 
> My comment about not needing this and using the timer state still stands.

... also, these are in the public section :) they should be down with the other members