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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[philippe (part-time)]

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.

Comment 1

[Nochum Sossonko [:Natch]]

This is intentional, see bug 416661.

Comment 2

[philippe (part-time)]

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)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 4

[philippe (part-time)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> 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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 11

[Johnny Stenback (:jst)]

Blocking on this regression for 1.9.2.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I guess I should take this. Though, not yet quite sure how to fix this.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Bug 487656 made this worse. Even if I fix image document, Firefox code always
sets zoom level to 1.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 16

[Dão Gottwald [:dao]]

(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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[Dão Gottwald [:dao]]

(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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 21

[Johnny Stenback (:jst)]

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.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/dd0d84ec56d4

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +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.

Comment 25

[Dão Gottwald [:dao]]

The browser tests did what they did for a reason. Please get these changes reviewed.

Comment 26

[Dão Gottwald [:dao]]

(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.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 28

[Dão Gottwald [:dao]]

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

The original _Gecko_ bug is a big deal for Gecko consumers other than Firefox.

Comment 30

[Dão Gottwald [:dao]]

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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Dão Gottwald [:dao]]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

No, that's precisely the use case I'm aware of.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2ed961eaed87