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

VERIFIED FIXED in Firefox 28

Status

()

Core
Layout
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jimm, Assigned: tnikkel)

Tracking

(Blocks: 1 bug)

26 Branch
mozilla29
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox28+ verified, firefox29 verified)

Details

(Whiteboard: [beta28] [layout])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8336842 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8336844 [details]
slow.cgi
(Reporter)

Updated

4 years ago
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.

Updated

4 years ago
Whiteboard: [triage] → [block28]
(Reporter)

Comment 4

4 years ago
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.)
(Reporter)

Comment 7

4 years ago
(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
(Reporter)

Updated

4 years ago
Whiteboard: [block28] → [beta28]
(Reporter)

Updated

4 years ago
Component: Panning and Zooming → Layout
(Reporter)

Updated

4 years ago
Whiteboard: [beta28] → [beta28][layout]

Updated

4 years ago
Blocks: 838081, 844132
No longer blocks: 886321
So that it doesn't get stuck in the "wrong" component, Timothy, please take a look at this.
Assignee: nobody → tnikkel
(Reporter)

Comment 9

4 years ago
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.
tracking-firefox28: --- → ?
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
(Reporter)

Updated

4 years ago
Blocks: 950808
(Reporter)

Updated

4 years ago
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

Updated

4 years ago
Whiteboard: [beta28][layout] → [beta28] [layout]
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
Created attachment 8355461 [details] [diff] [review]
Treat display roots as special (it's not root content document's that are special)
Attachment #8355461 - Flags: review?(roc)
(Assignee)

Comment 15

4 years ago
Created attachment 8355466 [details] [diff] [review]
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?
Attachment #8355466 - Flags: feedback?(bugmail.mozilla)
Attachment #8355466 - Flags: feedback?(botond)
(Assignee)

Comment 16

4 years ago
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.
Attachment #8355461 - Flags: review?(roc) → review+
(Reporter)

Comment 17

4 years ago
(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.
(Reporter)

Comment 18

4 years ago
(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+
(Assignee)

Comment 21

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2d0b22edf0
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
(Reporter)

Comment 23

4 years ago
(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
Last Resolved: 4 years ago
status-firefox28: --- → affected
status-firefox29: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/8b2d0b22edf0

Updated

4 years ago
No longer blocks: 838081
(Reporter)

Updated

4 years ago
Duplicate of this bug: 956692
looks like this could be ready for an uplift nomination
tracking-firefox28: ? → +
(Assignee)

Comment 29

4 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bee85121042
status-firefox28: affected → fixed
Flags: in-testsuite?

Comment 32

4 years ago
Marking for verification since in-testsuite is still "?"...
Keywords: verifyme

Comment 33

4 years ago
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
status-firefox28: fixed → verified
status-firefox29: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.