413433 - session restore restores text zoom instead of full zoom

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Spencer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

I accidentally scrolled up while closing a window.  Upon opening Firefox again (restore session: on) the text was scaled up but images weren't.  All sizes were treated as default and the normal sized images scaled down to very small images while the large text scaled down to normal sized text.

Reproducible: Sometimes

Steps to Reproduce:
1.  Ctrl+UpScroll+W on last window with restore session on
2.  Reopen Firefox
3.  Ctrl+Scroll
Actual Results:  
Images scaled all wanky

Expected Results:  
Images should have been rendered at the size they should have been (IE: scaled up with the text)

Default theme, nothing else of note.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sounds like it could be a combination of text zoom and full zoom...

Comment 2

[Simon Bünzli]

Not sure how text and full zoom interact, but FWIW we only save and restore text zoom. I guess the right thing would be to use full zoom instead...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Definitely! The "Zoom" command now uses full zoom instead of text zoom.

Comment 4

[Simon Bünzli]

Attachment: s/textZoom/fullZoom/

Since fullZoom is the new textZoom, it's IMO not worth it teaching SessionStore to distinguish these two modes - we should just switch to restore fullZoom now so that when migrating from Fx 2 people can still Ctrl+0 to revert to a non-zoomed page.

Comment 5

[Reed Loden [:reed]]

Actually, it may make sense to support both textZoom and fullZoom due to bug 401322.

Comment 6

[Simon Bünzli]

Supporting both would be confusing for people who upgrade because they might get a page which is stuck at a text zoom level they can't change through the UI (or at least not without toggling a pref - should one get added in bug 401322). WRT bug 401322 I'd then just use whichever that pref dictates (for the same reason).

Comment 7

[Elmar Ludwig]

Now that the current zoom level is remembered per site anyway (using the site- specific preferences), why is it necessary to save and restore the zoom level in SessionStore at all?

(In reply to comment #6)
> Supporting both would be confusing for people who upgrade because they might
> get a page which is stuck at a text zoom level they can't change through the UI

When the patch for bug 401322 lands, "View > Zoom > Reset" (or Ctrl + 0) will always reset both text and full zoom.

Comment 8

[Simon Bünzli]

Comment on attachment 298961 [details] [diff] [review]
s/textZoom/fullZoom/

(In reply to comment #7)
> why is it necessary to save and restore the zoom level in SessionStore at all?

Good question. AFAICT the only case where SessionStore would restore a different zoom setting than the site-specific prefs indicate is when restoring older sessions through a session managing extension. We could leave deciding what to do in that case to the extension, though...

So ripping out those two zoom-related lines would be fine by me.

Comment 9

[Elmar Ludwig]

(In reply to comment #8)
> Good question. AFAICT the only case where SessionStore would restore a
> different zoom setting than the site-specific prefs indicate is when restoring
> older sessions through a session managing extension.

Wouldn't in this case the site-specific pref override the one restored by the extension, at least when switching to a different tab and switching back, since the site-specific one is always applied when the URL changes?

> So ripping out those two zoom-related lines would be fine by me.

I think so too.

Comment 10

[Simon Bünzli]

Attachment: rip out zoom restoring

(In reply to comment #9)
> Wouldn't in this case the site-specific pref override the one restored by the
> extension, at least when switching to a different tab and switching back, 

At first thought, that's so counter-intuitive that I didn't even consider it. But as it currently stands, restoring zoom indeed doesn't make sense for SessionStore at all:

Even if we used the ZoomManager to retrieve/restore zoom settings, restoring an older session would then just overwrite the site specific pref which IMO would be a rather unwanted side-effect. Let's rip zoom restoring out, then.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 299144 [details] [diff] [review]
rip out zoom restoring

a=beltzner for 1.9

Comment 12

[Reed Loden [:reed]]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.93; previous revision: 1.92
done

Comment 13

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7799