Note: There are a few cases of duplicates in user autocompletion which are being worked on.

session restore restores text zoom instead of full zoom

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
Session Restore
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Spencer, Assigned: Simon Bünzli)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

10 years ago
Component: General → Session Restore
QA Contact: general → session.restore
Summary: image scaling not always correct → image scaling not always correct after session restore

Updated

10 years ago
Blocks: 4821
sounds like it could be a combination of text zoom and full zoom...
(Assignee)

Comment 2

10 years ago
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: image scaling not always correct after session restore → session restore restores text zoom instead of full zoom
Version: unspecified → Trunk
Definitely! The "Zoom" command now uses full zoom instead of text zoom.
(Assignee)

Comment 4

10 years ago
Created attachment 298961 [details] [diff] [review]
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.
Attachment #298961 - Flags: review?(dietrich)
Actually, it may make sense to support both textZoom and fullZoom due to bug 401322.
(Assignee)

Comment 6

10 years ago
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).
Depends on: 401322

Comment 7

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

Comment 8

10 years ago
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.
Attachment #298961 - Flags: review?(dietrich)

Comment 9

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

Comment 10

10 years ago
Created attachment 299144 [details] [diff] [review]
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.
Attachment #298961 - Attachment is obsolete: true
Attachment #299144 - Flags: review?(dietrich)
Attachment #299144 - Flags: review?(dietrich) → review+
(Assignee)

Updated

10 years ago
Attachment #299144 - Flags: approval1.9?
Comment on attachment 299144 [details] [diff] [review]
rip out zoom restoring

a=beltzner for 1.9
Attachment #299144 - Flags: approval1.9? → approval1.9+
Assignee: nobody → zeniko
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
Flags: in-litmus?
in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7799
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.