Bug 128225 – Wheel events don't latch to inner scrollable elements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

When Asynchronous scrolling is used, the following can happen:

1. Display a web page that includes an Iframe or other scrollable region embedded in the main page.
2. Place the mouse inside the scrollable view.
3. Perform a quick swipe or two-finger-scroll to scroll the inner scrollable region.

The inner region begins to scroll, but when the end of the scrollable region is reached, the outer page immediately begins scrolling. This can be very surprising if you were hoping to read the bottom portion of an inner scrollable region.

The expected behavior is for the inner scroll to latch when it runs out of scrollable area.

This is happening because the inner scrollable region always treats an "end-of-scroll" state as an unhanded case, causing the next-higher element in the page hierarchy to try to handle the scroll.

[Brent Fulgham]

<rdar://problem/12183688>

[Brent Fulgham]

Created attachment 223191 [details]
Patch

[Brent Fulgham]

Created attachment 223272 [details]
Example (test) that doesn't use IFrames for scrollable regions

Reduced test case showing scrolling elements without WebKit internal "widgets" (i.e., div's with overflow and select elements, rather than IFrame.

[Brent Fulgham]

Created attachment 223273 [details]
Patch

[Brent Fulgham]

Committed r163558: <http://trac.webkit.org/changeset/163558>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 128350

[Ryosuke Niwa]

This patch caused a regression. See bug 128358.

[Brent Fulgham]

The regression seems to be related to attaching a wheel event handler to the DOM document. If such a handler is present, the patch in this issue treats the scrolling event as handled and does not attempt to perform further processing.

I'll revise the test case and patch.

[Brent Fulgham]

Created attachment 223772 [details]
Revised for additional cases

[Brent Fulgham]

Comment on attachment 223772 [details]
Revised for additional cases

Patch has been revised to deal with pages with wheel event handlers attached to the document. I also made sure <select> boxes work properly with the desired scrolling behavior.

[Brent Fulgham]

Created attachment 223773 [details]
Test Case With Wheel Event Listener

Test case that includes attached Wheel Event listeners.

[Brent Fulgham]

Created attachment 223777 [details]
Patch

[Build Bot]

Comment on attachment 223777 [details]
Patch

Attachment 223777 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6703387185774592

New failing tests:
scrollbars/scroll-rtl-or-bt-layer.html
fast/frames/flattening/scrolling-in-object.html
fast/events/remove-child-onscroll.html
fast/events/wheelevent-mousewheel-interaction.html
fast/events/platform-wheelevent-in-scrolling-div.html
fast/events/wheelevent-in-text-node.html
fast/events/wheelevent-basic.html
fast/events/platform-wheelevent-with-delta-zero-crash.html

[Build Bot]

Created attachment 223789 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Brent Fulgham]

(In reply to comment #13)
> (From update of attachment 223777 [details])
> Attachment 223777 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/6703387185774592
> 
> New failing tests:
> scrollbars/scroll-rtl-or-bt-layer.html
> fast/frames/flattening/scrolling-in-object.html
> fast/events/remove-child-onscroll.html
> fast/events/wheelevent-mousewheel-interaction.html
> fast/events/platform-wheelevent-in-scrolling-div.html
> fast/events/wheelevent-in-text-node.html
> fast/events/wheelevent-basic.html
> fast/events/platform-wheelevent-with-delta-zero-crash.html

Hrm. I'll look into these.

[Jon Lee]

Comment on attachment 223777 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2459
> +            

Extra line and unnecessary braces.

> Source/WebCore/page/EventHandler.cpp:2473
> +    if (DominantScrollGestureDirection::Horizontal == gestureDirection && deltaX) {

It it correct to ignore the dominant scroll gesture direction if there is no x delta? If the dominant direction is horizontal why check the vertical direction?

> Source/WebCore/page/EventHandler.cpp:2517
> +    bool enclosingFrameIsMainFrame = view ? view->frame().isMainFrame() : false;

this seems non-standard. better to write as
view && view->frame().isMainFrame()

> Source/WebCore/page/EventHandler.cpp:2552
> +    }

Is it possible for e.shouldConsiderLatching() && e.shouldClearLatchedNode()? If so, isOverWidget is not explicitly set.

I see later on this patch that if the phase is PlatformWheelEventPhaseMayBegin this is possible.

> Source/WebCore/page/EventHandler.cpp:2617
> +        enclosingFrameIsMainFrame = view ? view->frame().isMainFrame() : false;

rewrite as view && view->frame().isMainFrame()

> Source/WebCore/page/EventHandler.cpp:2619
> +            bool didHandleWheelEvent = view ? view->wheelEvent(event) : false;

similarly here.

> Source/WebCore/rendering/RenderListBox.h:63
> +    virtual bool isAtBottom() const override;

Seems like we should also have isAtLeft() and isAtRight() with ASSERT_NOT_REACHED(). Interesting that you can't override the width and force a horizontal scrollbar.

[Brent Fulgham]

Comment on attachment 223777 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2459
>> +            
> 
> Extra line and unnecessary braces.

OK

>> Source/WebCore/page/EventHandler.cpp:2473
>> +    if (DominantScrollGestureDirection::Horizontal == gestureDirection && deltaX) {
> 
> It it correct to ignore the dominant scroll gesture direction if there is no x delta? If the dominant direction is horizontal why check the vertical direction?

The idea here is to default to a vertical direction; we only bother looking at horizontal if the dominant gesture has been horizontal in the previous few wheel events, and we have some kind of horizontal delta. Otherwise we only bother looking at the vertical direction.

>> Source/WebCore/page/EventHandler.cpp:2517
>> +    bool enclosingFrameIsMainFrame = view ? view->frame().isMainFrame() : false;
> 
> this seems non-standard. better to write as
> view && view->frame().isMainFrame()

OK

>> Source/WebCore/page/EventHandler.cpp:2552
>> +    }
> 
> Is it possible for e.shouldConsiderLatching() && e.shouldClearLatchedNode()? If so, isOverWidget is not explicitly set.
> 
> I see later on this patch that if the phase is PlatformWheelEventPhaseMayBegin this is possible.

Ah! Good catch.

>> Source/WebCore/page/EventHandler.cpp:2617
>> +        enclosingFrameIsMainFrame = view ? view->frame().isMainFrame() : false;
> 
> rewrite as view && view->frame().isMainFrame()

OK

>> Source/WebCore/page/EventHandler.cpp:2619
>> +            bool didHandleWheelEvent = view ? view->wheelEvent(event) : false;
> 
> similarly here.

OK

>> Source/WebCore/rendering/RenderListBox.h:63
>> +    virtual bool isAtBottom() const override;
> 
> Seems like we should also have isAtLeft() and isAtRight() with ASSERT_NOT_REACHED(). Interesting that you can't override the width and force a horizontal scrollbar.

Done!

[Build Bot]

Comment on attachment 223777 [details]
Patch

Attachment 223777 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5542302906843136

New failing tests:
scrollbars/scroll-rtl-or-bt-layer.html
fast/events/continuous-platform-wheelevent-in-scrolling-div.html
fast/events/wheelevent-direction-inverted-from-device.html
fast/frames/flattening/scrolling-in-object.html
fast/events/remove-child-onscroll.html
fast/events/wheelevent-mousewheel-interaction.html
fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html
fast/events/platform-wheelevent-in-scrolling-div.html
fast/events/wheelevent-in-text-node.html
fast/events/wheelevent-basic.html
animations/stop-animation-on-suspend.html
fast/events/platform-wheelevent-with-delta-zero-crash.html

[Build Bot]

Created attachment 223800 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Brent Fulgham]

Created attachment 223804 [details]
Patch

[Brent Fulgham]

Patch updated to correct the test failures found by the EWS. It also incorporates Jon's review comments.

[Build Bot]

Comment on attachment 223804 [details]
Patch

Attachment 223804 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5421285760827392

New failing tests:
fast/frames/flattening/scrolling-in-object.html

[Build Bot]

Created attachment 223813 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223804 [details]
Patch

Attachment 223804 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4515595940790272

New failing tests:
fast/frames/flattening/scrolling-in-object.html

[Build Bot]

Created attachment 223821 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 223804 [details]
Patch

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

Generally looks like a reasonable start, but enough room for improvement, combined with not compiling on EFL and not passing tests on the Mac EWS bot, that I am going to say review-.

> Source/WebCore/page/EventHandler.cpp:2
> + * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2013, 2014 Apple Inc. All rights reserved.

We recently re-read the Apple copyright guidelines, and we can use year ranges. So this can say:

* Copyright (C) 2006-2011, 2013, 2014 Apple Inc. All rights reserved.

Or if you can verify we made and published substantive changes in 2012 and simply forgot to include that year, then it can say:

* Copyright (C) 2006-2014 Apple Inc. All rights reserved.

> Source/WebCore/page/EventHandler.cpp:2451
> +static inline Node* findScrollableNodeForElement(Node* node)

Strange to name this “for element” since it takes and returns Node*, not an Element*; it should be named findScrollableElement and should take an argument of type Element& and should return Element*. We could almost use parentElement if it wasn’t for that shadow host stuff. We just need to make a parentElement that crosses shadow boundaries; local to this file or in a header, either is fine.

Unless this can also return the document, in which case the correct return type is ContainerNode* and the function should be named findScrollableNode.

> Source/WebCore/page/EventHandler.cpp:2453
> +    // If we don't have a renderer, send the wheel event to the first node we find with a renderer.

This comment doesn’t match the code. For one thing, this function doesn’t send wheel events, so a comment here should not say that. For another, the algorithm here is “first node that has a renderer that can be scrolled and has a scrollable area”, not “first node that has a renderer”. Also, the comment says “the first node we find”, but doesn’t mention where we are looking (parents).

> Source/WebCore/page/EventHandler.cpp:2454
> +    // This is needed for <option> and <optgroup> elements so that <select>s get a wheel scroll.

This part of the comment is useful, but maybe should go at the call site.

> Source/WebCore/page/EventHandler.cpp:2462
> +    Node* startNode = node;
> +    while (startNode) {
> +        RenderBox* box = startNode->renderBox();
> +        if (box && box->canBeScrolledAndHasScrollableArea())
> +            return startNode;
> +            
> +        startNode = startNode->parentOrShadowHostNode();
> +    }

The name startNode doesn't make sense given the function name. This would read better as a for loop:

    for (Node* candidate = node; candidate; candidate = candidate->parentOrShadowHostNode())

> Source/WebCore/page/EventHandler.cpp:2467
> +static bool isAtMaxDominantScrollPosition(ScrollableArea* scrollableArea, DominantScrollGestureDirection gestureDirection, float deltaX, float deltaY)

This should take a ScrollableArea& instead of a ScrollableArea&. I also think we can use shorter argument names. Just area and direction would be fine, I think.

> Source/WebCore/page/EventHandler.cpp:2512
> +    Node* scrollableElement = nullptr;

If this really is an element, then I think the type should be Element* rather than Node*. If not, then the type should be ContainerNode and the name should be scrollableContainer, not scrollableElement.

> Source/WebCore/page/EventHandler.cpp:2514
> +    RenderListBox* scrollableSelectArea = nullptr;

I think “select area” is confusing. I would call this “scrollable list box” or “scrollable list”. But why can't we just set scrollableArea to the RenderListBox? We’d possibly have to make the inheritance from ScrollableArea be public rather than private.

I think that RenderBox should have a function to return the scrollable area since it already has the canBeScrolledAndHasScrollableArea function. It’s not good structure to have all this logic here just to find the different types of scrollable area.

> Source/WebCore/page/EventHandler.cpp:2515
> +    bool enclosingFrameIsMainFrame = view && view->frame().isMainFrame();

I don’t think this needs to be put in a local variable. I would put the expression inside the if statement.

> Source/WebCore/page/EventHandler.cpp:2517
> +        scrollableArea = static_cast<ScrollableArea*>(view);

The static_cast here is not needed. Please omit it.

> Source/WebCore/page/EventHandler.cpp:2521
> +        if (scrollableElement && isHTMLSelectElement(scrollableElement))
> +            scrollableSelectArea = toRenderListBox(scrollableElement->renderer());

This seems like the wrong way to write this. Instead of checking isHTMLSelectElement we should get the renderer and check isRenderListBox.

> Source/WebCore/page/EventHandler.cpp:2523
> +        if (scrollableElement && !scrollableElement->isDocumentNode() && !scrollableSelectArea) {

Instead of checking !scrollableSelectArea I suggest just making this an else.

Why is the isDocumentNode check needed? Non-obvious why it is a problem to call layerForNode on a document node. If it would return null, then we can just call it.

> Source/WebCore/page/EventHandler.cpp:2525
> +                scrollableArea = static_cast<ScrollableArea*>(layer);

The static_cast here is not needed. Please omit it.

> Source/WebCore/page/EventHandler.cpp:2620
> +            if (scrollableElement == m_scrollableLatchedElement.get()) {
> +                if (!didHandleWheelEvent) {

Should not need the get() here. Should use && here instead of nesting.

> Source/WebCore/page/EventHandler.cpp:2631
> +        if ((scrollableArea || scrollableSelectArea) && !m_startedGestureAtScrollLimit) {
> +            if (scrollableElement == m_scrollableLatchedElement.get()) {

Should not need the get() here. Should use && here instead of nesting.

> Source/WebCore/page/EventHandler.h:2
> + * Copyright (C) 2006, 2007, 2009, 2010, 2011, 2013, 2014 Apple Inc. All rights reserved.

Same comment as above about copyright years.

> Source/WebCore/page/EventHandler.h:523
> +    RefPtr<Node> m_scrollableLatchedElement;

We should not have a date member that is a Node, but that is named Element. Please either rename this or change the type.

> Source/WebCore/platform/PlatformWheelEvent.h:173
> +            return m_phase == PlatformWheelEventPhaseBegan
> +                || m_phase == PlatformWheelEventPhaseMayBegin;

Consider writing this on a single line?

> Source/WebCore/platform/PlatformWheelEvent.h:175
> +        bool shouldClearLatchedNode() const

Not sure this really belongs as a data member here. It seems more like policy than just the event information.

Also, the point of this class is to abstract the platform, so it seems strange we are adding these functions only for PLATFORM(COCOA).

> Source/WebCore/platform/PlatformWheelEvent.h:179
> +            if (m_phase == PlatformWheelEventPhaseCancelled
> +                || m_phase == PlatformWheelEventPhaseMayBegin)
> +                return true;

I’d write the if condition out on a single line. Would be easier to read.

[Darin Adler]

Comment on attachment 223804 [details]
Patch

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

No need to address all my comments, but please at least consider all of them.

>> Source/WebCore/platform/PlatformWheelEvent.h:175
>> +        bool shouldClearLatchedNode() const
> 
> Not sure this really belongs as a data member here. It seems more like policy than just the event information.
> 
> Also, the point of this class is to abstract the platform, so it seems strange we are adding these functions only for PLATFORM(COCOA).

I meant member function.

[Brent Fulgham]

Comment on attachment 223804 [details]
Patch

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

Thanks for reviewing this so carefully. I'm revising things to incorporate those ideas, which has simplified the logic.

>> Source/WebCore/page/EventHandler.cpp:2
>> + * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2013, 2014 Apple Inc. All rights reserved.
> 
> We recently re-read the Apple copyright guidelines, and we can use year ranges. So this can say:
> 
> * Copyright (C) 2006-2011, 2013, 2014 Apple Inc. All rights reserved.
> 
> Or if you can verify we made and published substantive changes in 2012 and simply forgot to include that year, then it can say:
> 
> * Copyright (C) 2006-2014 Apple Inc. All rights reserved.

OK. I checked the repository history, and Anders and Beth made significant changes in 2012. I'll use the full range.

>> Source/WebCore/page/EventHandler.cpp:2451
>> +static inline Node* findScrollableNodeForElement(Node* node)
> 
> Strange to name this “for element” since it takes and returns Node*, not an Element*; it should be named findScrollableElement and should take an argument of type Element& and should return Element*. We could almost use parentElement if it wasn’t for that shadow host stuff. We just need to make a parentElement that crosses shadow boundaries; local to this file or in a header, either is fine.
> 
> Unless this can also return the document, in which case the correct return type is ContainerNode* and the function should be named findScrollableNode.

Yes. The traversal can run all the way to the document, if that's the only scrollable element in the set of containing elements. I'll revise the signature to use ContainerNode, and adjust the naming.

It seems like there are a few places in the code where this pattern is followed (i.e., traverse the DOM from one element to the root considering both parents and shadowHosts). Maybe this kind of traversal could be encapsulated in ContainerNode, taking a predicate functor argument.

>> Source/WebCore/page/EventHandler.cpp:2453
>> +    // If we don't have a renderer, send the wheel event to the first node we find with a renderer.
> 
> This comment doesn’t match the code. For one thing, this function doesn’t send wheel events, so a comment here should not say that. For another, the algorithm here is “first node that has a renderer that can be scrolled and has a scrollable area”, not “first node that has a renderer”. Also, the comment says “the first node we find”, but doesn’t mention where we are looking (parents).

I'll correct the comments.

>> Source/WebCore/page/EventHandler.cpp:2454
>> +    // This is needed for <option> and <optgroup> elements so that <select>s get a wheel scroll.
> 
> This part of the comment is useful, but maybe should go at the call site.

Will do.

>> Source/WebCore/page/EventHandler.cpp:2462
>> +    }
> 
> The name startNode doesn't make sense given the function name. This would read better as a for loop:
> 
>     for (Node* candidate = node; candidate; candidate = candidate->parentOrShadowHostNode())

Will do. It might be nice to have some kind of iterator exposed on ContainerNode that would allow us to do "for (auto& candidate : node.parentAndShadowHostNodes())"

>> Source/WebCore/page/EventHandler.cpp:2467
>> +static bool isAtMaxDominantScrollPosition(ScrollableArea* scrollableArea, DominantScrollGestureDirection gestureDirection, float deltaX, float deltaY)
> 
> This should take a ScrollableArea& instead of a ScrollableArea&. I also think we can use shorter argument names. Just area and direction would be fine, I think.

Will do. I think ScrollableArea should also be const, but probably doesn't matter for this simple function.

>> Source/WebCore/page/EventHandler.cpp:2512
>> +    Node* scrollableElement = nullptr;
> 
> If this really is an element, then I think the type should be Element* rather than Node*. If not, then the type should be ContainerNode and the name should be scrollableContainer, not scrollableElement.

Done.

>> Source/WebCore/page/EventHandler.cpp:2514
>> +    RenderListBox* scrollableSelectArea = nullptr;
> 
> I think “select area” is confusing. I would call this “scrollable list box” or “scrollable list”. But why can't we just set scrollableArea to the RenderListBox? We’d possibly have to make the inheritance from ScrollableArea be public rather than private.
> 
> I think that RenderBox should have a function to return the scrollable area since it already has the canBeScrolledAndHasScrollableArea function. It’s not good structure to have all this logic here just to find the different types of scrollable area.

Great Idea!  I'll change to follow this approach.

>> Source/WebCore/page/EventHandler.cpp:2515
>> +    bool enclosingFrameIsMainFrame = view && view->frame().isMainFrame();
> 
> I don’t think this needs to be put in a local variable. I would put the expression inside the if statement.

Done

>> Source/WebCore/page/EventHandler.cpp:2517
>> +        scrollableArea = static_cast<ScrollableArea*>(view);
> 
> The static_cast here is not needed. Please omit it.

Done

>> Source/WebCore/page/EventHandler.cpp:2521
>> +            scrollableSelectArea = toRenderListBox(scrollableElement->renderer());
> 
> This seems like the wrong way to write this. Instead of checking isHTMLSelectElement we should get the renderer and check isRenderListBox.

I rewrote this to use the new RenderBox::scrollableArea() method, which makes this mess go away.

>> Source/WebCore/page/EventHandler.cpp:2523
>> +        if (scrollableElement && !scrollableElement->isDocumentNode() && !scrollableSelectArea) {
> 
> Instead of checking !scrollableSelectArea I suggest just making this an else.
> 
> Why is the isDocumentNode check needed? Non-obvious why it is a problem to call layerForNode on a document node. If it would return null, then we can just call it.

I think I had this in place before I realized ScrollableArea might be null in other cases. This test isn't needed and I'll remove it. Thanks!

>> Source/WebCore/page/EventHandler.cpp:2525
>> +                scrollableArea = static_cast<ScrollableArea*>(layer);
> 
> The static_cast here is not needed. Please omit it.

It's gone!

>> Source/WebCore/page/EventHandler.cpp:2620
>> +                if (!didHandleWheelEvent) {
> 
> Should not need the get() here. Should use && here instead of nesting.

Done.

>> Source/WebCore/page/EventHandler.cpp:2631
>> +            if (scrollableElement == m_scrollableLatchedElement.get()) {
> 
> Should not need the get() here. Should use && here instead of nesting.

Done.

>> Source/WebCore/page/EventHandler.h:2
>> + * Copyright (C) 2006, 2007, 2009, 2010, 2011, 2013, 2014 Apple Inc. All rights reserved.
> 
> Same comment as above about copyright years.

Done.

>> Source/WebCore/page/EventHandler.h:523
>> +    RefPtr<Node> m_scrollableLatchedElement;
> 
> We should not have a date member that is a Node, but that is named Element. Please either rename this or change the type.

Done. I'm making these ContainerNodes.

>> Source/WebCore/platform/PlatformWheelEvent.h:173
>> +                || m_phase == PlatformWheelEventPhaseMayBegin;
> 
> Consider writing this on a single line?

Sure!

>>> Source/WebCore/platform/PlatformWheelEvent.h:175
>>> +        bool shouldClearLatchedNode() const
>> 
>> Not sure this really belongs as a data member here. It seems more like policy than just the event information.
>> 
>> Also, the point of this class is to abstract the platform, so it seems strange we are adding these functions only for PLATFORM(COCOA).
> 
> I meant member function.

All of the latching logic is protected by #if PLATFORM(COCOA), so I put theme here. I wanted to move this test here, since the same test is used for both WK1 and WK2 scrolling, but the implementations are in separate files. I'd prefer to keep this logic consistent for both scrolling mechanisms.

>> Source/WebCore/platform/PlatformWheelEvent.h:179
>> +                return true;
> 
> I’d write the if condition out on a single line. Would be easier to read.

Done.

> Source/WebCore/rendering/RenderListBox.cpp:855
> +    return ScrollableArea::isAtLeft();

Note: I'm changing these to always return 'true' (since we cannot scroll horizontally in the ListBox/Select element). This allows use the same logic for all scroll areas in our other code.

[Brent Fulgham]

Created attachment 223896 [details]
Patch

[Brent Fulgham]

Tests all pass locally on Mavericks.

[Simon Fraser (smfr)]

Comment on attachment 223896 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2450
> +static inline ContainerNode* findScrollableContainer(ContainerNode& node)

I don't think there's any benefit for this being inline.

> Source/WebCore/page/EventHandler.cpp:2505
> +#if PLATFORM(COCOA)

Why isn't this PLATFORM(COCOA) code in EventHandlerMac/iOS?

> Source/WebCore/page/EventHandler.cpp:2517
> +    if (!view || !view->frame().isMainFrame()) {
> +        scrollableContainer = element;
> +        scrollableArea = static_cast<ScrollableArea*>(view);
> +    } else {
> +        scrollableContainer = findScrollableContainer(*element);
> +        if (scrollableContainer) {
> +            if (RenderBox* box = scrollableContainer->renderBox())
> +                scrollableArea = box->scrollableArea();
> +        }
> +    }

Why compute the scrollableArea here when you only use it for one branch below?

Maybe this code can be split out into a helper function.

> Source/WebCore/page/EventHandler.cpp:2521
> +            m_startedGestureAtScrollLimit = isAtMaxDominantScrollPosition(*scrollableArea, m_recentWheelEventDeltaTracker->dominantScrollGestureDirection(), e.deltaX(), e.deltaY());

Do we always have a m_recentWheelEventDeltaTracker?

> Source/WebCore/page/EventHandler.cpp:2603
> +#if PLATFORM(COCOA)

More platform code in a cross-platorm file!

> Source/WebCore/platform/PlatformWheelEvent.h:174
> +        bool shouldClearLatchedNode() const

I'd call this shouldResetLatching(); this code doesn't care about nodes.

> Source/WebCore/rendering/RenderBox.cpp:4730
> +ScrollableArea* RenderBox::scrollableArea()
> +{
> +    if (!canBeScrolledAndHasScrollableArea())
> +        return nullptr;
> +
> +    return enclosingLayer();

Not sure this is right. First, it should be enclosingScrollableArea().

enclosingLayer() will return a layer, but that layer doesn't necessarily scroll.

> Source/WebCore/rendering/RenderListBox.h:66
> +    virtual bool isAtBottom() const override;
> +    virtual bool isAtLeft() const override;
> +    virtual bool isAtRight() const override;

"isAtTop" is ambiguous: scrolled to top, or positioned at the top of something else?

I'd called these "scrolledToTop()" etc.

[Brent Fulgham]

Comment on attachment 223896 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2450
>> +static inline ContainerNode* findScrollableContainer(ContainerNode& node)
> 
> I don't think there's any benefit for this being inline.

OK

>> Source/WebCore/page/EventHandler.cpp:2517
>> +    }
> 
> Why compute the scrollableArea here when you only use it for one branch below?
> 
> Maybe this code can be split out into a helper function.

It's also used down at the bottom of the method.

>> Source/WebCore/page/EventHandler.cpp:2521
>> +            m_startedGestureAtScrollLimit = isAtMaxDominantScrollPosition(*scrollableArea, m_recentWheelEventDeltaTracker->dominantScrollGestureDirection(), e.deltaX(), e.deltaY());
> 
> Do we always have a m_recentWheelEventDeltaTracker?

Yes. This is constructed at EventHandler creation, and lives for the duration of the EventHandler object.

>> Source/WebCore/platform/PlatformWheelEvent.h:174
>> +        bool shouldClearLatchedNode() const
> 
> I'd call this shouldResetLatching(); this code doesn't care about nodes.

Done.

>> Source/WebCore/rendering/RenderBox.cpp:4730
>> +    return enclosingLayer();
> 
> Not sure this is right. First, it should be enclosingScrollableArea().
> 
> enclosingLayer() will return a layer, but that layer doesn't necessarily scroll.

OK. I've revised based on our conversation in my office.

>> Source/WebCore/rendering/RenderListBox.h:66
>> +    virtual bool isAtRight() const override;
> 
> "isAtTop" is ambiguous: scrolled to top, or positioned at the top of something else?
> 
> I'd called these "scrolledToTop()" etc.

Done.

[Brent Fulgham]

Created attachment 223937 [details]
Patch

[Brent Fulgham]

Created attachment 223981 [details]
Bad patch

[Brent Fulgham]

Comment on attachment 223981 [details]
Bad patch

I revised the patch so that the Mac latching code now lives in EventHandlerMac. The "all other ports" code (which is just a stub) still lives in EventHandler.cpp to avoid having to modify five files just to add the identical stub.

[Brent Fulgham]

Created attachment 223983 [details]
Revised per Smfr's comments

[Simon Fraser (smfr)]

Comment on attachment 223983 [details]
Revised per Smfr's comments

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

> Source/WebCore/page/EventHandler.h:202
> +    void platformPrepareForWheelEvents(const PlatformWheelEvent&, const HitTestResult&, Element*&, ContainerNode*&, ScrollableArea*&, bool&);

You should give the parameters names here to say what they mean.

> Source/WebCore/page/EventHandler.h:204
> +    bool platformCompleteWheelEvent(const PlatformWheelEvent&, ContainerNode*, ScrollableArea*);

Here too.

> Source/WebCore/page/mac/EventHandlerMac.mm:745
> +    // node and traversing it's parents (or shadow hosts).

it's :|

> Source/WebCore/page/mac/EventHandlerMac.mm:755
> +static bool isAtMaxDominantScrollPosition(const ScrollableArea& area, DominantScrollGestureDirection direction, float deltaX, float deltaY)

scrolledToEdgeInDominantDirection?

> Source/WebCore/page/mac/EventHandlerMac.mm:770
> +void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& e, const HitTestResult& result, Element*& element, ContainerNode*& scrollableContainer, ScrollableArea*& scrollableArea, bool& isOverWidget)

Please don't abbreviate event to 'e'. 'element' could use a more descriptive name.

> Source/WebCore/page/mac/EventHandlerMac.mm:778
> +        scrollableArea = static_cast<ScrollableArea*>(view);

This cast should not be necessary.

> Source/WebCore/page/mac/EventHandlerMac.mm:787
> +        scrollableContainer = findScrollableContainer(*element);
> +        if (scrollableContainer) {
> +            if (RenderBox* box = scrollableContainer->renderBox()) {
> +                if (box->isListBox())
> +                    scrollableArea = toRenderListBox(box);
> +                else
> +                    scrollableArea = box->layer();
> +            }

Maybe findScrollableContainer() should be
ScrollableArea* findEnclosingScrollableArea(Element&*)

[Brent Fulgham]

Committed r163975: <http://trac.webkit.org/changeset/163975>