780395 - B2G: Listen for scrollTo calls and metrics being clobbered

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Doug Sherk (:drs) (inactive)]

Currently, if we're async panning and zooming and content requests a scrollTo or metrics are otherwise updated in such a way that our old ones are no longer valid, we just write over them.

We need to:
a) Listen for the scroll offset being updated and refresh our metrics.
b) Re-validate the current zoom level and scroll offset whenever the CSS rect is updated.
c) Re-validate the current scroll offset whenever the screen size changes (ex. due to a rotation)

Comment 1

[Joe Drew (not getting mail)]

What do we do for fennec here? If we don't handle this on Fennec, we similarly don't need to for basecamp.

Comment 2

[Doug Sherk (:drs) (inactive)]

Fennec has this functionality.

Comment 3

[Andrew Overholt [:overholt]]

cjones says we shipped fennec without this in the past so not a Basecamp blocker.

Comment 5

[Justin Lebar (not reading bugmail)]

This is a web-facing regression (see bug 786780); I'm surprised we'd ship a browser without this.

But anyway, we need this for bug 785663, which blocks.

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1, Listen for scrollTo calls and metrics being clobbered

This needs additional work, both in this bug and elsewhere. See bug 786808.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1, Listen for scrollTo calls and metrics being clobbered

Better patch.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 656608 [details] [diff] [review]
Part 1, Listen for scrollTo calls and metrics being clobbered

The document size could have changed too, right?  I think we should update all the rendering state here.

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> Comment on attachment 656608 [details] [diff] [review]
> Part 1, Listen for scrollTo calls and metrics being clobbered
> 
> The document size could have changed too, right?  I think we should update
> all the rendering state here.

We already handle that here:
https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#971

However, you're right, there are a _lot_ of problems with things like orientation flips causing us to go off screen if we're zoomed out. I will fix these problems up in a follow-up in this bug (the parts b and c I listed above).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 656608 [details] [diff] [review]
Part 1, Listen for scrollTo calls and metrics being clobbered


>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+  } else {
>+    // No paint was requested, but we got one anyways. One possible cause of this
>+    // is that content could have fired a scrollTo(). In this case, we should take
>+    // the new scroll offset.

Note here that document/viewport changes are handled elsewhere.

It makes sense to me to consolidate that code, but since particular deficiency is causing serious problems we can get it landed now.  Do ensure the followup gets done ;).

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since this blocks a blocker, we need to make this blocking too unfortunately.

Comment 12

[Chris Lee [:clee]]

Marking as P1 as this makes bug 785663 simple to fix per jlebar.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Chris Lee [:clee] from comment #12)
> Marking as P1 as this makes bug 785663 simple to fix per jlebar.

Hi, Chris.

The WebAPI priorities are internal priorities that the WebAPI team agreed to in a meeting yesterday.  They're not product or any other team's priorities.

Did you speak with Andrew or Jonas before re-prioritizing this bug?  It was marked as a P0 because the team believed that it was not part of the WebAPI umbrella.  I'm going to set it back to that, because that's the last I heard us agree to.

If product wants its own set of priorities, you are more than welcome to add your own whiteboard flag.  In fact, I bet product could even use the Bugzilla priority flag.  Shiny.  :)

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1, Listen for scrollTo calls and metrics being clobbered

r+ carried, fixed review comment.

Comment 15

[Justin Lebar (not reading bugmail)]

Should I wait for a try run, or should I trust you on this?

Comment 16

[Doug Sherk (:drs) (inactive)]

It's been given try runs before, but combined with other bugs. I made sure it built but there's nothing that tests this, so it should be fine.

Comment 17

[Justin Lebar (not reading bugmail)]

Good enough for me!

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Doug Sherk (:drs) (inactive)]

The basecamp-blocking part of this has landed, removing blocking-basecamp.

Comment 20

[Botond Ballo [:botond]]

I think bug 895905 at least partially addresses this issue.

Comment 21

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

A patch landed here, marking this bug fixed. As Botond said, bug 895905 addressed some of this, and bug 921020 covers the remaining bits.