944047 - Settings app with APZ sometimes deletes part of the display port

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #940706 +++

Spliting out the two issues mentioned in bug 940706.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, any chance you can look at this one while Benoit is looking at the other part of the original bug?

Comment 2

[Chris Lord [:cwiiis]]

Yup, I'll take it.

Comment 3

[Chris Lord [:cwiiis]]

So this seems to happen because the scroll events that happen on sub-frames get swallowed before they reach TabChild, and so APZC isn't updated correctly and its scroll position gets out of sync.

If I set the event listener to capture, it fixes the issue (and some others), but it's also quite janky, likely because the check that stops APZC-initiated scroll events from being echoed back to the APZC only works for the root frame currently.

It also feels like there's something conflicting with the APZC, it makes me wonder if BrowserElementPanning is being incorrectly used, but this last part is just conjecture.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #3)
> It also feels like there's something conflicting with the APZC, it makes me
> wonder if BrowserElementPanning is being incorrectly used, but this last
> part is just conjecture.

I've verified that this conjecture is correct, and is responsible for the scrolling feeling much worse than it should - it looks like the !docShell.asyncPanZoomEnabled check is either not working, or checks too early.

Comment 5

[Chris Lord [:cwiiis]]

Further investigation, it seems that TabChild::IsAsyncPanZoomEnabled is called via BrowserElementPanning.js before TabChild::InitRenderingState is called, so mZooming retains its default value (apz disabled).

Just to clarify things, there are two issues that result in this bug:

1- BrowserElementPanning is incorrectly enabling touch-handling. This conflicts with the APZC touch handling and causes scrolling to get set to weird offsets suddenly, or to animate into different positions to those set by APZC

2- TabChild isn't capturing the scroll event, and something swallows it before it reaches that point for sub-frames, meaning the APZC's idea of the scroll position is never updated by content (so when 1 happens, or scrollTo is called by some other means, we asynchronously scroll to where APZC thinks we should be, but the displayport remains unchanged, so we see blank content)

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Don't run BrowserElementPanning.js in the preload function

This still preloads BrowserElementPanning, but doesn't run it until after initialisation, due to the TabChild::IsAsyncPanZoomEnabled dependency.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Capture scroll events in TabChild

Capture scroll events in TabChild so that there's no possibility of a scroll position being set, but APZC not being made aware of it.

Comment 8

[Chris Lord [:cwiiis]]

I'll finish part 3 tomorrow (hopefully), which will involve implementing the mLastMetrics storage for sub-frames instead of just root frames.

Comment 9

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

Comment on attachment 8341903 [details] [diff] [review]
Part 2 - Capture scroll events in TabChild

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

Kill the printfs. r=me for the one-line patch

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8341900 [details] [diff] [review]
Part 1 - Don't run BrowserElementPanning.js in the preload function

I think you still want to preload it but just delay when it is inited by moving around http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#603 in BrowserElementChild.js.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug944047.panning.patch

I was thinking more of something like the attachment for part 1.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #11)
> Created attachment 8342214 [details] [diff] [review]
> bug944047.panning.patch
> 
> I was thinking more of something like the attachment for part 1.

With part2 and the attachment I made. I can't reproduce bug 943882 anymore, let's dup it.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #12)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #11)
> > Created attachment 8342214 [details] [diff] [review]
> > bug944047.panning.patch
> > 
> > I was thinking more of something like the attachment for part 1.
> 
> With part2 and the attachment I made. I can't reproduce bug 943882 anymore,
> let's dup it.

I also assume that the part that touch BrowserElementPanning.js will fix bug 942701.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #14)
> *** Bug 943882 has been marked as a duplicate of this bug. ***

Seems like it also resolve bug 943858.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 8342214 [details] [diff] [review]
bug944047.panning.patch

Verified this works and giving my r+ as discussed on IRC.

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Store FrameMetrics for sub-frames as well as root frames

This seems to work for me. As far as I know, the APZ tree manager only allows scrolling of one frame at a time, so we can just store whatever FrameMetrics was last sent.

I think the mechanism itself is still pretty flimsy, as if we get multiple updates before the scroll event comes back, we'll still erroneously send scroll events back to the APZC, but this works well enough in practice.

We should fix this more definitively at some point.

Comment 19

[Chris Lord [:cwiiis]]

This doesn't depend on bug 940706.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8342381 [details] [diff] [review]
Part 3 - Store FrameMetrics for sub-frames as well as root frames

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

I don't think this change is correct. Code in TabChild assumes that mLastMetrics corresponds to the root frame.

In particular, in HandlePossibleViewportChange(), a local FrameMetrics variable is initialized with mLastMetrics [1], and then ProcessUpdateFrame() (which updates the root frame) is called with that local metrics [2]. In between [1] and [2], a lot of the fields of the local metrics are set to correct values for the root frame, but not all (notably, not mScrollOffset and mZoom).

So, if the following sequence of events were to arise:
  1) Scroll a subframe, so TabChild receives a "scroll" event and sets mLastMetrics
     to the subframe's metrics
  2) Change the screen orientation so that HandlePossibleViewportChange() is called
then I believe the root frame would be updated with the mScrollOffset and mZoom of the subframe.

Instead, perhaps we can make mLastMetrics a map with the pres shell id + scroll id as the key and the FrameMetrics as the value? Or, since we really just need the scroll offset for subframes, we could keep a mLastMetrics being the root metrics and have a map with just scroll offsets as values?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#609
[2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#663

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> Instead, perhaps we can make mLastMetrics a map with the pres shell id +
> scroll id as the key and the FrameMetrics as the value? Or, since we really
> just need the scroll offset for subframes, we could keep a mLastMetrics
> being the root metrics and have a map with just scroll offsets as values?

It just occurred to me that perhaps we don't need a map - I think we can do what you're proposing (keep track of the scroll offset of the last thing that was scrolled), just do it in a different variable than mFrameMetrics which is assumed to correspond to the root layer.

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #21)
> (In reply to Botond Ballo [:botond] from comment #20)
> > Instead, perhaps we can make mLastMetrics a map with the pres shell id +
> > scroll id as the key and the FrameMetrics as the value? Or, since we really
> > just need the scroll offset for subframes, we could keep a mLastMetrics
> > being the root metrics and have a map with just scroll offsets as values?
> 
> It just occurred to me that perhaps we don't need a map - I think we can do
> what you're proposing (keep track of the scroll offset of the last thing
> that was scrolled), just do it in a different variable than mFrameMetrics
> which is assumed to correspond to the root layer.

This is the same conclusion that I'd come to, I'll attach a patch when I get to a good stopping point with what I'm currently looking at.

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Store FrameMetrics for sub-frames as well as root frames (v2)

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8343242 [details] [diff] [review]
Part 3 - Store FrameMetrics for sub-frames as well as root frames (v2)

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

Looks good, thanks!

Comment 25

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/015be0f83e7a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1689fa499c1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2722412248da

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/015be0f83e7a
https://hg.mozilla.org/mozilla-central/rev/1689fa499c1c
https://hg.mozilla.org/mozilla-central/rev/2722412248da