Latched scrolling may interact badly with custom programmatic scrolling

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 229692 [details]
Patch

[Brent Fulgham]

<rdar://problem/16249557>

[Brent Fulgham]

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?

[Brent Fulgham]

Created attachment 229696 [details]
Patch

[Brent Fulgham]

Created attachment 229710 [details]
Patch

[Brent Fulgham]

Created attachment 229714 [details]
Patch

[Darin Adler]

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.

[Brent Fulgham]

Committed r167560: <http://trac.webkit.org/changeset/167560>

[Simon Fraser (smfr)]

Created attachment 407569 [details]
Testcase