Closed Bug 942189 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

26 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox28 + verified
firefox29 --- verified

People

(Reporter: jimm, Assigned: tnikkel)

References

Details

(Whiteboard: [beta28] [layout])

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Attached file slow.cgi
Attachment #8336844 - Attachment mime type: application/x-perl → text/plain
Blocks: 933990
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.
(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.
Whiteboard: [triage] → [block28]
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.)
(In reply to Matt Brubeck (:mbrubeck) from comment #6)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Whiteboard: [block28] → [beta28]
Component: Panning and Zooming → Layout
Whiteboard: [beta28] → [beta28][layout]
Blocks: metrov1backlog, metrov1omtc&apzc
No longer blocks: metro-apzc
So that it doesn't get stuck in the "wrong" component, Timothy, please take a look at this.
Assignee: nobody → tnikkel
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.
Summary: Pages with slow loading sub frames can't pan → Pages with slow loading sub frames can't pan or zoom by touch
Duplicate of this bug: 933990
Blocks: 950808
Duplicate of this bug: 900352
Summary: Pages with slow loading sub frames can't pan or zoom by touch → Pages with slow loading sub frames (or other resources) can't pan or zoom by touch
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.
Blocks: 953012
Whiteboard: [beta28][layout] → [beta28] [layout]
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.
Attached patch Unify RecordFrameMetrics calls (obsolete) — Splinter Review
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?
Attachment #8355466 - Flags: feedback?(bugmail.mozilla)
Attachment #8355466 - Flags: feedback?(botond)
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.
(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.
(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 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.
Attachment #8355466 - Flags: feedback?(bugmail.mozilla) → feedback+
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);
Attachment #8355466 - Flags: feedback?(botond) → feedback+
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.
Attachment #8355466 - Attachment is obsolete: true
(In reply to Timothy Nikkel (:tn) from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2d0b22edf0

Did this stick?
Yeah, it was merged to central this morning. Presumably Ed will be along shortly to mark this bug fixed.
https://hg.mozilla.org/mozilla-central/rev/8b2d0b22edf0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
No longer blocks: metrov1backlog
Duplicate of this bug: 956692
looks like this could be ready for an uplift nomination
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
Attachment #8355461 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 952501
Attachment #8355461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite?
Marking for verification since in-testsuite is still "?"...
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.