Bug 195278 - Add testing API to hit-test and scroll overflow scrollers
Summary: Add testing API to hit-test and scroll overflow scrollers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-04 08:11 PST by Antti Koivisto
Modified: 2019-03-11 22:00 PDT (History)
10 users (show)

See Also:


Attachments
patch (18.49 KB, patch)
2019-03-04 08:44 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (10.80 KB, patch)
2019-03-11 10:55 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2019-03-04 08:11:03 PST
Existing tests use UI scripting, which is clumsy and slow.
Comment 1 Antti Koivisto 2019-03-04 08:44:42 PST
Created attachment 363516 [details]
patch
Comment 2 Frédéric Wang (:fredw) 2019-03-05 06:09:04 PST
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)?
Comment 3 Simon Fraser (smfr) 2019-03-05 09:30:10 PST
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).
Comment 4 Antti Koivisto 2019-03-05 11:58:09 PST
> slow!?

Yes, because swipe based scrolling needs long wait times stabilize. See the existing tests.
Comment 5 Antti Koivisto 2019-03-05 11:58:18 PST
*to stabilize
Comment 6 Antti Koivisto 2019-03-05 12:05:24 PST
> 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).
Comment 7 Simon Fraser (smfr) 2019-03-05 12:51:53 PST
(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"
Comment 8 Simon Fraser (smfr) 2019-03-05 12:52:50 PST
UIScriptController already has immediateScrollToOffset(). It just needs a Frame or Document parameter, and some "don't sync" smarts.
Comment 9 Frédéric Wang (:fredw) 2019-03-05 13:00:12 PST
(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?
Comment 10 Antti Koivisto 2019-03-05 13:08:56 PST
(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?).
Comment 11 Simon Fraser (smfr) 2019-03-05 13:44:55 PST
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.
Comment 12 Antti Koivisto 2019-03-05 14:36:51 PST
(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.
Comment 13 Simon Fraser (smfr) 2019-03-11 10:55:42 PDT
Created attachment 364271 [details]
Patch
Comment 14 WebKit Commit Bot 2019-03-11 21:59:42 PDT
Comment on attachment 364271 [details]
Patch

Clearing flags on attachment: 364271

Committed r242773: <https://trac.webkit.org/changeset/242773>
Comment 15 WebKit Commit Bot 2019-03-11 21:59:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-03-11 22:00:26 PDT
<rdar://problem/48797504>