Bug 131869 - Latched scrolling may interact badly with custom programmatic scrolling
Summary: Latched scrolling may interact badly with custom programmatic scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 128225
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-18 17:27 PDT by Brent Fulgham
Modified: 2020-08-30 11:13 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.26 KB, patch)
2014-04-18 17:34 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2014-04-18 18:21 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2014-04-18 20:50 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.72 KB, patch)
2014-04-18 21:10 PDT, Brent Fulgham
darin: review+
Details | Formatted Diff | Diff
Testcase (4.74 KB, text/html)
2020-08-30 10:51 PDT, Simon Fraser (smfr)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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