Bug 135195 - Refactor EventHandler to call ScrollAnimator::handleWheelEvent for overflow scrolling
Summary: Refactor EventHandler to call ScrollAnimator::handleWheelEvent for overflow s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 135244
Blocks: 134283
  Show dependency treegraph
 
Reported: 2014-07-23 06:37 PDT by Wenson Hsieh
Modified: 2014-07-31 10:16 PDT (History)
13 users (show)

See Also:


Attachments
Patch (35.57 KB, patch)
2014-07-23 14:06 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (9.17 KB, patch)
2014-07-24 11:26 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (1.09 MB, application/zip)
2014-07-24 13:28 PDT, Build Bot
no flags Details
Patch (10.04 KB, patch)
2014-07-24 13:55 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (9.33 KB, patch)
2014-07-24 16:02 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (516.32 KB, application/zip)
2014-07-24 17:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (598.57 KB, application/zip)
2014-07-24 19:36 PDT, Build Bot
no flags Details
Patch (10.11 KB, patch)
2014-07-25 10:06 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (481.57 KB, application/zip)
2014-07-25 11:06 PDT, Build Bot
no flags Details
Patch (10.55 KB, patch)
2014-07-25 11:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (10.55 KB, patch)
2014-07-25 11:43 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (9.85 KB, patch)
2014-07-28 13:59 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (512.92 KB, application/zip)
2014-07-28 15:10 PDT, Build Bot
no flags Details
Patch (10.39 KB, patch)
2014-07-29 09:53 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (11.14 KB, patch)
2014-07-29 18:12 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2014-07-23 06:37:55 PDT
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.
Comment 1 Wenson Hsieh 2014-07-23 14:06:18 PDT
Created attachment 235375 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-07-23 14:14:38 PDT
Comment on attachment 235375 [details]
Patch

Should figure out way to do this without #ifdefs.
Comment 3 Wenson Hsieh 2014-07-24 11:26:26 PDT
Created attachment 235438 [details]
Patch
Comment 4 Build Bot 2014-07-24 13:28:22 PDT
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
Comment 5 Build Bot 2014-07-24 13:28:25 PDT
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
Comment 6 Wenson Hsieh 2014-07-24 13:55:38 PDT
Created attachment 235457 [details]
Patch
Comment 7 WebKit Commit Bot 2014-07-24 13:58:20 PDT
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.
Comment 8 Wenson Hsieh 2014-07-24 16:02:52 PDT
Created attachment 235470 [details]
Patch
Comment 9 Build Bot 2014-07-24 17:34:52 PDT
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
Comment 10 Build Bot 2014-07-24 17:34:56 PDT
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
Comment 11 Build Bot 2014-07-24 19:36:09 PDT
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
Comment 12 Build Bot 2014-07-24 19:36:13 PDT
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
Comment 13 Jon Lee 2014-07-24 20:36:09 PDT
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.
Comment 14 Wenson Hsieh 2014-07-25 10:06:04 PDT
Created attachment 235523 [details]
Patch
Comment 15 Wenson Hsieh 2014-07-25 10:12:09 PDT
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.
Comment 16 Build Bot 2014-07-25 11:06:30 PDT
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
Comment 17 Build Bot 2014-07-25 11:06:34 PDT
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
Comment 18 Wenson Hsieh 2014-07-25 11:36:12 PDT
Created attachment 235530 [details]
Patch
Comment 19 Wenson Hsieh 2014-07-25 11:43:39 PDT
Created attachment 235533 [details]
Patch
Comment 20 Wenson Hsieh 2014-07-28 13:59:37 PDT
Created attachment 235613 [details]
Patch
Comment 21 Build Bot 2014-07-28 15:10:52 PDT
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
Comment 22 Build Bot 2014-07-28 15:10:57 PDT
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
Comment 23 Tim Horton 2014-07-28 16:59:29 PDT
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?
Comment 24 Simon Fraser (smfr) 2014-07-28 17:06:12 PDT
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.
Comment 25 Wenson Hsieh 2014-07-29 09:41:23 PDT
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.
Comment 26 Wenson Hsieh 2014-07-29 09:53:22 PDT
Created attachment 235694 [details]
Patch
Comment 27 Wenson Hsieh 2014-07-29 10:02:19 PDT
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).
Comment 28 Beth Dakin 2014-07-29 16:04:35 PDT
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.
Comment 29 Wenson Hsieh 2014-07-29 18:08:38 PDT
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.
Comment 30 Wenson Hsieh 2014-07-29 18:12:14 PDT
Created attachment 235721 [details]
Patch
Comment 31 Beth Dakin 2014-07-30 15:31:48 PDT
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!!
Comment 32 WebKit Commit Bot 2014-07-31 10:16:28 PDT
Comment on attachment 235721 [details]
Patch

Clearing flags on attachment: 235721

Committed r171862: <http://trac.webkit.org/changeset/171862>
Comment 33 WebKit Commit Bot 2014-07-31 10:16:34 PDT
All reviewed patches have been landed.  Closing bug.