1068802 - APZ can get out of sync when mixing Promises and scrollIntoView

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jim Porter (:squib)]

Attachment: Test case

Attached is a simple test case that appends a bunch of elements and scrolls to one of them. If you open this page on B2G, scroll up a bit, then reload and try to tap the items, you'll see that the item that gets selected is actually well *above* your finger.

This bug, under slightly different circumstances, was also reported at bug 1068471 (over there, I think the slowness of the Promises makes it so you don't have to reload at all, but you *do* have to use insertBefore somewhere).

Comment 1

[Jayachandra Yakasiri]

refer for more information 1068471

Comment 2

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

Can repro, will investigate.

Comment 3

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

Attachment: Part 1 - Reset callback transform on scroll update acknowledgement

This fixes the problem. In the scenario I reproduced what was happening was that when the page loads, the APZ sends a repaint request at (0,0) while content is doing the scrollIntoView. Content sets the scroll offset to y=520, and so the repaint request fails, resulting in a callback transform of y=-520 getting stored. The APZ gets a NLU call with the y=520 scroll offset update and sends an acknowledgement, but since there's no repaint happening the callback transform remains at -520. Subsequent touch events and clicks are incorrectly transformed by 520. I think clearing the callback transform on the scroll offset update is the right thing to do here, because it means that content and the APZ are back in sync with respect to the scroll offset and any future touch events shouldn't get translated around.

Comment 4

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

Attachment: Part 2 - Ride-along logging patch

Misc logging stuff

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

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

Tricky stuff! It took me a while to convince myself that this is correct in various scenarios, including overflow:hidden frames, but it seems sound.

Comment 6

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

Thanks! It took me some time to convince myself it was correct as well. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/60a55c2a7551
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d94eebed4ed

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60a55c2a7551
https://hg.mozilla.org/mozilla-central/rev/2d94eebed4ed

Comment 8

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

Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

Approval Request Comment
[Feature/regressing bug #]: original async scrolling implementation
[User impact if declined]: in rare cases the device will get stuck in a state where clicks/gestures will get delivered to the wrong content. panning or zooming will clear the bad state
[Describe test coverage new/current, TBPL]: manual testing locally; we don't have a good framework to reproduce this scenario in automated testing
[Risks and why]: low risk, but it would probably benefit from some bake time on master before uplift. if it does have regressions, they would manifest as odd behaviour when content changes the scroll position via JS.
[String/UUID change made/needed]: none

Comment 9

[Jayachandra Yakasiri]

Thanks Karthik :)

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Does this impact desktop and mobile as well as B2G or only B2G?

Comment 11

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

Only B2G and Metro.

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

Approving this given the 2-3 day bake time, also NI :no-jun to see if there is more testing he could do once this lands on 2.1 or atleast keep an eye on any obvious fallouts that will happen due this landing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2465cb72f06
https://hg.mozilla.org/releases/mozilla-aurora/rev/a72ffda2ec0f

Comment 14

[No-Jun Park [:njpark]]

I checked the example attached to this bug, as well as various scroll/select UIs (Contacts, Calendar, Dialer, Settings, Gallery, etc.) but I did not see any issues with it.