The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mbrubeck)

Tracking

({regression})

Trunk
Firefox 12
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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?

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 442252
This is not related to bug 442252. This bug is about page zoom and not image scaling.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Image zoom is reset when I switch tabs → Image zoom (Page zoom) is reset when I switch tabs
Duplicate of this bug: 578289
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.
Status: REOPENED → NEW
Keywords: regression, regressionwindow-wanted
OS: Mac OS X → All
Hardware: x86 → All

Comment 5

7 years ago
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

7 years ago
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
Blocks: 487656

Updated

7 years ago
Keywords: regressionwindow-wanted

Updated

6 years ago
Duplicate of this bug: 576778
This bug is also in Minefield 4.0b10pre 2011-01-18 (my full config is in Bug 576778).

Updated

6 years ago
Duplicate of this bug: 669591

Updated

5 years ago
Duplicate of this bug: 714105
(Assignee)

Comment 11

5 years ago
From the code it looks like this is a regression from bug 416661, though that does not match the range in comment 6.
Blocks: 416661
(Assignee)

Comment 12

5 years ago
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.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
Created attachment 587951 [details] [diff] [review]
patch

Added a test, and pushed to Try.
Attachment #587942 - Attachment is obsolete: true
Attachment #587951 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #587951 - Flags: review? → review?(gavin.sharp)

Comment 14

5 years ago
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
(Assignee)

Comment 15

5 years ago
Comment on attachment 587951 [details] [diff] [review]
patch

This patch caused browser_bug386835.js to time out on Try.  Investigating.
Attachment #587951 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

5 years ago
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
Attachment #587951 - Attachment is obsolete: true
Attachment #588036 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #588036 - Flags: review? → review?(gavin.sharp)
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).
(Assignee)

Comment 18

5 years ago
(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

5 years ago
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 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?
Attachment #588036 - Flags: review?(gavin.sharp) → review-
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...
Blocks: 679784
No longer blocks: 416661, 487656
Though that change landed much after this bug was filed, obviously, so the original cause may be unrelated.
(Assignee)

Comment 23

5 years ago
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.
Attachment #588036 - Attachment is obsolete: true
Attachment #588306 - Flags: review?
(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?
(Assignee)

Updated

5 years ago
Attachment #588306 - Flags: review? → review?(gavin.sharp)
(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.
Attachment #588306 - Flags: review?(gavin.sharp) → review+
(but it'd be nice to have the test cover that case too)
(Assignee)

Comment 27

5 years ago
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.
Attachment #588306 - Attachment is obsolete: true
Attachment #588558 - Flags: review?(gavin.sharp)
(Assignee)

Comment 28

5 years ago
Comment on attachment 588558 [details] [diff] [review]
patch v4

Wait, missed another test failure... :(
Attachment #588558 - Flags: review?(gavin.sharp)

Comment 29

5 years ago
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

5 years ago
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
(Assignee)

Comment 31

5 years ago
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.)
Attachment #588558 - Attachment is obsolete: true
Attachment #588588 - Flags: review?(gavin.sharp)

Comment 32

5 years ago
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.
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 on attachment 588588 [details] [diff] [review]
patch v5

We should switch this to using mozSyntheticDocument, but that should happen in another bug.
Attachment #588588 - Flags: review?(gavin.sharp) → review+
(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.
(Assignee)

Comment 36

5 years ago
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.
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/1f6244d044aa
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 722624
You need to log in before you can comment on or make changes to this bug.