with browser.zoom.siteSpecific=false: viewing a standalone image resets full zoom level

RESOLVED FIXED

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: philippe (part-time), Assigned: smaug)

Tracking

({regression})

Trunk
x86
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
STR 
1. disable browser.zoom.siteSpecific (set it to false)
2. visit http://mozilla.com
3. zoom in a few times (full zoom/page zoom)
4. right-click on Firefox logo, --> view image (in same tab)
5. go back to the (html) page

Actual results: zoom level is lost (reset to default)
Expected results: zoom level is remembered.

This works correctly if 'text-zoom' is used.
This is intentional, see bug 416661.
(Reporter)

Comment 2

9 years ago
If I understand bug 416661 correctly, this is not what I'm talking about. 

In this case: zoom in on a html document. View an image as standalone (the image is not zoomed, that is OK). Go back (back button) to the original html page: the zoom level is lost. I think that is wrong.

The behaviour exists in Gecko 1.9.0 and up.

(It is currently annoying users of Camino 2b, which doesn't support browser.zoom.siteSpecific - it then seems to be treated as false by Gecko)
Yeah, this is not bug 416661; a) this is not browser/ code, but rather something affecting all products, and b) this is about the image document causing Gecko to completely forget any previous tab zoom setting entirely, rather than being about not applying a site-specific zoom setting to an image as bug 416661 is.
(Reporter)

Comment 4

9 years ago
Real world use case:
http://www.archive.org/iathreads/post-view.php?id=253825
zoom in because text is too small, view attachments (links to standalone images), come back to the page.

via the forums:
http://forums.mozillazine.org/viewtopic.php?f=12&t=1350335
This looks like a regression from bug 389756.  Image document resets the zoom to 1.0 when it gets loaded, and since the zoom has been set to not be site-specific, it's set on every page load to be the same as for the previous page.  See the SetFullZoom calls in nsDocShell::RestoreFromHistory and nsDocShell::SetupNewViewer.

Given that docshell behavior, I think that the fix for bug 389756 was just completely wrong.
Blocks: 389756
Smaug, would just setting the zoom on the prescontext but NOT the document viewer do the right thing here?

How does the current behavior interact with the user changing page zoom on the image?
Component: Layout → DOM
Flags: blocking1.9.2?
Keywords: regression
QA Contact: layout → general
> How does the current behavior interact with the user changing page zoom on the
> image?

Going back, as well as moving on to another page/site/etc. (via location bar, bookmark, or other method of loading a new page), preserves the zoom factor that was set on the image document.

Also, this sequence

visit page 1, visit image, visit page 2, increase zoom, go back to image, go back to page 1

preserves the zoom factor set on page 2 when going back to both the image document and page 1.

It seems (but perhaps there are some other scenarios here I'm not thinking of right now which also need to be tested) that the only time the content zoom setting is thrown away is when going forward (doing an actual "pageload"--whatever the technical term for that is--whether via link-click or typing in the location bar) from a zoomed page to an image document.
> preserves the zoom factor set on page 2 when going back to both the image
> document and page 1.

This probably depends on whether the image document got bfcached.  The zoom is reset every time that document is set up fresh, basically.

The question was more targeted at Olli, though....
(In reply to comment #6)
> Smaug, would just setting the zoom on the prescontext but NOT the document
> viewer do the right thing here?
No, because zoom in/out (using ctrl+/-) the image would set the content viewer's zoom level.

I'm not sure what the behavior should be. Certainly image document shouldn't
affect to the zoom level if page zoom isn't used, but what if the page zoom is used. Should it still set the content viewer's zoom level?

Perhaps image document's zoom handling shouldn't affect to anything out side it.
It could always start from 1.0 (or the zoom level of previous page?).
Then page zoom should work as with normal pages, but using image zoom would
reset page zoom to original.

Loading image document from bfcache should probably keep the zoom level it had
when going in to cache. Same thing wouldn't work if the document wasn't bfcached.
Created attachment 388210 [details] [diff] [review]
wip

This doesn't quite work, since unload resets zoom level too early, so the image
gets zoomed just before new page is loaded. And we should probably try to
keep the image document in bfcache. Unload listener disables bfcache.
Blocking on this regression for 1.9.2.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
I guess I should take this. Though, not yet quite sure how to fix this.
Assignee: nobody → Olli.Pettay
Bug 487656 made this worse. Even if I fix image document, Firefox code always
sets zoom level to 1.
Blocks: 487656

Updated

9 years ago
Summary: with [browser.zoom.siteSpecific=false]: viewing an image (standalone) resets full/page zoom level → with browser.zoom.siteSpecific=false: viewing a standalone image resets full zoom level
Created attachment 394253 [details] [diff] [review]
patch

This makes the behavior more predictable. Full zoom keeps its value even in
image documents. And also the bfcache handling works like with other pages.

Note, this fixes the problem in gecko, but doesn't fix the regression Bug 487656
caused in Firefox.
Attachment #388210 - Attachment is obsolete: true
Attachment #394253 - Flags: review?(jst)
Created attachment 394254 [details] [diff] [review]
patch
Attachment #394253 - Attachment is obsolete: true
Attachment #394254 - Flags: review?(jst)
Attachment #394253 - Flags: review?(jst)
(In reply to comment #14)
> This makes the behavior more predictable. Full zoom keeps its value even in
> image documents.

Even though predictable, this doesn't sound right to me.
So you'd want image documents to start with zoom level 1?
Why exactly?

If the zoom level is the same as in the previous page, contextmenu->ViewImage
works more reasonable way.
I agree that if a page has a link to some big image, zoom level 1 might be
better when that image is loaded.
Shouldn't zoom level just be set by the app?  The key to this bug is that core code was overriding app code's zoom level setting, right?
(In reply to comment #17)
> So you'd want image documents to start with zoom level 1?
> Why exactly?

Because images have a natural size. Without the page as the context, I don't think it makes sense to load an image zoomed.

Updated

9 years ago
No longer blocks: 487656
Depends on: 510187
(In reply to comment #18)
> Shouldn't zoom level just be set by the app?  The key to this bug is that core
> code was overriding app code's zoom level setting, right?
Right. Now ImageDocument doesn't ever set zoom level. But uses the original zoom level so that https://bugzilla.mozilla.org/show_bug.cgi?id=389756&mark=0#c0 isn't regressed.
Status: NEW → ASSIGNED
Comment on attachment 394254 [details] [diff] [review]
patch

r=jst. I'd say lets take this and file followup bugs if we want this to work somehow differently, since I think this gets us closer to what we want than what we have right now.
Attachment #394254 - Flags: review?(jst) → review+
http://hg.mozilla.org/mozilla-central/rev/dd0d84ec56d4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Backed out because there is a test which relies on the wrong behavior.
Will fix the test.
http://hg.mozilla.org/mozilla-central/rev/01bf91c74490
Created attachment 397848 [details] [diff] [review]
+test fixes

http://hg.mozilla.org/mozilla-central/rev/22304c45a4c7

So this fixed gecko behavior, but Firefox behavior may need tweaking.
Please file a followup bug if that is the case.
The browser tests did what they did for a reason. Please get these changes reviewed.
Depends on: 513912
(In reply to comment #25)
> The browser tests did what they did for a reason. Please get these changes
> reviewed.

And spinning that off to another bug is maybe not the right way forward. From the test changes it looks like this fix puts it in an inconsistent state that's worse than what this bug was about*. Even if that inconsistency is OK for now (I don't think it is), at the very least the test changes shouldn't have adjusted the expected results to that nonsensical behavior but used todo().

*: As a reminder, page -> image -> back to page should keep the page's original zoom level; the bug report was not about the image not using the page's zoom level.
(In reply to comment #26)
> *: As a reminder, page -> image -> back to page should keep the page's original
> zoom level; the bug report was not about the image not using the page's zoom
> level.
Sure, but the most reasonable fix on gecko side I came up was to fix
image document zoom handling to work consistently with other documents.
I understand that part. But we need to make sure that the implications of that fix are well-understood and correctly being dealt with. If the implications are worse than the original bug, then maybe the fix should be backed out. I'd also suggest rethinking whether this needs to block 1.9.2, as the original bug doesn't seem like a big deal.
The original _Gecko_ bug is a big deal for Gecko consumers other than Firefox.
In which ways is it a big deal for them? Bug 389756 was fixed on 1.9, I think... why has no other Gecko consumer complained about it?
Because most other Gecko consumers that are shipping browsers (Camino, Seamonkey) are only now getting to shipping a browser based on 1.9 or later.
So we're talking about browsers, that's good to know. I thought maybe there were use cases other than page -> image -> back to page.
No, that's precisely the use case I'm aware of.
Depends on: 515298
You need to log in before you can comment on or make changes to this bug.