942189 - Pages with slow loading sub frames (or other resources) can't pan or zoom by touch

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jim Mathies [:jimm]]

Attachment: test case

str: load the testcase, try to pan the page.

The sub frame points to a cgi script that loads slowly over a thirty second period.

During the sub frame load, the root frame can't pan or zoom.

Comment 1

[Jim Mathies [:jimm]]

Attachment: slow.cgi

Comment 2

[Matt Brubeck (:mbrubeck)]

For events that happen while the new page (or its resources) are still loading, it looks like APZCTreeManager::ReceiveInputEvent is still returning the ScrollableLayerGuid of the old page instead of the new one.

It looks like APZCTreeManager::UpdatePanZoomControllerTree is never called until the page is completely loaded.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> It looks like APZCTreeManager::UpdatePanZoomControllerTree is never called
> until the page is completely loaded.

Sorry, that comment was incorrect.  UpdatePanZoomControllerTree does get called during page load, but it doesn't construct the new APZC tree at that time, I think because the container->GetFrameMetrics().IsScrollable() check always fails until the page is loaded.

Comment 4

[Jim Mathies [:jimm]]

Interesting, until the sub page loads, the main web page doesn't get a nsDisplayScrollLayer. Hence, there's nothing to scroll. This doesn't appear to be the case on desktop.

Comment 5

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

When I load attachment 8336842 [details] on my Aurora desktop build (with mixed content blocking disabled) I cannot scroll the iframe until it is done loading either. In fact the iframe remains blank for 30 seconds and then it renders and is scrollable all at once.

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> When I load attachment 8336842 [details] on my Aurora desktop build (with
> mixed content blocking disabled) I cannot scroll the iframe until it is done
> loading either.

The bug on Metro is that the top-level document is not scrollable by touch until the iframe finishes loading.  (Or more generally, no document is scrollable by touch until all its resources are loaded.)

Comment 7

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #6)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > When I load attachment 8336842 [details] on my Aurora desktop build (with
> > mixed content blocking disabled) I cannot scroll the iframe until it is done
> > loading either.
> 
> The bug on Metro is that the top-level document is not scrollable by touch
> until the iframe finishes loading.  (Or more generally, no document is
> scrollable by touch until all its resources are loaded.)

Which is odd since on desktop, the top level document gets a scrollable frame immediately. Here's a local link to the test case to get around mixed content warnings on desktop - 

http://www.mathies.com/mozilla/slowsubframe.html

Comment 8

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

So that it doesn't get stuck in the "wrong" component, Timothy, please take a look at this.

Comment 9

[Jim Mathies [:jimm]]

Per discussions between marcom and milan, we're requesting tracking on the remaining "big issues" related to apzc for metrofx, which is riding the 28 train. We expect these will get fixed during the long aurora train ride.

This bug is layout related, scrollids do not get set properly for slow loading pages, and as such slow loading pages can't be panned.

Comment 12

[Matt Brubeck (:mbrubeck)]

Bug 950808 notes that a "resize" event while the page is still loading (for example, an orientation change, or switching from full screen to split screen) will cause touch scrolling to start working.

Comment 13

[Timothy Nikkel (:tnikkel)]

The problem is that we don't have a displayport set until either load or DOMContentLoaded I'm guessing, and so shouldBuildLayer is false in ScrollFrameHelper::BuildDisplayList because we are dealing with the root scroll frame of the root content document. So there is no layer that is annotated with scrollable FrameMetrics. In the remote tab situation we will still have FrameMetrics recorded via the call in nsDisplayList::PaintForFrame because that is called on display root documents (remote tabs are display root documents, local tabs aren't).

The root content document check was added way back in http://hg.mozilla.org/mozilla-central/rev/dea535299cf2 which extended async scrolling from just the root scroll frame of the root content document to any other elements. There was already an existing RecordFrameMetrics call for the root scroll frame of the root content document from the existing async scroll infracture for root scroll frames. That changeset just added RecordFrameMetrics calls for all the other cases. I think this is a mistake from that era of treating the root scroll frame specially. I think that long term we want to call RecordFrameMetrics from one place and always build scroll info layers for scroll frames (root content doc root or not). But that has more risk. I can attach a patch which just extends our current way of doing things to the situation where the root content document isn't the display root.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: Treat display roots as special (it's not root content document's that are special)

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Unify RecordFrameMetrics calls

This is probably really broken, it's untested.

AZPC probably relies on the metrics on the root layer. But would you agree that this is a direction we should go in?

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8355466 [details] [diff] [review]
Unify RecordFrameMetrics calls

We could also go in the opposite direction and RecordFrameMetrics on all subdocuments. kats had a patch to do that.

Comment 17

[Jim Mathies [:jimm]]

(In reply to Timothy Nikkel (:tn) from comment #14)
> Created attachment 8355461 [details] [diff] [review]
> Treat display roots as special (it's not root content document's that are
> special)

This patch solves the problem on various sites I've seen this on.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #17)
> (In reply to Timothy Nikkel (:tn) from comment #14)
> > Created attachment 8355461 [details] [diff] [review]
> > Treat display roots as special (it's not root content document's that are
> > special)
> 
> This patch solves the problem on various sites I've seen this on.

It also exposes the problem of page load jank, which has been hidden behind this bug ever since we turned apzc on. We have a couple planned changes to help address this in v2 (input thread separation and switching back to content processes).

Comment 19

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

Comment on attachment 8355466 [details] [diff] [review]
Unify RecordFrameMetrics calls

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

The direction this is going in seems fine to me.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2425,5 @@
>      }
>  #endif
> +    bool rootContentDocRootScroll =
> +      mIsRoot && mOuter->PresContext()->IsRootContentDocument();
> +    shouldBuildLayer = (rootContentDocRootScroll || wantSubAPZC) &&

So we're eventually going to want "wantSubAPZC" to be true everywhere, in which case the first half of this clause will always be true, and rootContentDocRootScroll is irrelevant. I think that makes sense based on my understanding (since we're effectively getting rid of all the special-case behaviour for the root frame), just wanted to verify.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8355466 [details] [diff] [review]
Unify RecordFrameMetrics calls

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

I like the direction this is going in as well. I have one concern, see below.

Also, what is the relationship between this patch and the previous one ("treat display roots as special")? Are they meant to be applied together?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2425,5 @@
>      }
>  #endif
> +    bool rootContentDocRootScroll =
> +      mIsRoot && mOuter->PresContext()->IsRootContentDocument();
> +    shouldBuildLayer = (rootContentDocRootScroll || wantSubAPZC) &&

APZ code currently assumes that the root scroll frame of the root content document always has an APZC, even if it's not actually scrollable. Prior to this change, this was ensured by PaintForFrame() calling RecordFrameMetrics() unconditionally, even if the frame wasn't actually scrollable. To maintain this behaviour, we probably want to change this to:

shouldBuildLayer =    rootContentDocRootScroll
                   || (wantSubAPZC && (wantLayerV || wantLayerH));

which will eventually simplify to

shouldBuildLayer =    rootContentDocRootScroll
                   || (wantLayerV || wantLayerH);

Comment 21

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2d0b22edf0

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8355466 [details] [diff] [review]
Unify RecordFrameMetrics calls

I re-posted this patch, with my suggested change, to bug 951936, as it seems we are going with the other patch for this bug.

Comment 23

[Jim Mathies [:jimm]]

(In reply to Timothy Nikkel (:tn) from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2d0b22edf0

Did this stick?

Comment 24

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

Yeah, it was merged to central this morning. Presumably Ed will be along shortly to mark this bug fixed.

Comment 25

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/8b2d0b22edf0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b2d0b22edf0

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

looks like this could be ready for an uplift nomination

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8355461 [details] [diff] [review]
Treat display roots as special (it's not root content document's that are special)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): always there on metro?
User impact if declined: scrolling pages won't work while loading
Testing completed (on m-c, etc.): m-c for a few days
Risk to taking this patch (and alternatives if risky): should be pretty safe, any problems should be discovered fairly quickly
String or IDL/UUID changes made by this patch: none

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bee85121042

Comment 32

[Ioana (away)]

Marking for verification since in-testsuite is still "?"...

Comment 33

[Ada [:adalucinet]]

Reproduced this issue with Nightly from 2014-01-02 on Microsoft Surface Pro 2 with the attachment from comment 0: in Metro mode, I couldn't zoom/scroll until the sub page loaded.

Verified as fixed with latest Firefox 28 beta 9 (Build ID: 20140306171728) and latest Aurora (Build ID: 20140306004001): 
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0	
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0