Last Comment Bug 575830 - Image zoom (Page zoom) is reset when I switch tabs
: Image zoom (Page zoom) is reset when I switch tabs
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: Firefox 12
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
: 576778 578289 669591 714105 (view as bug list)
Depends on: 722624
Blocks: 679784
  Show dependency treegraph
 
Reported: 2010-06-29 16:32 PDT by Jesse Ruderman
Modified: 2012-01-30 22:34 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.30 KB, patch)
2012-01-11 21:04 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch (3.60 KB, patch)
2012-01-11 21:38 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v2 (4.54 KB, patch)
2012-01-12 08:16 PST, Matt Brubeck (:mbrubeck)
gavin.sharp: review-
Details | Diff | Splinter Review
patch v3 (6.17 KB, patch)
2012-01-12 21:26 PST, Matt Brubeck (:mbrubeck)
gavin.sharp: review+
Details | Diff | Splinter Review
patch v4 (6.83 KB, patch)
2012-01-13 16:13 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v5 (7.87 KB, patch)
2012-01-13 19:16 PST, Matt Brubeck (:mbrubeck)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-06-29 16:32:37 PDT
1. Load http://daringfireball.net/misc/2010/06/iphone3gs-homescreen.jpg
2. Zoom in a few notches using Cmd++
3. Switch to another tab
4. Switch back

Result: right after i switch back, it zooms back to normal

Expected: keep my zoom level

Is this a regression from bug 541779?
Comment 1 Kevin Brosnan 2010-07-27 19:05:57 PDT

*** This bug has been marked as a duplicate of bug 442252 ***
Comment 2 Henrik Skupin (:whimboo) 2010-07-28 02:18:27 PDT
This is not related to bug 442252. This bug is about page zoom and not image scaling.
Comment 3 Henrik Skupin (:whimboo) 2010-07-28 02:18:39 PDT
*** Bug 578289 has been marked as a duplicate of this bug. ***
Comment 4 Henrik Skupin (:whimboo) 2010-07-28 02:27:52 PDT
This works fine with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.12pre) Gecko/20100721 Shiretoko/3.5.12pre

And is broken with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.8pre) Gecko/20100718 Namoroka/3.6.8pre

Eventually it has been regressed between Firfox 3.5 and Firefox 3.6 or a backport from trunk introduced it on 1.9.2.
Comment 5 Reece H. Dunn 2010-07-28 03:11:50 PDT
The issue in bug 578289 is for "Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100712 Minefield/4.0b2pre" so this is broken in 4.0 as well.
Comment 6 Alice0775 White 2010-08-03 19:32:00 PDT
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/8366e5cc9f57
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090802 Namoroka/3.6a1pre ID:20090802042924
Fails:
http://hg.mozilla.org/mozilla-central/rev/51f332235f14
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090803 Namoroka/3.6a1pre ID:20090803044626
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8366e5cc9f57&tochange=51f332235f14
CandidateBug:
Bug 487656 - zoomed in by default while entering Private Browsing
Comment 7 Alice0775 White 2011-01-20 21:53:45 PST
*** Bug 576778 has been marked as a duplicate of this bug. ***
Comment 8 Jean-François Bastien 2011-01-20 22:01:21 PST
This bug is also in Minefield 4.0b10pre 2011-01-18 (my full config is in Bug 576778).
Comment 9 Alice0775 White 2011-07-06 05:52:43 PDT
*** Bug 669591 has been marked as a duplicate of this bug. ***
Comment 10 Thomas Ahlblom 2011-12-31 10:33:14 PST
*** Bug 714105 has been marked as a duplicate of this bug. ***
Comment 11 Matt Brubeck (:mbrubeck) 2012-01-11 20:51:09 PST
From the code it looks like this is a regression from bug 416661, though that does not match the range in comment 6.
Comment 12 Matt Brubeck (:mbrubeck) 2012-01-11 21:04:36 PST
Created attachment 587942 [details] [diff] [review]
patch

This fixes the bug, but I still need to (a) write a test and (b) make sure I didn't break any existing tests.
Comment 13 Matt Brubeck (:mbrubeck) 2012-01-11 21:38:36 PST
Created attachment 587951 [details] [diff] [review]
patch

Added a test, and pushed to Try.
Comment 14 Mozilla RelEng Bot 2012-01-12 01:30:40 PST
Try run for e67d0ec245ed is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e67d0ec245ed
Results (out of 109 total builds):
    success: 83
    warnings: 26
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-e67d0ec245ed
Comment 15 Matt Brubeck (:mbrubeck) 2012-01-12 07:51:38 PST
Comment on attachment 587951 [details] [diff] [review]
patch

This patch caused browser_bug386835.js to time out on Try.  Investigating.
Comment 16 Matt Brubeck (:mbrubeck) 2012-01-12 08:16:27 PST
Created attachment 588036 [details] [diff] [review]
patch v2

_applyPrefToSetting is no longer called when switching to an image tab, so I fixed browser_bug386835.js not to wait for it in that case.  (Or maybe I should make it wait for FullZoom.onLocationChange instead?)

Pushed to Try again: https://tbpl.mozilla.org/?tree=Try&rev=aefb6f210ab1
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-12 08:33:48 PST
Comment on attachment 588036 [details] [diff] [review]
patch v2


>+    // We don't save a pref for image documents, so don't try to restore it
>+    // after switching to a different tab.
>+    if (aIsTabSwitch && browser.contentDocument instanceof Ci.nsIImageDocument)
>+      return;
>+

nit: We tend to use simple DOM prototypes for this kind of checks (browser.contentDocument instanceof ImageDocument).
Comment 18 Matt Brubeck (:mbrubeck) 2012-01-12 12:01:01 PST
(In reply to Mano from comment #17)
> nit: We tend to use simple DOM prototypes for this kind of checks
> (browser.contentDocument instanceof ImageDocument).

Shall I update the style in the rest of this file too?
Comment 19 Mozilla RelEng Bot 2012-01-12 12:30:37 PST
Try run for aefb6f210ab1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=aefb6f210ab1
Results (out of 17 total builds):
    success: 15
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-aefb6f210ab1
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 18:53:46 PST
Comment on attachment 588036 [details] [diff] [review]
patch v2

There are other callers of _applyPrefToSetting() that seem to want to ignore image documents, like the one in onContentPrefSet and onContentPrefRemoved. Shouldn't _applyPrefToSetting just refuse to touch the zoom on image documents, rather than setting it to 1?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 18:54:12 PST
Oh, as far as I can tell bug 679784 did break this, by changing _applyPrefToSetting such that it sets the zoom level for image documents to 1 unconditionally, rather than not setting it at all:

http://hg.mozilla.org/mozilla-central/rev/3b04343f2382#l1.98

I don't really understand that change. It seems like we should just revert it...
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 18:57:21 PST
Though that change landed much after this bug was filed, obviously, so the original cause may be unrelated.
Comment 23 Matt Brubeck (:mbrubeck) 2012-01-12 21:26:22 PST
Created attachment 588306 [details] [diff] [review]
patch v3

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> There are other callers of _applyPrefToSetting() that seem to want to ignore
> image documents, like the one in onContentPrefSet and onContentPrefRemoved.
> Shouldn't _applyPrefToSetting just refuse to touch the zoom on image
> documents, rather than setting it to 1?

Updated the patch to ignore image documents completely in _applyPrefToSetting.  We still want to set the zoom to 1 on navigation to an image document (but not on tab switching or pref changes).  The patch adds code to do that directly in onLocationChange.

I also replaced Ci.nsIImageDocument with ImageDocument file-wide.
Comment 24 Dão Gottwald [:dao] 2012-01-13 00:09:03 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> Updated the patch to ignore image documents completely in
> _applyPrefToSetting.  We still want to set the zoom to 1 on navigation to an
> image document (but not on tab switching or pref changes).  The patch adds
> code to do that directly in onLocationChange.

Does this cover loads in background tab?
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-13 14:34:20 PST
(In reply to Dão Gottwald [:dao] from comment #24)
> > code to do that directly in onLocationChange.
> 
> Does this cover loads in background tab?

FullZoom.onLocationChange is called from the TabsProgressListener, so it should be fine.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-13 14:35:49 PST
(but it'd be nice to have the test cover that case too)
Comment 27 Matt Brubeck (:mbrubeck) 2012-01-13 16:13:48 PST
Created attachment 588558 [details] [diff] [review]
patch v4

Argh, sorry for the review spam.  I thought I ran all tests successfully with my last patch, but I guess not - it broke browser_bug386835.js again.  The only change in this version is to make browser_bug386835.js wait on setZoomForBrowser instead of _applyPrefToValue, since the former is still called on every page load but the latter is not.  This passes tests locally, and I pushed to Try again just to make sure.

(In reply to Dão Gottwald [:dao] from comment #24)
> Does this cover loads in background tab?

Yes, and this is tested in browser_bug386835.js.
Comment 28 Matt Brubeck (:mbrubeck) 2012-01-13 16:15:13 PST
Comment on attachment 588558 [details] [diff] [review]
patch v4

Wait, missed another test failure... :(
Comment 29 Mozilla RelEng Bot 2012-01-13 16:32:08 PST
Try run for ea0ad67fc58a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ea0ad67fc58a
Results (out of 72 total builds):
    success: 54
    warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-ea0ad67fc58a
 Timed out after 06 hours without completing.
Comment 30 Mozilla RelEng Bot 2012-01-13 16:32:13 PST
Try run for 19c6fbdeb3f5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=19c6fbdeb3f5
Results (out of 6 total builds):
    exception: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-19c6fbdeb3f5
Comment 31 Matt Brubeck (:mbrubeck) 2012-01-13 19:16:58 PST
Created attachment 588588 [details] [diff] [review]
patch v5

Fifth time's the charm?  This time it is green on Try.  (I had to apply the same fix to another test.)
Comment 32 Mozilla RelEng Bot 2012-01-14 00:16:46 PST
Try run for 87b8340ec148 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=87b8340ec148
Results (out of 18 total builds):
    success: 13
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-87b8340ec148
 Timed out after 06 hours without completing.
Comment 33 Jared Wein [:jaws] (please needinfo? me) 2012-01-14 14:52:37 PST
Matt: Can you make this work for VideoDocuments as well? You might just be able to switch your check of
> if (browser.contentDocument instanceof ImageDocument)
to
> if (browser.contentDocument.mozSyntheticDocument)
and then this would work for ImageDocument, VideoDocument, and PluginDocument.
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-18 12:40:19 PST
Comment on attachment 588588 [details] [diff] [review]
patch v5

We should switch this to using mozSyntheticDocument, but that should happen in another bug.
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2012-01-18 12:57:04 PST
(In reply to Jared Wein [:jaws] from comment #33)
> Matt: Can you make this work for VideoDocuments as well?

I should have tested this before I left that comment, but it actually already keeps the zoom level with VideoDocument.
Comment 36 Matt Brubeck (:mbrubeck) 2012-01-18 16:13:28 PST
Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6244d044aa

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> We should switch this to using mozSyntheticDocument, but that should happen
> in another bug.

Filed bug 719271 and assigned it to myself.
Comment 37 Marco Bonardo [::mak] 2012-01-19 02:57:05 PST
https://hg.mozilla.org/mozilla-central/rev/1f6244d044aa

Note You need to log in before you can comment on or make changes to this bug.