Bug 195278 – Add testing API to hit-test and scroll overflow scrollers

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Antti Koivisto]

Existing tests use UI scripting, which is clumsy and slow.

[Antti Koivisto]

Created attachment 363516 [details]
patch

[Frédéric Wang (:fredw)]

Comment on attachment 363516 [details]
patch

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

This looks good to me and would definitely allow to write more reliable tests. The only thing I wonder is whether new code should be behind a compilation flag so that it is only enabled for developer builds?

> Source/WebCore/testing/Internals.cpp:1713
> +    auto& scrollingCoordinator = downcast<AsyncScrollingCoordinator>(*document->page()->scrollingCoordinator());

It seems that the Windows bots don't like this. No idea why... Maybe you need to protect some stuff with ENABLE(ASYNC_SCROLLING)?

[Simon Fraser (smfr)]

Comment on attachment 363516 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        It would be useful to be able to test delegated user scrolling without using clumsy and slow UI scripting.

slow!?

> Source/WebCore/ChangeLog:14
> +        This mechanism currently supports subframes and overflow scrolling on iOS.

Will it support main frame scrolling, and macOS too?

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:221
> +void ScrollingStateScrollingNode::setRequestedDelegatedScrollPositionForTesting(const FloatPoint& requestedDelegatedScrollPositionForTesting)
> +{
> +    m_requestedDelegatedScrollPositionForTesting = requestedDelegatedScrollPositionForTesting;
> +    setPropertyChanged(RequestedDelegatedScrollPositionForTesting);
> +}

I don't think doing this via a scrolling tree commit is the right approach. This will trigger a commit of any dirty state in the scrolling state tree, which may mask bugs.

I think we want to simulate a user scroll more directly. On iOS, this should actually scroll the UIScrollVIew, which would trigger the wasScrolledByDelegatedScrolling() code path. On macOS, it would enter the code path that is called form handling wheel events. I think we also want this test mode to disable messaging back to the main thread/UI process.

So I think we do need UIScriptController on iOS (and maybe macOS too), in conjunction with something that cuts of the dispatch in ThreadedScrollingTree::scrollingTreeNodeDidScroll() (macOS) and calling the scrolling coordinator in RemoteScrollingTree::scrollingTreeNodeDidScroll (iOS).

[Antti Koivisto]

> slow!?

Yes, because swipe based scrolling needs long wait times stabilize. See the existing tests.

[Antti Koivisto]

*to stabilize

[Antti Koivisto]

> I think we want to simulate a user scroll more directly. On iOS, this should
> actually scroll the UIScrollVIew, which would trigger the
> wasScrolledByDelegatedScrolling() code path. On macOS, it would enter the
> code path that is called form handling wheel events.

This patch does scroll UIScrollView on iOS triggering wasScrolledByDelegatedScrolling.

Mac support would be a separate patch.

> I think we also want
> this test mode to disable messaging back to the main thread/UI process.

Separate API and a separate patch.

> So I think we do need UIScriptController on iOS (and maybe macOS too), in
> conjunction with something that cuts of the dispatch in
> ThreadedScrollingTree::scrollingTreeNodeDidScroll() (macOS) and calling the
> scrolling coordinator in RemoteScrollingTree::scrollingTreeNodeDidScroll
> (iOS).

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #4)
> > slow!?
> 
> Yes, because swipe based scrolling needs long wait times stabilize. See the
> existing tests.

It could be an instantaneous "scroll to"

[Simon Fraser (smfr)]

UIScriptController already has immediateScrollToOffset(). It just needs a Frame or Document parameter, and some "don't sync" smarts.

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #8)
> UIScriptController already has immediateScrollToOffset(). It just needs a
> Frame or Document parameter, and some "don't sync" smarts.

Interesting. Does it work for overflow scroll? Does it allow to test UI => Web process sync?

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #8)
> UIScriptController already has immediateScrollToOffset(). It just needs a
> Frame or Document parameter, and some "don't sync" smarts.

Any UIScriptController based API should probably take a point instead of Frame or Document so it also covers hit testing (can Frames and Documents even be reasonably passed to UIScriptController?).

[Simon Fraser (smfr)]

I'd rather the hit testing be a separate entry point. It's much easier to write a test that says "scroll element E" rathe than figuring out coords for it.

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #11)
> I'd rather the hit testing be a separate entry point. It's much easier to
> write a test that says "scroll element E" rathe than figuring out coords for
> it.

Sure, thus this patch. 

It is unclear to me how to make DOM objects like Element available to UIScriptController. There are no existing examples. Since it is your baby I think you'll get the honours of implementing this.

[Simon Fraser (smfr)]

Created attachment 364271 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364271 [details]
Patch

Clearing flags on attachment: 364271

Committed r242773: <https://trac.webkit.org/changeset/242773>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48797504>