575830 - Image zoom (Page zoom) is reset when I switch tabs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

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 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is not related to bug 442252. This bug is about page zoom and not image scaling.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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]

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]

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 8

[Jean-François Bastien]

This bug is also in Minefield 4.0b10pre 2011-01-18 (my full config is in Bug 576778).

Comment 11

[Matt Brubeck (:mbrubeck)]

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)]

Attachment: 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)]

Attachment: patch

Added a test, and pushed to Try.

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-e67d0ec245ed

Comment 15

[Matt Brubeck (:mbrubeck)]

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)]

Attachment: 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

[Asaf Romano (gone)]

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)]

(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]

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/[e-mail]-aefb6f210ab1

Comment 20

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

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: [e-mail]]]

Though that change landed much after this bug was filed, obviously, so the original cause may be unrelated.

Comment 23

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

(In reply to Gavin Sharp (use [e-mail] 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]]

(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: [e-mail]]]

(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: [e-mail]]]

(but it'd be nice to have the test cover that case too)

Comment 27

[Matt Brubeck (:mbrubeck)]

Attachment: 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)]

Comment on attachment 588558 [details] [diff] [review]
patch v4

Wait, missed another test failure... :(

Comment 29

[Mozilla RelEng Bot]

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/[e-mail]-ea0ad67fc58a
 Timed out after 06 hours without completing.

Comment 30

[Mozilla RelEng Bot]

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/[e-mail]-19c6fbdeb3f5

Comment 31

[Matt Brubeck (:mbrubeck)]

Attachment: 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]

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/[e-mail]-87b8340ec148
 Timed out after 06 hours without completing.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

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: [e-mail]]]

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)]

(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)]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6244d044aa

(In reply to Gavin Sharp (use [e-mail] 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]]

https://hg.mozilla.org/mozilla-central/rev/1f6244d044aa