Bug 80596 – add a general ScrollableArea::scroll function

• Status: NEW
• Component: UI Events

[Robert Kroeger]

ScrollableArea::scroll is too limited to serve the needs of track pads, touch screens etc. Instead, more complex scrolls are initiated via ScrollAnimator::handleWheelEvent. But ScrollableAreas should not be concerned with wheel events. (Note for example how methods like mouseEnteredContentArea() do not take mouse events but instead provide only the minimum information to the ScrollableArea to correctly handle scrolling.) Instead, there should be a richer ::scroll interface.

This bug tracks the idea of adding a generalized scroll function to ScrollableArea and then removing the dependency on wheel events. If this idea has merit, I'll add sub-bugs to track the actual code changes.

[Robert Kroeger]

Possibly nascent idea:

Scrolling a FrameView is different from scrolling a div. Does it need to be?

wheel event flow, FrameView.
===
5   0x135641f1 WebCore::ScrollableArea::scrollPositionChanged(WebCore::IntPoint const&)
6   0x13564581 WebCore::ScrollableArea::setScrollOffsetFromAnimation(WebCore::IntPoint const&)
-- D: common --
7   0x13556e1a WebCore::ScrollAnimator::notifyPositionChanged()
8   0x1381ced9 WebCore::ScrollAnimatorMac::notifyPositionChanged()
9   0x13556546 WebCore::ScrollAnimator::scroll(WebCore::ScrollbarOrientation, WebCore::ScrollGranularity, float, float)
10  0x1381c795 WebCore::ScrollAnimatorMac::scroll(WebCore::ScrollbarOrientation, WebCore::ScrollGranularity, float, float)
11  0x13556ad6 WebCore::ScrollAnimator::handleWheelEvent(WebCore::PlatformWheelEvent const&)
12  0x1381e299 WebCore::ScrollAnimatorMac::handleWheelEvent(WebCore::PlatformWheelEvent const&)
13  0x13564490 WebCore::ScrollableArea::handleWheelEvent(WebCore::PlatformWheelEvent const&)
-- C: found the scrollable area  --
14  0x126b966e WebCore::FrameView::wheelEvent(WebCore::PlatformWheelEvent const&)
-- A: start divergent path --
15  0x12673fbe WebCore::EventHandler::handleWheelEvent(WebCore::PlatformWheelEvent const&)

wheel event flow, scrollable div
===
1   0x6741e17e WebCore::ScrollableArea::scrollPositionChanged(WebCore::IntPoint const&)
2   0x6741e541 WebCore::ScrollableArea::setScrollOffsetFromAnimation(WebCore::IntPoint const&)
-- D: common --
3   0x67410dca WebCore::ScrollAnimator::notifyPositionChanged()
4   0x676d6e99 WebCore::ScrollAnimatorMac::notifyPositionChanged()
5   0x674104f6 WebCore::ScrollAnimator::scroll(WebCore::ScrollbarOrientation, WebCore::ScrollGranularity, float, float)
6   0x676d6755 WebCore::ScrollAnimatorMac::scroll(WebCore::ScrollbarOrientation, WebCore::ScrollGranularity, float, float)
7   0x6741de87 WebCore::ScrollableArea::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float)
-- C: found the scrollable area --
8   0x65a638cc WebCore::RenderLayer::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float)
9   0x659a6bb3 WebCore::RenderBox::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float, WebCore::Node**)
10  0x659a6c8e WebCore::RenderBox::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float, WebCore::Node**)
11  0x659a6c8e WebCore::RenderBox::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float, WebCore::Node**)
12  0x659a6c8e WebCore::RenderBox::scroll(WebCore::ScrollDirection, WebCore::ScrollGranularity, float, WebCore::Node**)
13  0x6652e36b WebCore::scrollNode(float, WebCore::ScrollGranularity, WebCore::ScrollDirection, WebCore::ScrollDirection, WebCore::Node*, WebCore::Node**)
-- B: dom dispatch done --
14  0x6652e0d0 WebCore::EventHandler::defaultWheelEventHandler(WebCore::Node*, WebCore::WheelEvent*)
15  0x67943c58 WebCore::Node::defaultEventHandler(WebCore::Event*)
16  0x67906b65 WebCore::EventDispatcher::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
17  0x67904bef WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const
18  0x679fa74b WebCore::WheelEventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const
19  0x67905a35 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>)
20  0x67943452 WebCore::Node::dispatchWheelEvent(WebCore::PlatformWheelEvent const&)
-- A: start divergent path --
21  0x6652df11 WebCore::EventHandler::handleWheelEvent(WebCore::PlatformWheelEvent const&)

Two refactorings seem possible that might improve the code. They can be done in either order and be completed
independently.

refactoring 1
----
unify code-path potion C-D by introducing a new single kind of scroll. Something like:
ScrollableArea::scrollByVector(FloatPoint, ScrollByVectorFlags) (Strawman name -- this one seems descriptive.)

ScrollByVectorFlags would subsume phase, momentum phase, granularity. The caller would be responsible
for setting up the sign of the FloatPoint to specify direction.

refactoring 2
---
To let scrollable divs get scrolled the same way as FrameViews whether it happens by calling
ScrollableArea::handleWheelEvent or by calling ScrollableArea::scrollByVector:

divide up the process of A-C in EventHandler::handleWheelEvent into three phases:
*   preserve the target finding code
*   perform A-B: dispatch the wheel event into the DOM without the callback into EventHandler.
*   perform B-C: find the ScrollableArea that's going to handle the scroll by a call from EventHandler.
*   finally, dispatch a ScrollableArea::scrollByVector (or ::handleWheelEvent)

Approach
---
*  on both paths we pass through ScrollAnimator:: to perform scrolls so we can verify the correctness
of the changes by validating the calls to ScrollAnimator::notifyPositionChanged. I would think that this
could be exposed to layout tests via window.internals.

*  modify existing wheel (and gesture) event tests to baseline their current position change handling

*  in either order perform refactoring 1 (incrementally) and refactoring 2. 

Comments? In particular: how should this be recast to help with the scrolling thread? Or is this all a bad idea?