Closed Bug 1265513 Opened 4 years ago Closed 4 years ago

Not handing off wheel scroll at the bottom of a transformed scrollable subframe

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

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.
I see this with and without APZ, all the way back to current release (45).
Whiteboard: [gfx-noted]
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.
Hmm. The handoff works for me on Linux, on the latest nightly, with APZ enabled, with mouse wheel scrolling.
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.
(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.)
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.
Attachment #8742573 - Flags: review?(bugmail.mozilla) → review+
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
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.
Attached patch WIP (obsolete) — Splinter Review
This reformulation of the function fixes the rounding error in this instance. Thoughts?
(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.
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?
See Also: → 1266154
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).
Attachment #8742573 - Attachment is obsolete: true
(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 :)
Assignee: nobody → bugmail.mozilla
Attachment #8743414 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/ee179e178809
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.