Bug 209134 – [macOS] Add support for scrollend event of programmatic scroll

• Component: Scrolling
• Status: RESOLVED FIXED

[cathiechen]

The programmatic scroll would fire scrollend events.

[cathiechen]

Created attachment 393744 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 393745 [details]
scrollend-programmatic-for-review

[cathiechen]

Created attachment 393746 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 393747 [details]
scrollend-programmatic-for-ews

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

Comment on attachment 393745 [details]
scrollend-programmatic-for-review

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

> Source/WebCore/dom/Document.cpp:804
> +    m_pendingScrollendEventTargetList = nullptr;

Should everything in this patch be "ScrollEnd"? Did you check what we do for other events?

> Source/WebCore/page/FrameView.cpp:2286
> +void FrameView::setScrollPosition(const ScrollPosition& scrollPosition, ScrollClamping clamping, bool animated, bool saveScrollendPosition)

I believe Simon complained in the past about "boolean trap". I wonder whether we want enums here?

> Source/WebCore/platform/ScrollView.cpp:543
> +    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation && newScrollPosition == this->scrollPosition()) {

this line is very long, maybe consider https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op ?

> Source/WebCore/platform/ScrollView.cpp:-534
> -        scrollAnimator().cancelAnimations();

Can you explain this behavior change? It seems you move it above where the if is no longer done.

> Source/WebCore/rendering/RenderLayer.cpp:2848
> +    bool autoscrollNotInProgress = !renderer().frame().eventHandler().autoscrollInProgress();

It seems the autscrollstatus change can be done in a separate preliminary patch?

[cathiechen]

Created attachment 395799 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 395800 [details]
scrollend-programmatic-for-review

[cathiechen]

Created attachment 395801 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 395802 [details]
scrollend-programmatic-for-review

[cathiechen]

Created attachment 395805 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 395806 [details]
scrollend-programmatic-for-review

[cathiechen]

Hi Fred,
Thanks for the review:)

> 
> > Source/WebCore/dom/Document.cpp:804
> > +    m_pendingScrollendEventTargetList = nullptr;
> 
> Should everything in this patch be "ScrollEnd"? Did you check what we do for
> other events?
> 

Done.

> > Source/WebCore/page/FrameView.cpp:2286
> > +void FrameView::setScrollPosition(const ScrollPosition& scrollPosition, ScrollClamping clamping, bool animated, bool saveScrollendPosition)
> 
> I believe Simon complained in the past about "boolean trap". I wonder
> whether we want enums here?
> 

Done.

> > Source/WebCore/platform/ScrollView.cpp:543
> > +    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && currentScrollBehaviorStatus() == ScrollBehaviorStatus::NotInAnimation && newScrollPosition == this->scrollPosition()) {
> 
> this line is very long, maybe consider
> https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op ?
> 

Done.

> > Source/WebCore/platform/ScrollView.cpp:-534
> > -        scrollAnimator().cancelAnimations();
> 
> Can you explain this behavior change? It seems you move it above where the
> if is no longer done.
> 

We should cancel the animation before return. There's a tiny chance that animation doesn't stop. Moved this change to https://bugs.webkit.org/show_bug.cgi?id=210171.

> > Source/WebCore/rendering/RenderLayer.cpp:2848
> > +    bool autoscrollNotInProgress = !renderer().frame().eventHandler().autoscrollInProgress();
> 
> It seems the autscrollstatus change can be done in a separate preliminary
> patch?

Done, I move this change to https://bugs.webkit.org/show_bug.cgi?id=210171

[cathiechen]

Created attachment 395939 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 395940 [details]
scrollend-programmatic-for-review

[cathiechen]

Created attachment 395943 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 395944 [details]
scrollend-programmatic-for-review

[cathiechen]

Comment on attachment 395944 [details]
scrollend-programmatic-for-review

Hi,
The patch is ready to review\o/ Thanks:)
This is based on the parsing patch(208802) and code style fix up for scroll animated patch(bug 210171).

[cathiechen]

Created attachment 396058 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 396059 [details]
scrollend-programmatic-for-review

[cathiechen]

Created attachment 396062 [details]
scrollend-programmatic-for-review

[cathiechen]

Comment on attachment 396062 [details]
scrollend-programmatic-for-review

Hi,
Rebase the code.
After commit the code style fix up patch and test import patch, now this patch is only based on the parsing patch(bug 208802). I wonder if I can commit the parsing patch now?

[cathiechen]

Created attachment 399465 [details]
scrollend-programmatic-for-ews

[cathiechen]

Created attachment 399466 [details]
scrollend-programmatic-for-review

Chrome has shipped the new "scrollend" event as an experimental feature. Are there plans to enable this feature to Safari as well?

[Simon Fraser (smfr)]

Is there a spec for this new event?

(In reply to Simon Fraser (smfr) from comment #25)
> Is there a spec for this new event?

Yes, here you are: https://wicg.github.io/overscroll-scrollend-events/
Chrome has shipped it behind experimental flag (for now, I think they only implemented the scrollend event).

[Šime Vidas]

The scrollend event shipped in Chrome 114 last month.

Simple test page (the page will turn yellow after the scroll): https://output.jsbin.com/ludiban/quiet

The event seems to now be defined in the CSSOM View spec: https://drafts.csswg.org/cssom-view/#eventdef-document-scrollend

[Ernst]

Firefox also shipped 'scrollend'. Please also enable it in Safari on both iOS and macOS. Would make some components much easier than with debouncing and heuristics to detect scroll end. Thanks.

[Rado]

Please implement the 'scrollend' event, because we must use a polyfill for Safari only. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/156077004>

[Nikos Mouchtaris]

Pull request: https://github.com/WebKit/WebKit/pull/48213

[EWS]

Committed 297630@main (cf4ff1c081a2): <https://commits.webkit.org/297630@main>

Reviewed commits have been landed. Closing PR #48213 and removing active labels.