Bug 204882 – Add support for scroll behavior relies on ScrollAnimation of the Web process

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[cathiechen]

This is the first support part of bug 188043.
Deal with scroll-behavior CSS property and ScrollOptions parsing and add a run time flag for scroll-behavior.
This will be followed by two other support parts:
  - Support smooth scrolling by animation timer in the Web process.
  - Support iOS smooth scrolling by using native interfaces in the UI process.

[cathiechen]

Created attachment 384896 [details]
Patch

[cathiechen]

Created attachment 384898 [details]
Patch

[cathiechen]

Update:
This bug will present the implement based on scroll animation in the Web process.

Sorry for the confusing description.

[cathiechen]

Created attachment 384908 [details]
Patch

[cathiechen]

Created attachment 385152 [details]
scroll-animation-in-web-process-for-review

This patch only contains the change of scroll animation in web process.

[cathiechen]

Created attachment 385153 [details]
Patch

[cathiechen]

Created attachment 385154 [details]
scroll-animation-in-web-process-for-review

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

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

> LayoutTests/platform/ios/TestExpectations:3468
> +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ]

I'm not sure what's the diff here.

> Source/WebCore/ChangeLog:164
> +        (WebCore::useSmoothScrolling):

These changes in the Changelog seem weird

> Source/WebCore/dom/Element.cpp:1307
> +        renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

Do you need explicit ScrollClamping::Clamped here?

> Source/WebCore/dom/Element.cpp:1330
> +        renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1636
> +    if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))

I would add a C++ comment here to explain the optimization and why it is skipped, even if it's in the changelog.

> Source/WebCore/platform/ScrollTypes.h:59
> +    WebAnimationTimerStarted,

Can we rename it WebProcessScrollAnimationTimerStarted? Or maybe NonNativeAnimationTimerStarted. Not sure what's best.

> Source/WebCore/platform/ScrollableArea.h:66
> +    bool isWebScrollAnimationInProgress();

WebProcess to?

> Source/WebKit/ChangeLog:12
> +        Add CSSOM smooth scrolling as an experimental feature.

this part is weird

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

Comment on attachment 385154 [details]
scroll-animation-in-web-process-for-review

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

See my comments on bug 188043

> LayoutTests/platform/ios/TestExpectations:3468
> +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ]

this change is weird

> Source/WebCore/dom/Element.cpp:1307
> +        renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

do you need to be explicit about ScrollClamping::Clamped

> Source/WebCore/dom/Element.cpp:1330
> +        renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped);

ditto

> Source/WebCore/page/DOMWindow.cpp:1636
> +    if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))

maybe we should explain this with a comment

> Source/WebCore/page/DOMWindow.cpp:1644
> +    // See https://github.com/w3c/csswg-drafts/issues/2977

maybe a separate webkit bug

> Source/WebCore/platform/ScrollTypes.h:59
> +    WebAnimationTimerStarted,

WebProcessAnimationTimerStarted?

or even NonNative?

> Source/WebCore/platform/ScrollableArea.h:66
> +    bool isWebScrollAnimationInProgress();

This one should be renamed too I guess.

[cathiechen]

Comment on attachment 385154 [details]
scroll-animation-in-web-process-for-review

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

Hi Fred,

Thanks for the review:)

>> LayoutTests/platform/ios/TestExpectations:3468
>> +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ]
> 
> this change is weird

Done

>> Source/WebCore/dom/Element.cpp:1307
>> +        renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped);
> 
> do you need to be explicit about ScrollClamping::Clamped

Done

>> Source/WebCore/dom/Element.cpp:1330
>> +        renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped);
> 
> ditto

Done

>> Source/WebCore/page/DOMWindow.cpp:1636
>> +    if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0))
> 
> maybe we should explain this with a comment

Done.

>> Source/WebCore/page/DOMWindow.cpp:1644
>> +    // See https://github.com/w3c/csswg-drafts/issues/2977
> 
> maybe a separate webkit bug

Done.

>> Source/WebCore/platform/ScrollTypes.h:59
>> +    WebAnimationTimerStarted,
> 
> WebProcessAnimationTimerStarted?
> 
> or even NonNative?

Change to NonNativeAnimationTimerStarted

>> Source/WebCore/platform/ScrollableArea.h:66
>> +    bool isWebScrollAnimationInProgress();
> 
> This one should be renamed too I guess.

Done, change to isNonNativeAnimationInProgress

[cathiechen]

Created attachment 385255 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385256 [details]
Non-native-animation-for-review

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

Comment on attachment 385256 [details]
Non-native-animation-for-review

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

LGTM in general. But again most of this is from me, we should find another reviewer...

> Source/WebCore/dom/Element.cpp:1305
> +        int effectiveLeft = clampToInteger(newLeft * renderer->style().effectiveZoom());

"effective" is maybe not the good name. clampedLeft instead?

> Source/WebCore/dom/Element.cpp:1328
> +        int effectiveTop = clampToInteger(newTop * renderer->style().effectiveZoom());

clampedTop

> Source/WebCore/page/DOMWindow.cpp:1636
> +    // The previous scroll animation should be stopped by this scroll, so it shouldn't be skipped.

Maybe "This is an optimization for the common case of scrolling to (0, 0) when the scroller is already at the origin. If an animated scroll is in progress, this optimization is skipped to ensure that the animated scroll is really stopped."

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:263
> +    // If web scroll animation is in progress, this is set by scroll animator, otherwise, the animation should stop first.

"If a non native animation"

That say, isNonNativeAnimationInProgress() is a bit confusing, it already sets the scroll behavior status to NotInAnimation when returning false, so it looks like the setScrollBehaviorStatus call is not necessary?

> Source/WebCore/platform/ScrollAnimator.cpp:75
> +    // FIXME (TODO): This should also take into account animations in derived classes.

can we fix this? or open a separate follow-up bug?

> Source/WebCore/platform/ScrollableArea.cpp:124
> +

I think currentBehaviorStatus() should be a private function updating the m_currentScrollBehaviorStatus(). Then you would define 

bool isScrollInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation; }
bool isNonNativeAnimationInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NonNativeAnimationTimerStarted; }

I think the need for an enum of scroll behavior status is not really clear without the iOS part, though. Maybe we'd want to introduce these latter and just use a boolean for now...

> Source/WebCore/platform/ScrollableArea.h:66
> +    bool isNonNativeAnimationInProgress();

private?

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

Comment on attachment 385256 [details]
Non-native-animation-for-review

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

> Source/WebCore/platform/ScrollTypes.h:57
> +enum class ScrollBehaviorStatus : uint8_t {

Maybe add a FIXME bug 204936 that explains we will extend this with a native scroll status?

[cathiechen]

Comment on attachment 385256 [details]
Non-native-animation-for-review

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

Hi Fred,
Thanks for the review:)

>> Source/WebCore/dom/Element.cpp:1305
>> +        int effectiveLeft = clampToInteger(newLeft * renderer->style().effectiveZoom());
> 
> "effective" is maybe not the good name. clampedLeft instead?

Done

>> Source/WebCore/dom/Element.cpp:1328
>> +        int effectiveTop = clampToInteger(newTop * renderer->style().effectiveZoom());
> 
> clampedTop

Done

>> Source/WebCore/page/DOMWindow.cpp:1636
>> +    // The previous scroll animation should be stopped by this scroll, so it shouldn't be skipped.
> 
> Maybe "This is an optimization for the common case of scrolling to (0, 0) when the scroller is already at the origin. If an animated scroll is in progress, this optimization is skipped to ensure that the animated scroll is really stopped."

Done. Thanks:)

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:263
>> +    // If web scroll animation is in progress, this is set by scroll animator, otherwise, the animation should stop first.
> 
> "If a non native animation"
> 
> That say, isNonNativeAnimationInProgress() is a bit confusing, it already sets the scroll behavior status to NotInAnimation when returning false, so it looks like the setScrollBehaviorStatus call is not necessary?

isNonNativeAnimationInProgress() will not change status.

>> Source/WebCore/platform/ScrollAnimator.cpp:75
>> +    // FIXME (TODO): This should also take into account animations in derived classes.
> 
> can we fix this? or open a separate follow-up bug?

Now that we are using the ScrollBehaviorStatus, it seems this is not needed. I'll remove ScrollAnimator::isScrollInProgress and related interfaces.

>> Source/WebCore/platform/ScrollTypes.h:57
>> +enum class ScrollBehaviorStatus : uint8_t {
> 
> Maybe add a FIXME bug 204936 that explains we will extend this with a native scroll status?

Done

>> Source/WebCore/platform/ScrollableArea.cpp:124
>> +
> 
> I think currentBehaviorStatus() should be a private function updating the m_currentScrollBehaviorStatus(). Then you would define 
> 
> bool isScrollInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation; }
> bool isNonNativeAnimationInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NonNativeAnimationTimerStarted; }
> 
> I think the need for an enum of scroll behavior status is not really clear without the iOS part, though. Maybe we'd want to introduce these latter and just use a boolean for now...

Done.

>> Source/WebCore/platform/ScrollableArea.h:66
>> +    bool isNonNativeAnimationInProgress();
> 
> private?

Currently, isScrollInProgress() == isNonNativeAnimationInProgress(), after add native animation status, they will be different.
isScrollInProgress() will include native and non-native scroll.
isNonNativeAnimationInProgress() is non-native scroll.
This is need by different scenarios, so it should be public.

[cathiechen]

Created attachment 385269 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385270 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385273 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385274 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385278 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385279 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385353 [details]
Non-native-animation-for-ews(including parsing)

Rebase code to fix the compile error.

[cathiechen]

Created attachment 385354 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385365 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385366 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385368 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385369 [details]
Non-native-animation-for-review

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

Comment on attachment 385369 [details]
Non-native-animation-for-review

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

OK LGTM. Again, it would be good to have advice from another reviewer, as I wrote part of this patch.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:266
> +

I think that part is a no-op until we add a third scrollbehaviorstatus. Should it be moved to the other patch?

[cathiechen]

Comment on attachment 385369 [details]
Non-native-animation-for-review

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

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:266
>> +
> 
> I think that part is a no-op until we add a third scrollbehaviorstatus. Should it be moved to the other patch?

Yes, indeed.

[cathiechen]

Created attachment 385370 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385371 [details]
Non-native-animation-for-review

[cathiechen]

Created attachment 385385 [details]
Non-native-animation-for-ews(including parsing)

[cathiechen]

Created attachment 385386 [details]
Non-native-animation-for-review

[cathiechen]

This patch has presented for a while. We plan to land it tomorrow.
Please leave a msg if there's any concern. Thanks:)

I'll rebase it after bug 205009 landing.

[cathiechen]

Created attachment 388135 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 388137 [details]
Patch

[cathiechen]

Created attachment 388147 [details]
Patch

[cathiechen]

Created attachment 388153 [details]
Patch

[cathiechen]

Created attachment 388168 [details]
Patch

[cathiechen]

Created attachment 388175 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388175 [details]
Patch

Clearing flags on attachment: 388175

Committed r254807: <https://trac.webkit.org/changeset/254807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58720725>

[Antti Koivisto]

Comment on attachment 388175 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:923
> -    scrollBy({ x, y });
> +    scrollBy(ScrollToOptions(x, y));

Why these changes?

[cathiechen]

(In reply to Antti Koivisto from comment #44)
> Comment on attachment 388175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388175&action=review
> 
> > Source/WebCore/dom/Element.cpp:923
> > -    scrollBy({ x, y });
> > +    scrollBy(ScrollToOptions(x, y));
> 
> Why these changes?

ScrollToOptions inherits ScrollOptions, this changes make ScrollToOptions carries a default value `ScrollBehavior::Auto`.
The changes about ScrollToOptions and ScrollOptions is from https://bugs.webkit.org/show_bug.cgi?id=205009

[Fujii Hironori]

Filed: Bug 206494 – ScrollableArea.cpp(63): error C2338: ScrollableArea_should_stay_small

[Fujii Hironori]

Committed r254839: <https://trac.webkit.org/changeset/254839>

[Fujii Hironori]

Broke Apple internal builds. Reverted in r254839.

[Fujii Hironori]

*** Bug 206494 has been marked as a duplicate of this bug. ***

[cathiechen]

Created attachment 388282 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388282 [details]
Patch

Clearing flags on attachment: 388282

Committed r254849: <https://trac.webkit.org/changeset/254849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke scrolling with Page Up/Page Down on macOS. I'll revert for now.

[Simon Fraser (smfr)]

It relanded in http://trac.webkit.org/changeset/254807

[Simon Fraser (smfr)]

Rollout failed. I'll try to debug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206559

[Jonathan Bedard]

This also breaks a test:

https://results.webkit.org/?suite=layout-tests&test=scrollingcoordinator%2Fmac%2Ffixed-scrolled-body.html

[Simon Fraser (smfr)]

Sorry, I had to roll out both patches to get it to roll out cleanly.

Please make sure macOS page up/down works before re-landing (and add a test!)

[Simon Fraser (smfr)]

A big problem with the scrolling code is that it's really hard to know what the "bottleneck" functions are. There are just so many things called "scrollTo" or "setScrollPosition".

That makes it hard know where to insert the "requestScrollPositionUpdate" code.

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #59)
> A big problem with the scrolling code is that it's really hard to know what
> the "bottleneck" functions are. There are just so many things called
> "scrollTo" or "setScrollPosition".
> 
> That makes it hard know where to insert the "requestScrollPositionUpdate"
> code.

Yes, indeed, sorry for the mistake, I'll take a look.

Here is the recursive codepath:
ScrollAnimator::notifyPositionChanged
ScrollableArea::setScrollOffsetFromAnimation
AsyncScrollingCoordinator::requestScrollPositionUpdate
AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll
ScrollableArea::scrollToOffsetWithoutAnimation
ScrollAnimator::scrollToOffsetWithoutAnimation
ScrollAnimator::notifyPositionChanged

So I moved "requestScrollPositionUpdate" out of setScrollOffsetFromAnimation.
It seems I only insert it partly. I'll try to find another way to fix this.

[cathiechen]

Created attachment 389500 [details]
Patch

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

Comment on attachment 389500 [details]
Patch

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

The pattern X && useSmoothScrolling(options.behavior, *X) is very frequent. Do we always need to check X != nullptr ? Maybe you want to move this into useSmoothScrolling instead (i.e. make the parameter a pointer) although I think references are preferred in WebKit. Alternatively, you can maybe follow https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op to avoid very long lines.

> Source/WebCore/rendering/RenderLayer.cpp:2828
> +            bool animated = (box->element() && useSmoothScrolling(options.behavior, *box->element()));

parenthesis not needed here

[cathiechen]

Hi Fred,
Thank you:)

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389500&action=review
> 
> The pattern X && useSmoothScrolling(options.behavior, *X) is very frequent.
> Do we always need to check X != nullptr ? Maybe you want to move this into
> useSmoothScrolling instead (i.e. make the parameter a pointer) although I
> think references are preferred in WebKit. Alternatively, you can maybe
> follow https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op to
> avoid very long lines.

Done

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2828
> > +            bool animated = (box->element() && useSmoothScrolling(options.behavior, *box->element()));
> 
> parenthesis not needed here

Done

[cathiechen]

Created attachment 389780 [details]
Patch

[cathiechen]

Created attachment 389785 [details]
Patch

[cathiechen]

Hi,

The patch has passed the check of EWS. I'm going to land it again:)

[cathiechen]

Hi,

The patch has passed the check of EWS. I'm going to land it again:)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389785 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389785 [details]
Patch

Clearing flags on attachment: 389785

Committed r255957: <https://trac.webkit.org/changeset/255957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.