Bug 180789 - Support Autoscrolling in contenteditable for WK2
Summary: Support Autoscrolling in contenteditable for WK2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
: 148061 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-12-13 18:07 PST by Megan Gardner
Modified: 2019-12-05 01:55 PST (History)
6 users (show)

See Also:


Attachments
Patch (21.61 KB, patch)
2017-12-15 14:01 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (21.67 KB, patch)
2017-12-15 15:33 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2017-12-15 16:49 PST, Megan Gardner
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2017-12-13 18:07:03 PST
Support Autoscrolling in contenteditable for WK2
Comment 1 Megan Gardner 2017-12-15 14:01:57 PST
Created attachment 329517 [details]
Patch
Comment 2 Megan Gardner 2017-12-15 14:02:48 PST
<rdar://problem/19005092>
Comment 3 Megan Gardner 2017-12-15 15:33:47 PST
Created attachment 329527 [details]
Patch
Comment 4 Simon Fraser (smfr) 2017-12-15 15:45:35 PST
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.
Comment 5 Megan Gardner 2017-12-15 16:49:28 PST
Created attachment 329542 [details]
Patch
Comment 6 Wenson Hsieh 2017-12-15 17:47:25 PST
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?
Comment 7 Simon Fraser (smfr) 2017-12-15 18:19:38 PST
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
Comment 8 Megan Gardner 2017-12-18 11:45:15 PST
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.
Comment 9 Megan Gardner 2017-12-18 12:14:40 PST
https://trac.webkit.org/r226067
Comment 10 Radar WebKit Bug Importer 2017-12-18 12:15:31 PST
<rdar://problem/36112755>
Comment 11 Megan Gardner 2018-02-19 11:59:12 PST
*** Bug 148061 has been marked as a duplicate of this bug. ***
Comment 12 Tim Horton 2019-12-05 01:55:32 PST
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