Markus Stange [:mstange] Reporter
	Description • 8 years ago

STR:
 1. Go to https://webkit.org/demos/scroll-snap/ and scroll down to the demo that's rotated by 30 degrees.
 2. Scroll the rotated element all the way to the bottom corner.
 3. Try to scroll down some more, inside the element.

The last scroll should be handed off to the page. But nothing is scrolled.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 8 years ago

I see this with and without APZ, all the way back to current release (45).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 8 years ago

Actually the handoff does work without APZ, if you move the mouse a bit after scrolling the subframe to the end. I guess that starts a new wheel transaction. With APZ the same doesn't work, so this is an APZ regression.

	Botond Ballo [:botond]
	Comment 3 • 8 years ago

Hmm. The handoff works for me on Linux, on the latest nightly, with APZ enabled, with mouse wheel scrolling.

	Botond Ballo [:botond]
	Comment 4 • 8 years ago

Handoff works for me with touch events as well, although I'm not seeing momentum scrolling (flinging) after handoff - that, at least, is a bug.

	Botond Ballo [:botond]
	Comment 5 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/47323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47323/

	Botond Ballo [:botond]
	Comment 6 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #4)
> I'm not seeing
> momentum scrolling (flinging) after handoff - that, at least, is a bug.

So, there are two issues here:

  1) If the scroll position is already at the target snap point when the
     fingers are lifted after a touch, we were still starting a no-op 
     smooth scroll animation instead of a fling animation. The posted
     patch fixes this.

  2) If the rotated scroll frame is, say, scrolled to its far extent
     vertically but not horizontally, a pan that's downward from the
     point of view of the page will have a component that's horizontal
     from the point of view of the scroll frame. This will cause a bit
     of horizontal panning, and then when the finger is lifted, you'll
     snap back to the original corner snap point (or to the next
     snap point horizontally if you're panned far enough). Since you
     are snapping in this case, no fling (and thus no handoff) occurs.

     I think this behaviour is expected, but I'm willing to entertain
     arguments to the contrary. (Conceivably we could model a scroll
     snap as "consuming" part but not all of a delta, and hand off
     the unconsumed part.)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 8 years ago

Comment on attachment 8742573 [details]
MozReview Request: Bug 1265513 - Don't start a scroll snap animation if we're already at the destination. r=kats

https://reviewboard.mozilla.org/r/47323/#review44143

This patch is fine, but probably belongs on a new bug since it doesn't fix the issue Markus filed this bug for. If I understand your previous comment correctly, it sounds like you're suggesting this is expected behaviour and WONTFIX. That sounds plausible, however in this case the user is scrolled to the bottom-right of the rotated scrollframe, so even the horizontal component shouldn't be doing anything within that scrollframe and should get handed off. It might just be rounding error or something, but we should make sure.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 8 years ago

So what's happening in *this* bug is that even after the subframe is scrolled to the end, mY.CanScroll(aDelta) is returning true for that APZC inside [1]. So the subframe is picked as the target APZC, and PanGestureBlocks don't allow handoff [2], so all the inputs go to that subframe. The reason CanScroll(aDelta) is returning true instead of false appears to be rounding error. At [3] aDelta.value is 0.866028 and DisplacementWillOverscrollAmoutn(aDelta).value is 0.865967.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=4a137817e1f5#636
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=4a137817e1f5#721
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?rev=4a137817e1f5#462

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 8 years ago

The values inside DisplacementWillOverscrollAmount are:

origin 1100.000000 displacement 0.866028 compend 1600.000000 pagestart 0.000000 pageend 1600.000000

I guess storing 1600.866028 in a float rounds it to 1600.865967 because of precision issues. Reformulating that function a little bit seems to fix the problem but we might want to fix this more globally somehow, I'm not really sure.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 8 years ago

This reformulation of the function fixes the rounding error in this instance. Thoughts?

	Botond Ballo [:botond]
	Comment 11 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> This patch is fine, but probably belongs on a new bug since it doesn't fix
> the issue Markus filed this bug for. If I understand your previous comment
> correctly, it sounds like you're suggesting this is expected behaviour and
> WONTFIX. That sounds plausible, however in this case the user is scrolled to
> the bottom-right of the rotated scrollframe, so even the horizontal
> component shouldn't be doing anything within that scrollframe and should get
> handed off.

The behaviour that I was suggesting is expected, pertains to momentum scrolling only. That is, if, when you lift your finger, we need to snap, we won't also fling / momentum-scroll. I'll split the momentum scrolling patch and discussion out into a new bug like you suggested.

	Botond Ballo [:botond]
	Comment 12 • 8 years ago

Comment on attachment 8742823 [details] [diff] [review]
WIP

Review of attachment 8742823 [details] [diff] [review]:
-----------------------------------------------------------------

The reformulation is fine, but might we want to adjust the comparisons to be fuzzy (using COORDINATE_EPSILON) instead?

	Botond Ballo [:botond]
	Comment 13 • 8 years ago

Comment on attachment 8742573 [details]
MozReview Request: Bug 1265513 - Don't start a scroll snap animation if we're already at the destination. r=kats

Moved to bug 1266154 (the bug I split out for the momentum scrolling).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #12)
> The reformulation is fine, but might we want to adjust the comparisons to be
> fuzzy (using COORDINATE_EPSILON) instead?

Yeah, that works too, and sounds like the better fix :)

	Pulsebot
	Comment 16 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee179e178809

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ee179e178809