Report accurate Overscroll on handleWheel.

Report accurate Overscroll on handleWheel.

Report accurate Overscroll on handleWheel. Currently 
invalid ScrollResult is returned on overscroll.

compositor changes @
https://codereview.chromium.org/1203693003/

Bug=442859
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198007

[MuVen, 2015-06-22 15:35:00]

sataya.m@samsung.com changed reviewers:
+ ccameron@chromium.org, jdduke@chromium.org, majidvp@chromium.org

[MuVen, 2015-06-22 15:42:04]

sataya.m@samsung.com changed reviewers:
- ccameron@chromium.org

[MuVen, 2015-06-22 15:45:32]

Hi majid,

PTAL of the changes made to report overscroll on handleWheel. 

Thanks,

[majidvp, 2015-06-23 15:47:21]

On 2015/06/22 15:45:32, MuVen wrote:
> Hi majid,
> 
> PTAL of the changes made to report overscroll on handleWheel. 
> 
> Thanks,

Please add the bug number to the description.

The patch needs tests but on first glance it seem to be doing the right thing in
reporting 
overscroll amount for wheel.


I am mainly concerned how this is going to be used to support elastic overscroll
(rubberbanding) on Mac OSX.
In particular:
- How is the velocity is computed for wheel events? The CC implementation relies
on observing 
all wheel events and calculating velocity that way[1].  Are you planning to
implement this same logic on main?
- What is the plan to modify the InputScrollElasticityController to also handle
DidOverscroll from main?

[jdduke (slow), 2015-06-23 15:56:09]

jdduke@chromium.org changed reviewers:
+ ccameron@chromium.org

[jdduke (slow), 2015-06-23 15:56:09]

+ccameron for the Mac side of things.

I don't think we want wheel overscroll on Android (correct me if I'm wrong, but
I've never noticed this in Android views).

[ccameron, 2015-06-23 16:06:34]

This looks reasonable to me -- has it been manually tested on Mac?

On 2015/06/23 15:47:21, majidvp wrote:
> On 2015/06/22 15:45:32, MuVen wrote:
> > Hi majid,
> > 
> > PTAL of the changes made to report overscroll on handleWheel. 
> > 
> > Thanks,
> 
> Please add the bug number to the description.
> 
> The patch needs tests but on first glance it seem to be doing the right thing
in
> reporting 
> overscroll amount for wheel.
> 
> 
> I am mainly concerned how this is going to be used to support elastic
overscroll
> (rubberbanding) on Mac OSX.
> In particular:
> - How is the velocity is computed for wheel events? The CC implementation
relies
> on observing 
> all wheel events and calculating velocity that way[1].  Are you planning to
> implement this same logic on main?

There is no concept of velocity in events/gestures on Mac (there is one that is
computed by finite differences elsewhere).

As long as the ScrollResult being returned has correct "unusedDelta" parameters,
we're in good shape.

> - What is the plan to modify the InputScrollElasticityController to also
handle
> DidOverscroll from main?

That shouldn't need to be modified -- the result from the scroll event is passed
over to the impl thread and handled there.

There is a (possibly not obsolete) doc on the structure of Mac overscroll at:
https://docs.google.com/document/d/1ePfdAkOKh7DMc7UBOyyXrkmf_25e-d8zTJktAGHtc...

[bokan, 2015-06-23 16:20:45]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2015-06-23 16:20:45]

https://codereview.chromium.org/1195803003/diff/60001/Source/core/frame/RootF...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/core/frame/RootF...
Source/core/frame/RootFrameViewport.cpp:125: return ScrollResult(false, false,
viewScrollResult.unusedScrollDeltaX, viewScrollResult.unusedScrollDeltaY);
This is returning the layout viewport's overscroll, which can be consumed by the
visual viewport. You need to subtract out the usedLocationDelta (how much the
visual viewport scrolled)

https://codereview.chromium.org/1195803003/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1786: #if defined(WTF_OS_MACOSX)
This should be based on a setting rather than an #ifdef and the setting should
be turned on only for Mac. #ifdefs make testing more complicated.

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimator.cpp:98: result.didScrollY = (deltaY < 0 &&
maxForwardScrollDelta.height() > 0) || (deltaY > 0 &&
maxBackwardScrollDelta.height() > 0);
Since we're going to try to scroll now anyways, it'd be more clear to save the
scrollableArea scrollPosition here and set didScrollX/Y below based on whether
the scrollPosition changed.

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:304: return ScrollResult(false, false,
wheelEvent.deltaX(), wheelEvent.deltaY());
Doesn't this mean that a browser zoom (ctrl + wheel) will now trigger
overscroll?

[bokan, 2015-06-23 16:23:09]

On 2015/06/23 15:56:09, jdduke wrote:
> +ccameron for the Mac side of things.
> 
> I don't think we want wheel overscroll on Android (correct me if I'm wrong,
but
> I've never noticed this in Android views).

I mentioned in code, IMO the decision to report overscroll should be based on a
setting which only Mac would enable. The plumbing should otherwise be
platform-agnostic.

[MuVen, 2015-06-23 17:41:17]

Hi All,

Thanks for the review comments. I have tested manually on the mac, and
its works fine. I shall modify and upload the patch with tests as per 
the above mentioned review comments.

Thanks,
~MuVen.

[MuVen, 2015-06-25 04:06:43]

Patchset #6 (id:100001) has been deleted

[MuVen, 2015-06-25 04:19:17]

Patchset #7 (id:140001) has been deleted

[MuVen, 2015-06-25 04:19:23]

Patchset #6 (id:120001) has been deleted

[MuVen, 2015-06-25 07:37:15]

Patchset #8 (id:200001) has been deleted

[MuVen, 2015-06-25 09:24:53]

Patchset #5 (id:80001) has been deleted

[MuVen, 2015-06-25 09:25:03]

Patchset #5 (id:160001) has been deleted

[MuVen, 2015-06-25 09:25:11]

Patchset #5 (id:180001) has been deleted

[MuVen, 2015-06-25 09:25:26]

Patchset #5 (id:220001) has been deleted

[MuVen, 2015-06-25 12:55:52]

PTAL.

https://codereview.chromium.org/1195803003/diff/60001/Source/core/frame/RootF...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/core/frame/RootF...
Source/core/frame/RootFrameViewport.cpp:125: return ScrollResult(false, false,
viewScrollResult.unusedScrollDeltaX, viewScrollResult.unusedScrollDeltaY);
On 2015/06/23 16:20:45, bokan wrote:
> This is returning the layout viewport's overscroll, which can be consumed by
the
> visual viewport. You need to subtract out the usedLocationDelta (how much the
> visual viewport scrolled)

Done.

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimator.cpp:98: result.didScrollY = (deltaY < 0 &&
maxForwardScrollDelta.height() > 0) || (deltaY > 0 &&
maxBackwardScrollDelta.height() > 0);
On 2015/06/23 16:20:45, bokan wrote:
> Since we're going to try to scroll now anyways, it'd be more clear to save the
> scrollableArea scrollPosition here and set didScrollX/Y below based on whether
> the scrollPosition changed.

Done.

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1195803003/diff/60001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:304: return ScrollResult(false, false,
wheelEvent.deltaX(), wheelEvent.deltaY());
On 2015/06/23 16:20:45, bokan wrote:
> Doesn't this mean that a browser zoom (ctrl + wheel) will now trigger
> overscroll?

Done. True, i checked it, it wont trigger overscroll. Thanks for pointing out.

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:99: ScrollResult viewScrollResult =
layoutViewport().handleWheel(event);
bokan@, handleWheel should be triggered, irrespective of isScrollable() to
calculate unUsedDelta. wdyt ? Example: 

https://css-tricks.com/examples/WebKitScrollbars/ in this case as the
layoutViewport is not scrollable(reason: NotScrollableNoOverflow), due to which
unusedDelta is not calculated.

[bokan, 2015-06-25 14:54:36]

https://codereview.chromium.org/1195803003/diff/240001/Source/platform/scroll...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1195803003/diff/240001/Source/platform/scroll...
Source/platform/scroll/ScrollAnimator.cpp:95: IntPoint currentPos =
m_scrollableArea->scrollPosition();
Sorry, I (slightly) misled you before. We can't just compare the position since
if we use smooth scrolling (i.e. animated scroll) then we wont see a difference.
I think that's why the old code was checking against the max/min position.

But actually, I think we can remove this altogether since the userScroll calls
below will return true/false in the ScrollResultOneDimensional.didScroll. Just
fill the result.didScrollX/Y values from that.

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:99: ScrollResult viewScrollResult =
layoutViewport().handleWheel(event);
On 2015/06/25 12:55:51, MuVen wrote:
> bokan@, handleWheel should be triggered, irrespective of isScrollable() to
> calculate unUsedDelta. wdyt ? Example: 
> 
> https://css-tricks.com/examples/WebKitScrollbars/ in this case as the
> layoutViewport is not scrollable(reason: NotScrollableNoOverflow), due to
which
> unusedDelta is not calculated. 

Sounds fine to me - it's actually an improvement - as long as all the tests
pass.

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return ScrollResult(false, false,
viewScrollResult.unusedScrollDeltaX - usedLocationDelta.x(),
viewScrollResult.unusedScrollDeltaY - usedLocationDelta.y());
This still looks wrong to me, I think you have the negatives mixed up -
admittedly they're very confusing here.

Digging through the wheel handing code, it seems scrolling down on a page will
generate PlatformWheelEvents with a negative delta.
ScrollAnimator::handleWheeEvent negates the delta, calls userScroll, then
negates the result so the ScrollResult.unusedScrollDelta is the unused *event*
delta. i.e. calling layoutViewport().handleWheel with WheelEvent{0, -10}, say it
scrolls the page down 2px, viewScrollResult.unusedScrollDeltaY == -8. This is
why we negate the locationDelta above. So this should be
-viewScrollResult.unusedScrollDelta - usedLocationDelta.

Unfortunately, it looks like the ScrollResult returned by this method has
different semantics than the base class version in ScrollableArea. That version
returns the unused portion of the PlatformWheelEvent (i.e. negative amount
remaining). This method will return the amount of *applied* delta that was
unused (i.e. positive amount remaining). We should definitely fix that (I think
it makes sense to make it always return the amount of unused applied delta) but
lets do that in a separate patch. Please leave a TODO and/or file a bug.

P.S. When testing your changes, make sure you're using
--disable-threaded-scrolling, otherwise you'll be scrolling on the compositor
and not testing these changes.

[MuVen, 2015-06-25 16:36:41]

Patchset #9 (id:320001) has been deleted

[MuVen, 2015-06-25 17:05:36]

bokan, PTAL.

As mentioned i have merged the codepaths in RootFrameViewport::handleWheel

https://codereview.chromium.org/1195803003/diff/240001/Source/platform/scroll...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1195803003/diff/240001/Source/platform/scroll...
Source/platform/scroll/ScrollAnimator.cpp:95: IntPoint currentPos =
m_scrollableArea->scrollPosition();
On 2015/06/25 14:54:35, bokan wrote:
> Sorry, I (slightly) misled you before. We can't just compare the position
since
> if we use smooth scrolling (i.e. animated scroll) then we wont see a
difference.
> I think that's why the old code was checking against the max/min position.
> 
> But actually, I think we can remove this altogether since the userScroll calls
> below will return true/false in the ScrollResultOneDimensional.didScroll. Just
> fill the result.didScrollX/Y values from that.

Done.

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:99: ScrollResult viewScrollResult =
layoutViewport().handleWheel(event);
On 2015/06/25 14:54:35, bokan wrote:
> On 2015/06/25 12:55:51, MuVen wrote:
> > bokan@, handleWheel should be triggered, irrespective of isScrollable() to
> > calculate unUsedDelta. wdyt ? Example: 
> > 
> > https://css-tricks.com/examples/WebKitScrollbars/ in this case as the
> > layoutViewport is not scrollable(reason: NotScrollableNoOverflow), due to
> which
> > unusedDelta is not calculated. 
> 
> Sounds fine to me - it's actually an improvement - as long as all the tests
> pass.

Done.

https://codereview.chromium.org/1195803003/diff/260001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return ScrollResult(false, false,
viewScrollResult.unusedScrollDeltaX - usedLocationDelta.x(),
viewScrollResult.unusedScrollDeltaY - usedLocationDelta.y());
On 2015/06/25 14:54:35, bokan wrote:
> This still looks wrong to me, I think you have the negatives mixed up -
> admittedly they're very confusing here.
> 
> Digging through the wheel handing code, it seems scrolling down on a page will
> generate PlatformWheelEvents with a negative delta.
> ScrollAnimator::handleWheeEvent negates the delta, calls userScroll, then
> negates the result so the ScrollResult.unusedScrollDelta is the unused *event*
> delta. i.e. calling layoutViewport().handleWheel with WheelEvent{0, -10}, say
it
> scrolls the page down 2px, viewScrollResult.unusedScrollDeltaY == -8. This is
> why we negate the locationDelta above. So this should be
> -viewScrollResult.unusedScrollDelta - usedLocationDelta.
> 
> Unfortunately, it looks like the ScrollResult returned by this method has
> different semantics than the base class version in ScrollableArea. That
version
> returns the unused portion of the PlatformWheelEvent (i.e. negative amount
> remaining). This method will return the amount of *applied* delta that was
> unused (i.e. positive amount remaining). We should definitely fix that (I
think
> it makes sense to make it always return the amount of unused applied delta)
but
> lets do that in a separate patch. Please leave a TODO and/or file a bug.
> 
> P.S. When testing your changes, make sure you're using
> --disable-threaded-scrolling, otherwise you'll be scrolling on the compositor
> and not testing these changes.

Done. filed bug @ crbug.com/504389.

[bokan, 2015-06-25 17:50:31]

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:105: // Move the location by the
negative of the remaining scroll delta.
Nit: Can you elaborate on this comment so that future generations don't have to
do the digging we did? e.g. "The delta in PlatformWheelEvent is negative when
scrolling the wheel towards the user so negate it to get the scroll delta that
should be applied to the page"

Also, a note above that the unusedScrollDelta in viewScrollResult is negative
(and incorrect) is also probably warranted.

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return ScrollResult(didScrollX,
didScrollY, viewScrollResult.unusedScrollDeltaX - usedLocationDelta.x(),
viewScrollResult.unusedScrollDeltaY - usedLocationDelta.y());
Is this working as expected? AFAICT, the unusedScrollDelta will be negative for
a downwards scroll but usedLocationDelta will be positive, so you'll get a
larger negative overscroll...

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Sett...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Sett...
Source/core/frame/Settings.in:343: elasticOverscrollEnabled initial=false
elastic overscroll is implemented in the CC so this is a bit misleading. IMO, it
should be called reportWheelOverscroll

https://codereview.chromium.org/1195803003/diff/350001/Source/core/input/Even...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/input/Even...
Source/core/input/EventHandler.cpp:2267: // ElasticOverscroll should be applied
even on non-Scrollable axis.
This distinction seems strange to me, we shouldn't have a difference at the
Blink level here. If we don't want to have an overscroll effect on unscrollable
axes, that should be done at the implementation - Blink should pass all the
overscroll information along anyway.

Could we move the scrollability check in the Android block to CC? That would
obviously have to be done in a separate patch (chromium side). For now the
simplest thing might just be to move the zeroing out of unscrollable delta for
Android to handleGestureScrollUpdate so it's not dependent on this flag.

[MuVen, 2015-06-26 09:55:45]

PTAL.

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:105: // Move the location by the
negative of the remaining scroll delta.
On 2015/06/25 17:50:31, bokan wrote:
> Nit: Can you elaborate on this comment so that future generations don't have
to
> do the digging we did? e.g. "The delta in PlatformWheelEvent is negative when
> scrolling the wheel towards the user so negate it to get the scroll delta that
> should be applied to the page"
> 
> Also, a note above that the unusedScrollDelta in viewScrollResult is negative
> (and incorrect) is also probably warranted.

Done.

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return ScrollResult(didScrollX,
didScrollY, viewScrollResult.unusedScrollDeltaX - usedLocationDelta.x(),
viewScrollResult.unusedScrollDeltaY - usedLocationDelta.y());
On 2015/06/25 17:50:31, bokan wrote:
> Is this working as expected? AFAICT, the unusedScrollDelta will be negative
for
> a downwards scroll but usedLocationDelta will be positive, so you'll get a
> larger negative overscroll...

You are right on this. i was negating the unusedDelta in the render_widget(which
i have noticed, on debugging, it was a mistake sorry:(). Changed as you have
suggested. Negating here itself. Will check the bug 504389 to ensure unusedDelta
state remains same everywhere. Thanks.

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Sett...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/frame/Sett...
Source/core/frame/Settings.in:343: elasticOverscrollEnabled initial=false
On 2015/06/25 17:50:31, bokan wrote:
> elastic overscroll is implemented in the CC so this is a bit misleading. IMO,
it
> should be called reportWheelOverscroll

Done.

https://codereview.chromium.org/1195803003/diff/350001/Source/core/input/Even...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1195803003/diff/350001/Source/core/input/Even...
Source/core/input/EventHandler.cpp:2267: // ElasticOverscroll should be applied
even on non-Scrollable axis.
On 2015/06/25 17:50:31, bokan wrote:
> This distinction seems strange to me, we shouldn't have a difference at the
> Blink level here. If we don't want to have an overscroll effect on
unscrollable
> axes, that should be done at the implementation - Blink should pass all the
> overscroll information along anyway.
> 
> Could we move the scrollability check in the Android block to CC? That would
> obviously have to be done in a separate patch (chromium side). For now the
> simplest thing might just be to move the zeroing out of unscrollable delta for
> Android to handleGestureScrollUpdate so it's not dependent on this flag.

Done.

[MuVen, 2015-06-26 10:47:22]

Patchset #13 (id:410001) has been deleted

[bokan, 2015-06-26 17:40:41]

Thanks for your patience, lgtm!

https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7854: // On disabling elasticoverscroll,
overscroll is not reported on MouseWheel.
Nit: s/elasticoverscroll/reportsWheelOverscroll

[MuVen, 2015-06-29 13:27:16]

sataya.m@samsung.com changed reviewers:
+ pdr@chromium.org

[MuVen, 2015-06-29 13:27:17]

Pdr, 

PTAL. Need Owners Approval.

Thanks,
MuVen.

https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7854: // On disabling elasticoverscroll,
overscroll is not reported on MouseWheel.
On 2015/06/26 17:40:41, bokan (OOO until Jul 13) wrote:
> Nit: s/elasticoverscroll/reportsWheelOverscroll

Done.

[pdr., 2015-06-29 17:18:22]

On 2015/06/29 at 13:27:17, sataya.m wrote:
> Pdr, 
> 
> PTAL. Need Owners Approval.
> 
> Thanks,
> MuVen.
> 
>
https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
> File Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/1195803003/diff/430001/Source/web/tests/WebFr...
> Source/web/tests/WebFrameTest.cpp:7854: // On disabling elasticoverscroll,
overscroll is not reported on MouseWheel.
> On 2015/06/26 17:40:41, bokan (OOO until Jul 13) wrote:
> > Nit: s/elasticoverscroll/reportsWheelOverscroll
> 
> Done.

LGTM

[MuVen, 2015-06-29 18:11:05]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2015-06-29 18:11:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/1195803003/#ps490001
(title: "fixed unittest failures")

[commit-bot: I haz the power, 2015-06-29 18:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195803003/490001

[commit-bot: I haz the power, 2015-06-29 18:15:18]

Committed patchset #16 (id:490001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198007