Closed Bug 510187 Opened 15 years ago Closed 8 years ago

browser.zoom.siteSpecific=false and image documents and print preview set zoom level to 1

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: smaug, Unassigned)

References

Details

(Keywords: regression) 

This is a regression from bug 487656.
Flags: blocking-firefox3.6?
At least the print preview part is intentional. Before bug 487656, print preview for zoomed pages was broken.
Keywords: regression
It can't possibly be intentional that going in and out of print preview loses your zoom level on the original document.   Unzooming on print preview and rezooming on exit would make sense, of course.
Blocks: 503729
(In reply to comment #2)
> It can't possibly be intentional that going in and out of print preview loses
> your zoom level on the original document.

Going out of print preview is expected to restore the zoom level.
I(In reply to comment #1)
> At least the print preview part is intentional. Before bug 487656, print
> preview for zoomed pages was broken.
In which way broken? Bug#?
Print preview has its own zoom level, which is initially 1.
(In reply to comment #4)
> I(In reply to comment #1)
> > At least the print preview part is intentional. Before bug 487656, print
> > preview for zoomed pages was broken.
> In which way broken? Bug#?

See bug 487656 comment 47 and bug 487656 comment 48. There's no bug for that, as far as I know.
I also think the image document part is expected (see bug 503729 comment 19; we also have a browser chrome test for this), except that going back to a previously zoomed page should ideally restore that page's zoom level.
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
(In reply to comment #1)
> At least the print preview part is intentional. Before bug 487656, print
> preview for zoomed pages was broken.
Anyway, would be good to file a (core) bug to fix this in the right place.
Depends on: 510465
IINM, bug 487656 comment 68 is about a different problem than this bug.  Am I missing something?
How is bug 487656 comment 68 not about this bug?
Bug 487656 made Firefox to set zoom level to 1 whenever loading a new page
(if browser.zoom.siteSpecific=false or image document load or print preview)
Before bug 487656, the code had just 'return', now it has something like
if (!this.siteSpecific || gInPrintPreviewMode ||
    browser.contentDocument instanceof Ci.nsIImageDocument);
  ZoomManager.setZoomForBrowser(browser, 1);
(In reply to comment #9)
> How is bug 487656 comment 68 not about this bug?
> Bug 487656 made Firefox to set zoom level to 1 whenever loading a new page
> (if browser.zoom.siteSpecific=false or image document load or print preview)
> Before bug 487656, the code had just 'return', now it has something like
> if (!this.siteSpecific || gInPrintPreviewMode ||
>     browser.contentDocument instanceof Ci.nsIImageDocument);
>   ZoomManager.setZoomForBrowser(browser, 1);

Oh, sorry for the confusion.  I stand corrected.

But I think this needs a UI decision.  Let's see what Alex thinks here.
Keywords: uiwanted
(In reply to comment #9)
> How is bug 487656 comment 68 not about this bug?
> Bug 487656 made Firefox to set zoom level to 1 whenever loading a new page
> (if browser.zoom.siteSpecific=false or image document load or print preview)
> Before bug 487656, the code had just 'return'

Before bug 487656, image documents also set their zoom level to 1 independently. You're going to undo that in bug 503729.
(In reply to comment #11)
> Before bug 487656, image documents also set their zoom level to 1
> independently.
Yes, that is a bug I caused, and that is a bug I'm fixing in bug 503729.
Bug 503729 comment 0 is about the zoom level not being restored when going back to a page that was zoomed. It doesn't complain about images not using that page's zoom level.
Well, if Firefox wants to set the zoom level to 1, it can.
What I'm more concerned is browser.zoom.siteSpecific=false.
Though, even in that case it is up to the gecko user (Firefox) to do whatever
it wants. But for gecko I'd like to fix the zoom handling the way the patch in
bug 503729 does.
>Oh, sorry for the confusion.  I stand corrected.
>But I think this needs a UI decision.  Let's see what Alex thinks here.

Giving these comments and bug 487656 a quick read I'm not sure I completely understand the issue, but overall there should be no visible connection (at least at an interface level) between the zoom of a page in the content area and that same information in a print preview dialog.  Hopefully that helps clear up the intended UI, but if not can you describe the current issue?
Alex, I think the main question here is that how should we handle
browser.zoom.siteSpecific=false. Should the tab keep the same zoom level
for every page loaded in it. Gecko does that, but
trunk FF resets zoom level to 1.
>I think the main question here is that how should we handle
>browser.zoom.siteSpecific=false

Is there anyway for the user to get that setting without directly changing it through about:config?
(In reply to comment #18)
> >I think the main question here is that how should we handle
> >browser.zoom.siteSpecific=false
> 
> Is there anyway for the user to get that setting without directly changing it
> through about:config?

No.
>the main question here is that how should we handle
>browser.zoom.siteSpecific=false. Should the tab keep the same zoom level
>for every page loaded in it. Gecko does that, but
>trunk FF resets zoom level to 1.

Myk: could you answer this?  I'm not all that familiar with how these about:config prefs were intended to behave.

Since this doesn't really touch the UI (about:config void your warranty I hear) I'm really fine with people just doing whatever seems logical.
Currently when browser.zoom.siteSpecific=false reloading a page resets zoom level
to 1. That "feels" pretty wrong to me.
The browser.zoom.siteSpecific preference was added in bug 419609.  The primary intent of the preference is to allow users to revert from site-specific zoom to the previous behavior of Firefox in which the zoom level, once set for a given tab, remains at that level until changed again by the user, i.e. tab-specific zoom.

So it is indeed a regression and incorrect behavior for Firefox to now reset the zoom level to 1 each time a user browses to a new page when browser.zoom.siteSpecific is set to false.  Instead, Firefox should leave the zoom level at its existing value (as persisted across page loads by Gecko) in that case.
I'm not sure how to fix this bug.  I removed the resetZoom code in browser-textZoom.js but still when I go to a page, zoom it and go to an image document, the image document appears with the default zoom level, and hitting back will reset the zoom level of that page as well.
Bug 503729 fixes the image zoom part in gecko. But it doesn't affect to 
browser.zoom.siteSpecific=false handling.
Print preview was fixed in bug 510465.
(In reply to comment #24)
> Bug 503729 fixes the image zoom part in gecko. But it doesn't affect to 
> browser.zoom.siteSpecific=false handling.
> Print preview was fixed in bug 510465.

Thanks for the pointer.  I'll give it another shot once bug 503729 is fixed.
Is it worth maintaining browser.zoom.siteSpecific anyway? Looks like nobody on trunk noticed the change from bug 487656, except for Olli after looking at the code. Would be nice if we had data about how many users touched that pref, but I'm sure we haven't.
about:config prefs aren't the subject of "uiwanted", removing keyword.

(In reply to comment #26)
I think it is worth it to keep the siteSpecific pref, with the correct set of tests to ensure it isn't manhandled or misinterpreted in the future. I'm sure there's a substantial subset of users that would like to keep the "old behavior" (as there always is), and this isn't all that much to maintain. Obviously this all depends on the complexity of the fix, etc. I'm just pointing out that this pref isn't "another choice for the user" rather it's restoring an older previous behavior users experienced with Firefox. IMHO.
Keywords: uiwanted
(In reply to comment #26)
> Is it worth maintaining browser.zoom.siteSpecific anyway?

It's hard to say.  I think we do want tab-specific zoom mode to be available, although it should be an extension rather than a hidden pref if very few users want to use that mode.  I'm not sure how to get the data to make that determination, though.
Depends on: 513912
Flags: blocking-firefox3.6? → blocking-firefox3.6-
Cannot Reproduce Mac 10.11 and Windows 10
Version 	47.0.1
Build ID 	20160623154057
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Confirmation: Steps tested
1) set browser.zoom.siteSpecific to false (also tested "true")
2) access https://developer.mozilla.org/en-US/Firefox
3) zoom the page (170%)
4) Applied Print Preview
5) Returned to the original page and zoom level remained
If I have misunderstood the issue, feel free to reopen and adjust steps as needed, thanks.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.