Bug 135195 – Refactor EventHandler to call ScrollAnimator::handleWheelEvent for overflow scrolling

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

This is a subproblem of the snap points feature, seen here: https://bugs.webkit.org/show_bug.cgi?id=134283

In order to implement snap points in overflow scrolling on Mac, I need to expose wheel event information (i.e. phase and momentum phase) from EventHandler::defaultWheelEventHandler to ScrollAnimator::scroll. This is also necessary for implementing rubber-banding behavior in overflow scrolling.

[Wenson Hsieh]

Created attachment 235375 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 235375 [details]
Patch

Should figure out way to do this without #ifdefs.

[Wenson Hsieh]

Created attachment 235438 [details]
Patch

[Build Bot]

Comment on attachment 235438 [details]
Patch

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

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select.html

[Build Bot]

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

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

[Wenson Hsieh]

Created attachment 235457 [details]
Patch

[WebKit Commit Bot]

Attachment 235457 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 235470 [details]
Patch

[Build Bot]

Comment on attachment 235470 [details]
Patch

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

New failing tests:
fast/events/wheelevent-in-scrolling-div.html
fast/forms/number/number-wheel-event.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 235470 [details]
Patch

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

New failing tests:
fast/events/wheelevent-in-scrolling-div.html
fast/forms/number/number-wheel-event.html

[Build Bot]

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

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

[Jon Lee]

Comment on attachment 235470 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        (WebCore::platformWheelEventFromWheelEvent): Stub that makes a PlatformWheelEvent from a WheelEvent. Will soon be replaced by a simple WheelEvent::wheelEvent().

Looks like this is from the old patch.

[Wenson Hsieh]

Created attachment 235523 [details]
Patch

[Wenson Hsieh]

Comment on attachment 235470 [details]
Patch

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

This attempt seems to have broken scrolling in cases where WheelEvent is created without PlatformWheelEvent, presumably via JavaScript. I've (hopefully) fixed this in a new version by using the old code-path that directly calls RenderLayer::scroll if the WheelEvent is artificially generated, i.e. it has no corresponding PlatformWheelEvent.

>> Source/WebCore/ChangeLog:11
>> +        (WebCore::platformWheelEventFromWheelEvent): Stub that makes a PlatformWheelEvent from a WheelEvent. Will soon be replaced by a simple WheelEvent::wheelEvent().
> 
> Looks like this is from the old patch.

Good catch -- updated the ChangeLogs in the new patch.

[Build Bot]

Comment on attachment 235523 [details]
Patch

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

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

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

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

[Wenson Hsieh]

Created attachment 235530 [details]
Patch

[Wenson Hsieh]

Created attachment 235533 [details]
Patch

[Wenson Hsieh]

Created attachment 235613 [details]
Patch

[Build Bot]

Comment on attachment 235613 [details]
Patch

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

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

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

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

[Tim Horton]

Comment on attachment 235613 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:311
> +                    *stopElement = currentEnclosingBox->element();

that's a lot of nesting :| can we make this flatter?

> Source/WebCore/page/EventHandler.cpp:-2640
> -        dominantDirection = m_recentWheelEventDeltaTracker->dominantScrollGestureDirection();

where did all this code go?

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:378
> +RenderBlock* RenderNamedFlowThread::fragmentFromCurrentlyScrollingBlockAsRenderBlock(RenderBlock* renderBlock, const IntPoint& absolutePoint, const RenderBox& flowedBox)

this name seems a bit weird since what the function does doesn't seem to have anything to do with scrolling?

[Simon Fraser (smfr)]

Comment on attachment 235613 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This should go above the explanatory paragraph.

> Source/WebCore/page/EventHandler.cpp:296
> +static inline bool handleWheelEventInScrollableArea(Node* startNode, WheelEvent* wheelEvent, Element** stopElement)

Name could be slightly better, communicating the fact that it looks up the stack of enclosing scrollableAreas.

> Source/WebCore/page/EventHandler.cpp:299
> +        return false;

Blank line below please.

> Source/WebCore/page/EventHandler.cpp:300
> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();

Is startNode guaranteed to have a renderer()?

> Source/WebCore/page/EventHandler.cpp:303
> +    RenderBox* currentEnclosingBox = initialEnclosingBox;

Blank line above please.

> Source/WebCore/page/EventHandler.cpp:304
> +    RenderBlock* nextScrollBlock;

You should declare this on first use. If declared here, I assume that you'll refer to it outside the loop but that's not the case.

> Source/WebCore/page/EventHandler.cpp:317
> +        nextScrollBlock = currentEnclosingBox->containingBlock();

Why do you need the nextScrollBlock variable? Can't you just say
currentEnclosingBox = currentEnclosingBox->containingBlock() here?

>> Source/WebCore/page/EventHandler.cpp:-2640
>> -        dominantDirection = m_recentWheelEventDeltaTracker->dominantScrollGestureDirection();
> 
> where did all this code go?

What Tim said.

[Wenson Hsieh]

Comment on attachment 235613 [details]
Patch

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

Thank you for looking at my code, Tim and Simon! I've fixed these issues, and I'll have a new patch up in a bit.

>> Source/WebCore/ChangeLog:10
>> +        Reviewed by NOBODY (OOPS!).
> 
> This should go above the explanatory paragraph.

Got it -- fixed.

>> Source/WebCore/page/EventHandler.cpp:296
>> +static inline bool handleWheelEventInScrollableArea(Node* startNode, WheelEvent* wheelEvent, Element** stopElement)
> 
> Name could be slightly better, communicating the fact that it looks up the stack of enclosing scrollableAreas.

Got it. It's a little hard to name this one though :P. If "handleWheelEventInAppropriateEnclosingScrollableArea" isn't too long, I think I'd be a more fitting name since it implies we have do something to obtain the correct ScrollableArea before handling the wheel event.

>> Source/WebCore/page/EventHandler.cpp:299
>> +        return false;
> 
> Blank line below please.

Fixed.

>> Source/WebCore/page/EventHandler.cpp:300
>> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();
> 
> Is startNode guaranteed to have a renderer()?

If startNode doesn't have a renderer(), the above if statement should catch it and return false early.

>> Source/WebCore/page/EventHandler.cpp:303
>> +    RenderBox* currentEnclosingBox = initialEnclosingBox;
> 
> Blank line above please.

Fixed!

>> Source/WebCore/page/EventHandler.cpp:304
>> +    RenderBlock* nextScrollBlock;
> 
> You should declare this on first use. If declared here, I assume that you'll refer to it outside the loop but that's not the case.

I'm removing nextScrollBlock, but I'll keep that in mind. I'm keeping currentEnclosingBox outside though, since it starts with an initial value of initialEnclosingBox.

>> Source/WebCore/page/EventHandler.cpp:311
>> +                    *stopElement = currentEnclosingBox->element();
> 
> that's a lot of nesting :| can we make this flatter?

Flattened by 1 level by taking out the local variable platformEvent. Hopefully it's more readable.

>> Source/WebCore/page/EventHandler.cpp:317
>> +        nextScrollBlock = currentEnclosingBox->containingBlock();
> 
> Why do you need the nextScrollBlock variable? Can't you just say
> currentEnclosingBox = currentEnclosingBox->containingBlock() here?

Good point, fixed!

>>> Source/WebCore/page/EventHandler.cpp:-2640
>>> -        dominantDirection = m_recentWheelEventDeltaTracker->dominantScrollGestureDirection();
>> 
>> where did all this code go?
> 
> What Tim said.

Sorry about that -- I tried to handle scrolling in both axes in "handleWheelEventInScrollableArea", but after looking at the dominant scrolling issue, I realized doing so would break the fix to <rdar://problem/14758615> using dominantDirection. I've changed my code to deal with only one axis at a time, and also reinstated checking for dominant direction when scrolling.

>> Source/WebCore/rendering/RenderNamedFlowThread.cpp:378
>> +RenderBlock* RenderNamedFlowThread::fragmentFromCurrentlyScrollingBlockAsRenderBlock(RenderBlock* renderBlock, const IntPoint& absolutePoint, const RenderBox& flowedBox)
> 
> this name seems a bit weird since what the function does doesn't seem to have anything to do with scrolling?

Got it. "fragmentFromRenderBoxAsRenderBlock" sounds a bit clearer, I think.

[Wenson Hsieh]

Created attachment 235694 [details]
Patch

[Wenson Hsieh]

Comment on attachment 235694 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:293
> +static inline bool handleWheelEventInAppropriateEnclosingBoxForSingleAxis(Node* startNode, WheelEvent* wheelEvent, Element** stopElement, bool isVerticalAxis)

I considered using an enum for this instead of a bool flag, but the only existing enum that made sense was ScrollbarOrientation, which sounded like it should be used for scrollbar-specific purposes. Meanwhile, ScrollDirection sounds more accurate, but it's specific to all 4 scrolling directions, whereas I only need the orientation (left/right vs. up/down).

[Beth Dakin]

Comment on attachment 235694 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:290
> +    return scrollableArea->scroll(delta < 0 ? (isVerticalAxis ? ScrollUp : ScrollLeft) : (isVerticalAxis ? ScrollDown : ScrollRight), wheelGranularityToScrollGranularity(wheelEvent->deltaMode()), delta > 0 ? delta : -delta);

This is a little hard to read. Maybe you can pull some of those ternary operator evaluations out into well-named local variable?

>> Source/WebCore/page/EventHandler.cpp:293
>> +static inline bool handleWheelEventInAppropriateEnclosingBoxForSingleAxis(Node* startNode, WheelEvent* wheelEvent, Element** stopElement, bool isVerticalAxis)
> 
> I considered using an enum for this instead of a bool flag, but the only existing enum that made sense was ScrollbarOrientation, which sounded like it should be used for scrollbar-specific purposes. Meanwhile, ScrollDirection sounds more accurate, but it's specific to all 4 scrolling directions, whereas I only need the orientation (left/right vs. up/down).

I think you should consider adding a new enum. Something called ScrollGestureAxis or ScrollEventAxis or something along those lines?

> Source/WebCore/page/EventHandler.cpp:298
> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();

Is there any reason you want to use this as a pointer instead of a reference?

> Source/WebCore/page/EventHandler.cpp:306
> +        if (boxLayer && (platformEvent != nullptr ? boxLayer->handleWheelEvent(isVerticalAxis ? platformEvent->copyIgnoringHorizontalDelta() : platformEvent->copyIgnoringVerticalDelta()) : didScrollInScrollableAreaForSingleAxis(boxLayer, wheelEvent, isVerticalAxis))) {

This is very difficult to read. Again, I suggest using some well-named local variables for the ternary operator evaluations.

[Wenson Hsieh]

Comment on attachment 235694 [details]
Patch

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

Thanks for looking over my code! I'll have a fixed version up soon (hopefully it will be ready after this one)

>> Source/WebCore/page/EventHandler.cpp:290
>> +    return scrollableArea->scroll(delta < 0 ? (isVerticalAxis ? ScrollUp : ScrollLeft) : (isVerticalAxis ? ScrollDown : ScrollRight), wheelGranularityToScrollGranularity(wheelEvent->deltaMode()), delta > 0 ? delta : -delta);
> 
> This is a little hard to read. Maybe you can pull some of those ternary operator evaluations out into well-named local variable?

Good call. I agree that the nested ternary operators are a bit much -- I'll pull out the ScrollDirections into positiveDirection and negativeDirection before the return, so the first part "delta < 0 ? negativeDirection : positiveDirection" makes more sense.

>>> Source/WebCore/page/EventHandler.cpp:293
>>> +static inline bool handleWheelEventInAppropriateEnclosingBoxForSingleAxis(Node* startNode, WheelEvent* wheelEvent, Element** stopElement, bool isVerticalAxis)
>> 
>> I considered using an enum for this instead of a bool flag, but the only existing enum that made sense was ScrollbarOrientation, which sounded like it should be used for scrollbar-specific purposes. Meanwhile, ScrollDirection sounds more accurate, but it's specific to all 4 scrolling directions, whereas I only need the orientation (left/right vs. up/down).
> 
> I think you should consider adding a new enum. Something called ScrollGestureAxis or ScrollEventAxis or something along those lines?

That sounds good. I can use it in my scroll snapping manager as well, since it makes more sense in that context than ScrollbarOrientation.

>> Source/WebCore/page/EventHandler.cpp:298
>> +    RenderBox* initialEnclosingBox = &startNode->renderer()->enclosingBox();
> 
> Is there any reason you want to use this as a pointer instead of a reference?

Good catch. Changed to reference.

>> Source/WebCore/page/EventHandler.cpp:306
>> +        if (boxLayer && (platformEvent != nullptr ? boxLayer->handleWheelEvent(isVerticalAxis ? platformEvent->copyIgnoringHorizontalDelta() : platformEvent->copyIgnoringVerticalDelta()) : didScrollInScrollableAreaForSingleAxis(boxLayer, wheelEvent, isVerticalAxis))) {
> 
> This is very difficult to read. Again, I suggest using some well-named local variables for the ternary operator evaluations.

Changed the outer ternary operator to an if statement and local variable instead -- it's much more readable now.

[Wenson Hsieh]

Created attachment 235721 [details]
Patch

[Beth Dakin]

Comment on attachment 235721 [details]
Patch

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

Looks good!

> Source/WebCore/page/EventHandler.cpp:292
> +    return scrollableArea->scroll(delta < 0 ? negativeDirection : positiveDirection, wheelGranularityToScrollGranularity(wheelEvent->deltaMode()), delta > 0 ? delta : -delta);

So much more readable!!

[WebKit Commit Bot]

Comment on attachment 235721 [details]
Patch

Clearing flags on attachment: 235721

Committed r171862: <http://trac.webkit.org/changeset/171862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.