Bug 211342 - Make it possible to test overlay scrollbar interactions
Summary: Make it possible to test overlay scrollbar interactions
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar
: 199323 (view as bug list)
Depends on:
Reported: 2020-05-02 12:30 PDT by Simon Fraser (smfr)
Modified: 2020-05-08 16:06 PDT (History)
5 users (show)

See Also:

Patch (29.75 KB, patch)
2020-05-02 12:33 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (29.84 KB, patch)
2020-05-02 13:40 PDT, Simon Fraser (smfr)
dbates: review+
Details | Formatted Diff | Diff
Patch (29.92 KB, patch)
2020-05-02 16:20 PDT, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-05-02 12:30:21 PDT
Make it possible to test overlay scrollbar interactions
Comment 1 Simon Fraser (smfr) 2020-05-02 12:33:17 PDT
Created attachment 398292 [details]
Comment 2 Daniel Bates 2020-05-02 13:39:54 PDT
Comment on attachment 398292 [details]

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

Patch looks good. Better patch would use js-test.js instead of js-test-{pre, post} in tests.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:893
> +        result.append(",expanded"_s);

OK as-is. No change needed. Optimal solution would have space after ',' to make results human pretty.

> Source/WebCore/testing/Internals.cpp:2756
> +ExceptionOr<ScrollableArea*> Internals::scrollableAreaForNode(RefPtr<Node>& node) const

This is ok-is. No change needed. The current signature is optimal, but need to amend to take out a Ref<Node> INSIDE the function before the updateLayout. Alternative optimal solution is take  param by CONST lvalue reference + use a local for the nullptr case.

> Source/WebCore/testing/Internals.cpp:2767
>      if (is<Document>(*node)) {

OK as-is. No change needed. If do ^^^ then optimal solution omits the * here.

> Source/WebCore/testing/Internals.cpp:2773
>      } else if (is<Element>(*node)) {


> LayoutTests/fast/scrolling/mac/scrollbars/overlay-scrollbar-hovered.html:28
> +            debug('Hovering vertical scrollbar should show expanded scrollbar');

OK as-is. No change needed. Slightly better solution is to prefix string with <br> because results will be prettier to read. Same thing can be down throughout this patch.
Comment 3 Simon Fraser (smfr) 2020-05-02 13:40:01 PDT
Created attachment 398296 [details]
Comment 4 Daniel Bates 2020-05-02 14:20:03 PDT
Comment on attachment 398296 [details]

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

> Source/WebCore/testing/Internals.cpp:2756
> +ExceptionOr<ScrollableArea*> Internals::scrollableAreaForNode(RefPtr<Node>& node) const

OK as-is. No change needed. This change makes node an out-argument. Is this OK? Optimal solutions in previous comment.
Comment 5 Simon Fraser (smfr) 2020-05-02 16:20:53 PDT
Created attachment 398297 [details]
Comment 6 EWS 2020-05-02 20:30:01 PDT
Dan Bates found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 7 Simon Fraser (smfr) 2020-05-02 20:49:40 PDT
Comment 8 Radar WebKit Bug Importer 2020-05-02 20:50:21 PDT
Comment 9 Simon Fraser (smfr) 2020-05-08 16:06:45 PDT
*** Bug 199323 has been marked as a duplicate of this bug. ***