Zoomed page keeps unzooming while it finishes loading

VERIFIED FIXED in Firefox 11

Status

()

defect
P1
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: deb, Assigned: pcwalton)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 verified, firefox12 verified, fennec11+)

Details

(Whiteboard: [MTD])

Attachments

(5 attachments, 5 obsolete attachments)

2.50 KB, patch
Details | Diff | Splinter Review
2.00 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
2.07 KB, patch
Details | Diff | Splinter Review
4.16 KB, patch
mfinkle
: feedback+
Details | Diff | Splinter Review
8.60 KB, patch
mfinkle
: feedback+
Details | Diff | Splinter Review
Going to the NYT website on Fennec, I'll zoom in on the front page (for example) so I can scroll around and read stuff, but while the page finishes loading it keeps zooming back out.  This happens 3-4 times before the page finally settles down and stays zoomed in.
This is, far and away, the worst bug I encounter when using Fennec. It makes browsing basically impossible for me.
Whiteboard: [MTD]
Taking this. I assume this is a P1.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Duplicate of this bug: 711098
Priority: -- → P1
Blocks: 709492
Posted patch WIP patch. (obsolete) — Splinter Review
Here's a WIP that solves lots of the issues in this bug for me by building on the work in bug 709492. It just removes the spurious updateViewport() events. Unfortunately, it breaks the metrics first page loaded for me, so it needs a little bit more work, perhaps in session restore.
Attachment #583079 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 583079 [details] [diff] [review]
WIP patch.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>       case "DOMContentLoaded": {
>         let target = aEvent.originalTarget;
> 
>         // ignore on frames
>         if (target.defaultView != this.browser.contentWindow)
>           return;
> 
>-        this.updateViewport(true);
>-

I agree with taking this one out once the patches in your other bug work as intended.

>     switch (aEvent.type) {
>-      case "DOMWindowCreated":
>-        this.resetMetadata(tab);
>-        break;
>-
>       case "DOMMetaAdded":
>         if (target.name == "viewport")
>           this.updateMetadata(tab);
>         break;
> 
>-      case "DOMContentLoaded":
>-      case "pageshow":
>-        this.updateMetadata(tab);
>-        break;
>-
>       case "resize":

For these I'm not so sure, you might want to check with mbrubeck as well because we might need to recheck the meta viewport tags at different points.

FWIW, when I added some of these updateViewport calls, they were always to handle on or more of these scenarios where the gecko initiates the viewport update, so we should make sure that changes don't regress any of these scenarios unintentionally:

1) initial start up
2) open a new tab
3) switch tabs
4) closing active tab
5) closing inactive tab
6) loading a new page
7) fast cached back/forward navigation
8) slow back/forward navigation (the events sent for this are different than for 7)
9) page reload
10) anchor jump (i.e. click on link to #foo)
11) page JS does scrollTo()
Attachment #583079 - Flags: feedback?(bugmail.mozilla) → feedback+
None of these scenarios regress.
This patch removes viewport changes that occur after the page starts.
Attachment #583079 - Attachment is obsolete: true
Attachment #584901 - Flags: review?(mbrubeck)
Patch part 2 makes the initial page size nicer (i.e. not 1x1), to mitigate weird behaviors when loading the first page.
Attachment #584902 - Flags: review?(bugmail.mozilla)
Comment on attachment 584902 [details] [diff] [review]
Proposed patch, part 2: Use a nicer initial page size.

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

Looks good to me. Does it makes sense to also make the viewport size equal the display size instead of being 1x1? I'm not a fan of initializing things to 1x1 in general, so the more we can get rid of that stuff the better.
Attachment #584902 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 584901 [details] [diff] [review]
Proposed patch, part 1: Remove viewport changes that occur after page show.

I haven't finished building so I can test this yet, but I'm worried that it will make any page without a <meta name="viewport"> tag inherit the viewport settings from that last page in the browser history that *did* have such a tag.  That is, if you load a page with <meta name="viewport" content="width=320">, then navigate to a "desktop" page in the same tab, the desktop page will continue to use a 320-px-wide viewport instead of switching to the default 980px.

We might need to at least keep the resetMetadata call on DOMWindowCreated.  If that's not sufficient, then what we really need is code to change the CSS viewport without resetting the scroll/zoom level (at least if the viewport size hasn't changed).
Patch part 1 brings back the document-shown patch from bug 709492. Carrying forward r+ from bz.
Patch part 2 keys viewport metadata to the document rather than the tab, using a WeakMap to avoid leaking documents.
Attachment #585945 - Flags: review?(mbrubeck)
Patch part 3 eliminates jumpiness that can occur when moving from page to page when the two pages have different viewport metadata. In this case, showing a new document can result in the browser size changing. We need to avoid drawing the new page until browser.js has managed to set the browser size appropriately.

To avoid the instability that the initial set of patches in bug 709492 suffered from, we don't implement drawing suppression as a flag that event handlers set and unset. Rather, we simply remember which document the current browser size reflects (via the documentIdForCurrentViewport field in the tab). We suppress painting unless the document in the browser matches the document that the browser size reflects.

Note that, to eliminate the possibility of leaking content documents, we don't hold on to strong references; rather, we use IDs.

This patch also viewport metadata adjustment to the document-shown event handler, so that when navigating we read the viewport metadata as early as possible.
Attachment #585949 - Flags: review?(bugmail.mozilla)
Patch part 4 removes viewport changes that occur after page show (except those that result from the document modifying the meta tag), fixing this bug. All documents that are loaded should fire document-shown, so we will never miss viewport changes.
Attachment #584901 - Attachment is obsolete: true
Attachment #584901 - Flags: review?(mbrubeck)
Attachment #585950 - Flags: review?(mbrubeck)
Attachment #585945 - Flags: review?(mbrubeck) → review+
Attachment #585950 - Flags: review?(mbrubeck) → review+
Patch part 5 is the same as patch part 2 from earlier with comments addressed. Carrying forward r+.
Attachment #584902 - Attachment is obsolete: true
Comment on attachment 585945 [details] [diff] [review]
Proposed patch, part 2: Key viewport metadata to the document rather than the tab.

I'm not a big fan of this patch. It feels wrong to be throwing a collection of documents/metadata into BrowserApp.
(In reply to Mark Finkle (:mfinkle) from comment #16)
> I'm not a big fan of this patch. It feels wrong to be throwing a collection
> of documents/metadata into BrowserApp.

Perhaps the weakmap should live in ViewportHandler instead?
(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> (In reply to Mark Finkle (:mfinkle) from comment #16)
> > I'm not a big fan of this patch. It feels wrong to be throwing a collection
> > of documents/metadata into BrowserApp.
> 
> Perhaps the weakmap should live in ViewportHandler instead?

That does feel a bit better to me
Comment on attachment 585949 [details] [diff] [review]
Proposed patch, part 3: Suppress paint events until we've changed the page size.

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

LGTM
Attachment #585949 - Flags: review?(bugmail.mozilla) → review+
(In reply to Mark Finkle (:mfinkle) from comment #18)
> That does feel a bit better to me

Or factor it out into some sort of DocumentInfo class? All we're really doing is storing private data on documents here. Would make sense to put that in a table separate from BrowserApp so we can extend it if we need to.
Patch part 2 version 2 hopefully addresses mfinkle's comments by moving the document info out of the BrowserApp and to a separate DocumentInfo class.
Attachment #585945 - Attachment is obsolete: true
Attachment #586297 - Flags: feedback?(mark.finkle)
Ditto for patch part 3.
Attachment #585949 - Attachment is obsolete: true
Attachment #586298 - Flags: feedback?(mark.finkle)
Patrick did a great job explaining the issues of why we need to track documents. It totally makes sense to me. My only feedback is that we should just reuse ViewportHandler to hold the data, to cut down on the extra helpers.

If this is all about managing viewports, it makes sense to store the docs there too. I don't know if a generic store of documents will be useful to anyone else at this point and it might give people ideas that they can use the documents without knowing how we are managing them.
Comment on attachment 586297 [details] [diff] [review]
Proposed patch, part 2, version 2: Key viewport metadata to the document rather than to the tab.

Just add a little more comments about why we need to track this so we don't lose that info.
Attachment #586297 - Flags: feedback?(mark.finkle) → feedback+
Attachment #586298 - Flags: feedback?(mark.finkle) → feedback+
tracking-fennec: --- → 11+
(In reply to Mark Finkle (:mfinkle) from comment #23)
> Patrick did a great job explaining the issues of why we need to track
> documents. It totally makes sense to me. My only feedback is that we should
> just reuse ViewportHandler to hold the data, to cut down on the extra
> helpers.
> 
> If this is all about managing viewports, it makes sense to store the docs
> there too. I don't know if a generic store of documents will be useful to
> anyone else at this point and it might give people ideas that they can use
> the documents without knowing how we are managing them.

I moved them into ViewportHandler before I pushed.
Samsung Nexus S (Android 4.0.3)
Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20120110 Firefox/12.0a1 Fennec/12.0a1

Aurora nom?
Status: RESOLVED → VERIFIED
Comment on attachment 585944 [details] [diff] [review]
Proposed patch, part 1: Add an observer for page show.

[Approval Request Comment]
Regression caused by (bug #): Not a regression
User impact if declined: Annoying behavior when panning and zooming immediately after page load. As comment 1 shows, this is a severe usability issue.
Testing completed (on m-c, etc.): It's been baking on m-c for a few days.
Risk to taking this patch (and alternatives if risky): It involves painting suppression, so there's always the risk that painting will not get unsuppressed. (Note that the need for painting suppression should be alleviated by bug 716575.)
Attachment #585944 - Flags: approval-mozilla-aurora?
Attachment #585950 - Flags: approval-mozilla-aurora?
Attachment #585951 - Flags: approval-mozilla-aurora?
Attachment #586297 - Flags: approval-mozilla-aurora?
Attachment #586298 - Flags: approval-mozilla-aurora?
Comment on attachment 585944 [details] [diff] [review]
Proposed patch, part 1: Add an observer for page show.

[Triage Comment]
Mobile only (and an awesome fix) - approved for Aurora.
Attachment #585944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #585950 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #585951 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #586297 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #586298 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:11.0a2) Gecko/20120123 Firefox/11.0a2 Fennec/11.0a2
Device: LG Optimus 2X (Android 2.2)
You need to log in before you can comment on or make changes to this bug.