Bug 131869

Summary: Latched scrolling may interact badly with custom programmatic scrolling
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, commit-queue, dino, esprehn+autocc, kangil.han, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128225    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
darin: review+
Testcase none

Description Brent Fulgham 2014-04-18 17:27:20 PDT
Some sites manually scroll widgets by attaching 'mousewheel'/'wheel'/'scroll' handlers that scroll the element using the DOM 'setScrollTop' method, and block further propagation of the wheel event to the rest of the DOM.

Latched scrolling (added in <https://trac.webkit.org/changeset/163975> bypasses this scrolling logic, but interacts badly with the blocking of the event propagation, resulting in "unscrollable" widgets.

This patch identifies cases where the scrollable area is being set via "setScrollTop", and does not perform latched scrolling in these cases. It is assumed that the web developer controlling their scrolling manually will deal with any desired latching behavior.
Comment 1 Brent Fulgham 2014-04-18 17:34:42 PDT
Created attachment 229692 [details]
Patch
Comment 2 Brent Fulgham 2014-04-18 17:35:22 PDT
<rdar://problem/16249557>
Comment 3 Brent Fulgham 2014-04-18 17:36:02 PDT
smfr: Do we need logic to somehow clear the programmatic scroll state? If so, when would we decide to do so? Maybe at the beginning of a swipe event?
Comment 4 Brent Fulgham 2014-04-18 18:21:04 PDT
Created attachment 229696 [details]
Patch
Comment 5 Brent Fulgham 2014-04-18 20:50:11 PDT
Created attachment 229710 [details]
Patch
Comment 6 Brent Fulgham 2014-04-18 21:10:03 PDT
Created attachment 229714 [details]
Patch
Comment 7 Darin Adler 2014-04-19 14:53:57 PDT
Comment on attachment 229714 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:802
> +    if (RenderBox* rend = renderBox()) {

rend is such an unpleasant local variable name; I suggest renderer instead

> Source/WebCore/dom/Element.cpp:804
> +        if (ScrollableArea* scrollableArea = static_cast<ScrollableArea*>(rend->layer()))

This typecast is a bad idea. A RenderLayer is a ScrollableArea, so there is no reason to cast, but further, no reason to narrow the type like this. It should just say:

    if (auto* layer = renderer->layer())
        layer->setScrolledProgrammatically(true);

> Source/WebCore/dom/Element.cpp:805
> +            scrollableArea->setScrolledProgrammatically(true);

This won’t ever get reset to false? It’s a permanent state of affairs?

> Source/WebCore/page/EventHandler.h:301
> +protected:
> +    void clearLatchedState();

should be private not protected; no classes derive from EventHandler, so there’s no difference between protected and private

Also, should always prefer private to protected unless there is some reason a member needs to be exposed to derived classes.
Comment 8 Brent Fulgham 2014-04-19 19:33:38 PDT
Committed r167560: <http://trac.webkit.org/changeset/167560>
Comment 9 Simon Fraser (smfr) 2020-08-30 10:51:19 PDT
Created attachment 407569 [details]
Testcase