730985 - Scroll does not snap back if we get preventDefault in java halfway through a pan

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Wesley Johnston (:wesj)]

Touch events don't work well with async scrolling. There are times when we don't hear back from Gecko quickly about whether or not to pan a page. In those cases, we favor panning rather than waiting for Gecko. However, when we do make this mistake and finally do hear back from Gecko, we freeze sending events to the PanZoomController, which can result in the page not snapping to a sensible position.

Alternatively, if we fail during a pinch zoom, it can leave the user with a zoomed page thats very tough to fix (since touch events will prevent them from pinch zooming back out, and I've worked hard to make them not fail often).

I think we need to be a bit smarter here.

1.) Store the viewport metrics when the users any pan
2.) If we receive preventDefault, ignore it for the remainder of this touch session
3.) When the touch session ends, restore the original viewportmetrics.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Seems like a good time to drag kats back into this fun stuff. This stores the initial viewport when we start a drag. If, on touchup, we we're supposed to prevent panning, we snap back to those original metrics.

There are other more complicated things we can do here. But this seems like a simple first step that I think will cover 90% of the problems. Its getting hard to trigger here as well (yay!), so I'm not sure its worth worrying about.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 603409 [details] [diff] [review]
Patch

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

Sorry for the huge delay, but now that maple has landed I can review this more thoroughly.

The main problem I see with this patch is that if there's a page that intercepts touch events and uses them to do scrolling on its own by calling window.scrollTo, we'll clobber that scroll as soon as the user lifts their finger. It seems like we want to put back initialViewportMetrics as soon as we get the preventPanning(true) call instead, since presumably that's the first point at which we know the page is hijacking that event. Bonus points if you can somehow add two tests to robocop that test this distinction, although I don't know how easy that will be. Might not be too hard if you use mDriver.getScrollHeight() to check the scroll position.

Also, I would really really like a cleanup patch that renames all the class variables to follow the mVariableName convention (in the spirit of micro-commiting, this should be a separate patch from the main one).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Better STR?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

It seems like this should be fixed (as best we can do) with bug 742019. We wait a reasonable period for the page to respond, and if it's late we throw away the preventDefault. Trying to handle a late preventDefault without seriously borking the rest of the code and state is IMO not worth it.

Comment 6

[Wesley Johnston (:wesj)]

I agree. As best I remember the new stuff, if we get a late preventDefault now, we basically ignore it, which should "fix" this. I'm gonna mark this WFM.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1) go to m.google.com/maps
2) while the page is loading pan to the left.
3) if you don't see the issue, try to zoom and pan and overscroll

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

qawanted - ; STRs in place (based on comments in IRC from wesj)