Deb Richardson [:dria] (plz NEEDINFO) Reporter
	Description • 13 years ago

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.

	Joe Drew (not getting mail)
	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.

	Patrick Walton (:pcwalton) Assignee
	Comment 2 • 13 years ago

Taking this. I assume this is a P1.

	Patrick Walton (:pcwalton) 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	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()

	Patrick Walton (:pcwalton) Assignee
	Comment 6 • 13 years ago

None of these scenarios regress.

	Patrick Walton (:pcwalton) Assignee
	Comment 7 • 13 years ago

This patch removes viewport changes that occur after the page starts.

	Patrick Walton (:pcwalton) 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	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.

	Matt Brubeck (:mbrubeck)
	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).

	Patrick Walton (:pcwalton) Assignee
	Comment 11 • 13 years ago

Patch part 1 brings back the document-shown patch from bug 709492. Carrying forward r+ from bz.

	Patrick Walton (:pcwalton) 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.

	Patrick Walton (:pcwalton) 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.

	Patrick Walton (:pcwalton) 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.

	Patrick Walton (:pcwalton) Assignee
	Comment 15 • 13 years ago

Patch part 5 is the same as patch part 2 from earlier with comments addressed. Carrying forward r+.

	Mark Finkle (:mfinkle) (use needinfo?)
	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.

	Matt Brubeck (:mbrubeck)
	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?

	Mark Finkle (:mfinkle) (use needinfo?)
	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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	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

	Patrick Walton (:pcwalton) 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.

	Patrick Walton (:pcwalton) 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.

	Patrick Walton (:pcwalton) Assignee
	Comment 22 • 13 years ago

Ditto for patch part 3.

	Mark Finkle (:mfinkle) (use needinfo?)
	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.

	Mark Finkle (:mfinkle) (use needinfo?)
	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.

	Patrick Walton (:pcwalton) 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

	Patrick Walton (:pcwalton) 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	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

	Aaron Train [:aaronmt]
	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?

	Patrick Walton (:pcwalton) 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.)

	Alex Keybl [:akeybl]
	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.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 32 • 13 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/381c06eb3eb4

	Brad Lassey [:blassey] (use needinfo?)
	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

	Andreea Pod
	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)