Bug 217709 – Make isUserScrollInProgress supporting more user scroll actions

• Component: Scrolling
• Status: NEW

[cathiechen]

Besides wheel actions, make isUserScrollInProgress support other scroll actions, like scrollbar, keyboard and dragging scroll actions.

[cathiechen]

Created attachment 411325 [details]
Patch

[cathiechen]

Hi Simon,

The user scroll animations are scatter. I wonder could we add an interface to get the status of user scroll.
`isUserScrollInProgress` has supported the wheel event, maybe we can extend it to other user actions too? Like, keyboard, scrollbar and dragging scroll.

As to test, there seems not an obvious case for it.

Please take a look at this, thanks!

[Simon Fraser (smfr)]

Comment on attachment 411325 [details]
Patch

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

> Source/WebCore/page/AutoscrollController.cpp:69
> +    if (!renderbox)
> +        return;
> +    if (renderbox->isRenderView()) {
> +        renderbox->view().frameView().scrollAnimator().setUserScrollInProgress(inProgress);
> +        return;
> +    }
> +    if (renderbox->layer())
> +        renderbox->layer()->scrollAnimator().setUserScrollInProgress(inProgress);
> +}

Should this really call setUserScrollInProgress on both? We only ever scroll one thing at a time, so not sure if this is necessary. Also, what about all the overflow:scroll that might be between this renderBox and the view?

> Source/WebCore/page/AutoscrollController.cpp:297
> +    updateScrollActionState(m_autoscrollRenderer, true);

Unclear what "true" means here.

> Source/WebCore/platform/ScrollAnimator.h:127
> -    virtual bool isUserScrollInProgress() const { return false; }
> +    void setUserScrollInProgress(bool);
> +    virtual bool isUserScrollInProgress() const { return m_scrollActionsInProgress; }

Currently ScrollController has isUserScrollInProgress() (but ScrollAnimator has a virtual function, and ScrollAnimatorMac just overrides it to call its m_scrollController, which is confusing. But storing additional state in ScrollAnimator is now spreading state around in too many places. I think this needs a cleaner solution.

I've never liked the ScrollAnimator/ScrollController split. Maybe it's time to decide on a good design for that.

> Source/WebCore/platform/ScrollView.cpp:544
> +    scrollAnimator().setUserScrollInProgress(true);

How do we know this is a user scroll here?

> Source/WebCore/rendering/RenderLayer.cpp:4168
> +    scrollAnimator().setUserScrollInProgress(true);

Do we know this is a user scroll here?

There's also ambiguity with ScrollableArea::currentScrollType(). What's the distinction between that and setUserScrollInProgress()?

[Radar WebKit Bug Importer]

<rdar://problem/70529241>

[cathiechen]

Comment on attachment 411325 [details]
Patch

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

Hi Simon,
Sorry for the slow reply!
I made some twists of this patch. ScrollAnimator::isUserScrollInProgress() now can support scrollbar, drag select and keyboard scroll. For wheel scroll, it is not fully effective. I think we can support it after we finish the work in bug: 218277?

>> Source/WebCore/page/AutoscrollController.cpp:69
>> +}
> 
> Should this really call setUserScrollInProgress on both? We only ever scroll one thing at a time, so not sure if this is necessary. Also, what about all the overflow:scroll that might be between this renderBox and the view?

Ah, yes, I'll move the autoscroll check to RenderLayer::scrollRectToVisible().

>> Source/WebCore/page/AutoscrollController.cpp:297
>> +    updateScrollActionState(m_autoscrollRenderer, true);
> 
> Unclear what "true" means here.

Sorry, I'll add a clear name here.

>> Source/WebCore/platform/ScrollAnimator.h:127
>> +    virtual bool isUserScrollInProgress() const { return m_scrollActionsInProgress; }
> 
> Currently ScrollController has isUserScrollInProgress() (but ScrollAnimator has a virtual function, and ScrollAnimatorMac just overrides it to call its m_scrollController, which is confusing. But storing additional state in ScrollAnimator is now spreading state around in too many places. I think this needs a cleaner solution.
> 
> I've never liked the ScrollAnimator/ScrollController split. Maybe it's time to decide on a good design for that.

Yeah, the state of ScrollAnimator only indicates user scroll actions' statuses, but not the wheel scroll's.
I wanted to reuse the wheel scroll status in ScrollController. After moving the common interfaces from ScrollController to ScrollAnimator (in bug 218277), we can make it support wheel scroll too.

>> Source/WebCore/platform/ScrollView.cpp:544
>> +    scrollAnimator().setUserScrollInProgress(true);
> 
> How do we know this is a user scroll here?

I think it is a frameview keyboard scroll here. I'll add a scrolltype check here.

>> Source/WebCore/rendering/RenderLayer.cpp:4168
>> +    scrollAnimator().setUserScrollInProgress(true);
> 
> Do we know this is a user scroll here?
> 
> There's also ambiguity with ScrollableArea::currentScrollType(). What's the distinction between that and setUserScrollInProgress()?

Like ScrollView::scroll, I try to handle the overflow element keyboard command scroll here. Element::scrollByLines and Element::scrollByPages also can call here, but it seems they are deprecated.

Yeah, we treat each command as a user scroll action here, so the status of isUserScrollInProgress for keyboard scrolls seems the same as ScrollType. As for other user scroll, it's different, isUserScrollInProgress() indicates whether the user scroll action or user scroll animation is in progress. For the programmatic smooth scroll, it can start the animator at first, then stops if it is interrupted by a scroll with ScrollType::User. But for programmatic instant scroll, it can not know if the user scroll is in progress, this will be needed by "scrollend" event, for only one scrollend event should be fired when scrolls combined. With isUserScrollInProgress, it can stop smooth scroll animation at the very beginning and provide a way for programmatic instant scroll to figure out if it should fire a "scrollend" event or not.

[cathiechen]

Created attachment 414903 [details]
WIP-patch

Needs to support isUserscrollInProgress for wheel event scrolling after refactoring done in 218277