Bug 226453 - add Internals method to expose synchronous scrolling reasons for individual scrolling tree scrolling nodes
Summary: add Internals method to expose synchronous scrolling reasons for individual s...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords:
Depends on:
Blocks: 226399
  Show dependency treegraph
 
Reported: 2021-05-30 22:17 PDT by Cameron McCormack (:heycam)
Modified: 2021-06-01 15:22 PDT (History)
13 users (show)

See Also:


Attachments
Patch (11.78 KB, patch)
2021-05-30 22:20 PDT, Cameron McCormack (:heycam)
fred.wang: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-05-30 22:17:58 PDT
So we can test the synchronous scrolling reasons are set correctly on scrolling tree nodes for overflow:scroll elements.
Comment 1 Cameron McCormack (:heycam) 2021-05-30 22:20:45 PDT
Created attachment 430174 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2021-06-01 06:26:15 PDT
Comment on attachment 430174 [details]
Patch

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

> Source/WebCore/testing/Internals.h:441
> +    ExceptionOr<String> synchronousScrollingReasonsForScrollableElement(Element&) const;

Is it possible to make this a const Element&?
Comment 3 Simon Fraser (smfr) 2021-06-01 10:20:38 PDT
Doesn't dumping the scrolling state tree suffice?
Comment 4 Cameron McCormack (:heycam) 2021-06-01 13:59:07 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Doesn't dumping the scrolling state tree suffice?

As I discovered from looking at other tests, yes it does.  But it includes a lot of other irrelevant information for my tests.  Seems like it's better to avoid having to update test expectations for changes in other parts of the scrolling state tree, but if you think it's better to just use the existing thing and avoid adding something new, I can do that.
Comment 5 Darin Adler 2021-06-01 14:31:09 PDT
Comment on attachment 430174 [details]
Patch

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

>> Source/WebCore/testing/Internals.h:441
>> +    ExceptionOr<String> synchronousScrollingReasonsForScrollableElement(Element&) const;
> 
> Is it possible to make this a const Element&?

Why does that matter? This is designed to be called from JavaScript.
Comment 6 Cameron McCormack (:heycam) 2021-06-01 15:22:19 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Doesn't dumping the scrolling state tree suffice?

I've updated my tests in bug 226399 to use the scrolling state tree dump so I'm not going to use this patch.