722624 - Zoom level become 1.0 after viewing Image Document when browser.zoom.siteSpecific=false

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/e33539a90ae2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120130 Firefox/12.0a1 ID:20120130031142

Zoom level become 1.0 after viewing Image Document when browser.zoom.siteSpecific=false

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with clean profile
2. set browser.zoom.siteSpecific=false
3. Open any HTML page
4. Zoom-In or Zoom-out
5. Open Image document in the same tab
6. Navigation back or Open any other HTML page in the same tab

Actual Results:
  Zoom level become 1.0.
  
Expected Results:
  Zoom level should not be reset. 
  Browser should preserve zoom level every tab.

Comment 1

[Matt Brubeck (:mbrubeck)]

I'll try to work on a fix for this if I have time.  Or if someone else wants to work on it, let me know.  The patch from bug 575830 should give you a good idea of where to start.

Comment 2

[bryant rojas]

I was wondering if I could pick this up and work on it? 
At the moment, while browser.zoom.siteSpecific=false
the zoom level does not reset back to 1.0 after opening an image document or opening another HTML page. However the zoom level is not preserved going from tab to tab. 

So the only thing would be to preserve zoom level switching back and forth through tabs. Is this still needed?

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to bryant rojas from comment #2)
> I was wondering if I could pick this up and work on it? 
> At the moment, while browser.zoom.siteSpecific=false
> the zoom level does not reset back to 1.0 after opening an image document or
> opening another HTML page.

What about if you follow the exact steps from comment 0, without switching tabs -- open an HTML document, change the zoom level, then open an image document, then go back to the HTML document?  The expected result is that the zoom level is set to 1.0 when you open the image document, but gets restored when you go back to the HTML document.

> So the only thing would be to preserve zoom level switching back and forth
> through tabs. Is this still needed?

No, with site-specific zoom disabled, I don't think zoom level should be preserved across multiple tabs.  This bug is about what happens when navigating within a single tab.

Comment 4

[Jason Tackaberry]

A related issue: if in step 6 instead of navigating back you simply click the image, it zooms to the previous zoom level that the site had.  (Clicking it again doesn't toggle or anything, it remains zoomed at this level.)

This doesn't happen with siteSpecific=true.

A NoSquint user reported this as a bug in NoSquint, since we need to set siteSpecific=false to implement custom zoom logic.

Comment 5

[Bellindira Castillo [:bellindira]]

Attachment: Patch v1

Basically, the proposed solution is to save the tab zoom level in a browser attribute (browser.content.full-zoom) before zoom level is set to 1 (when an Image Document is opened in the same tab), and then we could use this value to reset the zoom level when the user navigates back or open any other HTML page in the same tab.

This patch doesn't solve the related issue described by Jason Tackaberry.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 672712 [details] [diff] [review]
Patch v1

"browser.content.full-zoom" doesn't make sense as an attribute name. I'm not sure why you'd use an attribute in the first place, rather than a property or even better a WeakMap stored on the FullZoom object. That said, see below...

>     // Media documents should always start at 1, and are not affected by prefs.
>     if (!aIsTabSwitch && browser.contentDocument.mozSyntheticDocument) {
>       ZoomManager.setZoomForBrowser(browser, 1);
>       return;
>     }

Can we just make this block conditional on this.siteSpecific being true? For !this.siteSpecific we'd have to decide what's worse, images being zoomed or viewing an image resetting the zoom level to 1 when viewing an image.

I'm not a fan of adding any complexity to this brittle code just for a hidden pref.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #6)
> For
> !this.siteSpecific we'd have to decide what's worse, images being zoomed or
> viewing an image resetting the zoom level to 1 when viewing an image.

I think I'm leaning towards the former being worse, i.e. WONTFIXing this bug...

Comment 8

[Bellindira Castillo [:bellindira]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 672712 [details] [diff] [review]
> Patch v1
> 
> "browser.content.full-zoom" doesn't make sense as an attribute name. I'm not
> sure why you'd use an attribute in the first place, rather than a property
> or even better a WeakMap stored on the FullZoom object. That said, see
> below...

I've used this name because this is the name used to identify the setting in the content prefs database when siteSpecific=true.

I've used an attribute because zoom value on siteSpecific=false has to be kept for each tab (e.g. first tab has zoom value of 1.5 and second tab has a zoom value of 1.3). The property used to keep this value is called zoom (ZoomManager). However when an Image Document is opened, the zoom value of the tab is set to 1, and the previous zoom (e.g 1.5) is lost. So, when the user navigates back or opens any other HTML page in the same tab, it is not possible to reset the zoom level to its previous value because the value was not saved somewhere. I think, other solution is to create other property on ZoomManager to keep this previous value but I don't know if it would be confusing.

> >     // Media documents should always start at 1, and are not affected by prefs.
> >     if (!aIsTabSwitch && browser.contentDocument.mozSyntheticDocument) {
> >       ZoomManager.setZoomForBrowser(browser, 1);
> >       return;
> >     }
> 
> Can we just make this block conditional on this.siteSpecific being true? For
> !this.siteSpecific we'd have to decide what's worse, images being zoomed or
> viewing an image resetting the zoom level to 1 when viewing an image.
> 
> I'm not a fan of adding any complexity to this brittle code just for a
> hidden pref.

I agree to keep this simple, it makes more sense to make this block conditional on this.siteSpecific=true but it is up to you. So please let me know how to proceed.