Closed
Bug 708746
Opened 13 years ago
Closed 13 years ago
Zoomed page keeps unzooming while it finishes loading
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox11 verified, firefox12 verified, fennec11+)
VERIFIED
FIXED
Firefox 12
People
(Reporter: deb, Assigned: pcwalton)
References
Details
(Whiteboard: [MTD])
Attachments
(5 files, 5 obsolete files)
2.50 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
mbrubeck
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.07 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.16 KB,
patch
|
mfinkle
:
feedback+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.60 KB,
patch
|
mfinkle
:
feedback+
akeybl
:
approval-mozilla-aurora+
|
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.
Comment 1•13 years ago
|
||
This is, far and away, the worst bug I encounter when using Fennec. It makes browsing basically impossible for me.
Updated•13 years ago
|
Whiteboard: [MTD]
Assignee | ||
Comment 2•13 years ago
|
||
Taking this. I assume this is a P1.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Updated•13 years ago
|
Priority: -- → P1
Assignee | ||
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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+
Assignee | ||
Comment 6•13 years ago
|
||
None of these scenarios regress.
Assignee | ||
Comment 7•13 years ago
|
||
This patch removes viewport changes that occur after the page starts.
Attachment #583079 -
Attachment is obsolete: true
Attachment #584901 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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 10•13 years ago
|
||
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).
Assignee | ||
Comment 11•13 years ago
|
||
Patch part 1 brings back the document-shown patch from bug 709492. Carrying forward r+ from bz.
Assignee | ||
Comment 12•13 years ago
|
||
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)
Assignee | ||
Comment 13•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #585949 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 14•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #585945 -
Flags: review?(mbrubeck) → review+
Updated•13 years ago
|
Attachment #585950 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 15•13 years ago
|
||
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 16•13 years ago
|
||
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.
Comment 17•13 years ago
|
||
(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?
Comment 18•13 years ago
|
||
(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 19•13 years ago
|
||
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+
Assignee | ||
Comment 20•13 years ago
|
||
(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.
Assignee | ||
Comment 21•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
Ditto for patch part 3.
Attachment #585949 -
Attachment is obsolete: true
Attachment #586298 -
Flags: feedback?(mark.finkle)
Comment 23•13 years ago
|
||
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 24•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #586298 -
Flags: feedback?(mark.finkle) → feedback+
Updated•13 years ago
|
tracking-fennec: --- → 11+
Assignee | ||
Comment 25•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2a79ca496f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5ab1377d94
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a2b5e395ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/4539e9ec1e19
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce8729ba2c7
Assignee | ||
Comment 26•13 years ago
|
||
(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.
Comment 28•13 years ago
|
||
This was merged to m-c a few days ago:
https://hg.mozilla.org/mozilla-central/rev/ed2a79ca496f
https://hg.mozilla.org/mozilla-central/rev/ef5ab1377d94
https://hg.mozilla.org/mozilla-central/rev/18a2b5e395ca
https://hg.mozilla.org/mozilla-central/rev/4539e9ec1e19
https://hg.mozilla.org/mozilla-central/rev/7ce8729ba2c7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Comment 29•13 years ago
|
||
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
Assignee | ||
Comment 30•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Attachment #585950 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•13 years ago
|
Attachment #585951 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•13 years ago
|
Attachment #586297 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•13 years ago
|
Attachment #586298 -
Flags: approval-mozilla-aurora?
Comment 31•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #585950 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #585951 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #586297 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #586298 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 32•13 years ago
|
||
Comment 33•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/381c06eb3eb4
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ce2316780c6
https://hg.mozilla.org/releases/mozilla-aurora/rev/4cf9a42a8de7
https://hg.mozilla.org/releases/mozilla-aurora/rev/77e173e17108
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a7e2ca2c8c5
Comment 34•13 years ago
|
||
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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•