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 User image 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 User image Kevin Brosnan 2010-07-27 19:05:57 PDT

*** This bug has been marked as a duplicate of bug 442252 ***
Comment 2 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2010-07-28 02:18:39 PDT
*** Bug 578289 has been marked as a duplicate of this bug. ***
Comment 4 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image 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 User image 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 User image Alice0775 White 2011-01-20 21:53:45 PST
*** Bug 576778 has been marked as a duplicate of this bug. ***
Comment 8 User image 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 User image Alice0775 White 2011-07-06 05:52:43 PDT
*** Bug 669591 has been marked as a duplicate of this bug. ***
Comment 10 User image Thomas Ahlblom 2011-12-31 10:33:14 PST
*** Bug 714105 has been marked as a duplicate of this bug. ***
Comment 11 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image :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 User image :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 User image :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 User image 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 User image 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 User image :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 User image :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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image :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 User image 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 User image 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 User image 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.