513912 - Fix image zoom in Firefox

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Bug 503729 fixed image zoom handling in gecko, but Firefox may want to do something else what gecko does by default.

In Bug 503729 some Firefox tests had to be changed. Should make sure those changes
are what we want.

I think this bug should end up removing all the cases where image document's zoom
level is automatically set to 1.

Comment 1

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

Attachment: patch

This is the behavior I'd prefer.
(But I guess someone will disagree with me.)

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 397874 [details] [diff] [review]
patch

I think we should load images at 100% in the siteSpecific case, as we store the previous zoom level and thus won't encounter bug 503729.

Comment 3

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

Attachment: like this?

Comment 4

[Dão Gottwald [:dao]]

Yeah, although the test changes from bug 503729 still look wrong.

Comment 5

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

Attachment: patch

Had to tweak the gecko part a bit, so that the zoom level doesn't get reset
at wrong time.
Doesn't regress Bug 503729.

Comment 6

[Dão Gottwald [:dao]]

Something is still not right in the gecko part. If I select "View Image" for a small image on a zoomed page, the image gets loaded with zoom level 1, but will resize when being clicked on. If I do the same for a big image, it will be zoomed to fill the content area are correctly, but clicking it will zoom it to the page's level, and clicking it again won't make it fill the content are correctly.

Comment 7

[Dão Gottwald [:dao]]

... both of which are regressions from your gecko changes in comment 5, I think.

Comment 8

[Dão Gottwald [:dao]]

Also, the early return for !this.siteSpecific regresses bug 487656:
browser_privatebrowsing_zoomrestore.js | Zoom level for about:privatebrowsing should be reset - Got 1.100000023841858, expected 1

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #7)
> ... both of which are regressions from your gecko changes in comment 5, I
> think.

Well, said cases are also broken without that change, although in a slightly different manner. The patch from bug 503729 should be backed out, imho.

Comment 10

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

Well, IMHO, Bug 487656 should be backed out.
Anyway, I'm trying to fix this all now in this bug.

Comment 11

[Dão Gottwald [:dao]]

Bug 487656 is unrelated to the problems mentioned in comment 6.

Comment 12

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

(In reply to comment #8)
> Also, the early return for !this.siteSpecific regresses bug 487656:
> browser_privatebrowsing_zoomrestore.js | Zoom level for about:privatebrowsing
> should be reset - Got 1.100000023841858, expected 1
I've fixed other cases, but this is still something to do.
The bug is that browser-fullzoom handles sitespecific and private browsing the same way.
I wonder what to do with that. Probably have to split FullZoom_get_siteSpecific to two different getters. (This is sort of a regression from Bug 487656)

Comment 13

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

Attachment: patch

Waiting for test results

Comment 14

[Dão Gottwald [:dao]]

Won't this regress bug 503729?

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Won't this regress bug 503729?

FWIW, I'm fine with breaking browser.zoom.siteSpecific, but it still seems that the backend code should be able to handle this better, and without a pref.

Comment 16

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

(In reply to comment #14)
> Won't this regress bug 503729?
Um, did I upload something wrong. Testing...

(In reply to comment #15)
> but it still seems that
> the backend code should be able to handle this better, and without a pref.
All the suggestions how backend could handle this in a good way are very welcome.

By default (in gecko, not necessarily in FF) image documents shouldn't change
full page zoom and the next page should have the same zoom as the previous page.

Comment 17

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

Attachment: compomise

Not perfect, but perhaps good enough even if FF user has
browser.zoom.siteSpecific=false (like I do).

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> Not perfect, but perhaps good enough even if FF user has
> browser.zoom.siteSpecific=false (like I do).

So with siteSpecific=false, you load the image zoomed (expected), but clicking it will set the zoom level to 1, and going back gives you bug 503729.

However, if I also set browser.reset_image_document_zoom to false, that's not a problem. Can the Gecko code just read browser.zoom.siteSpecific instead?

Comment 19

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

(In reply to comment #18)
> So with siteSpecific=false, you load the image zoomed (expected), but clicking
> it will set the zoom level to 1, and going back gives you bug 503729.
Well, "clicking" isn't part of bug 503729.

> 
> However, if I also set browser.reset_image_document_zoom to false, that's not a
> problem.
right.

> Can the Gecko code just read browser.zoom.siteSpecific instead?
Well, what has browser.zoom.siteSpecific to do with image documents?

Comment 20

[Dão Gottwald [:dao]]

browser.zoom.siteSpecific=true implies that the zoom level for the image document can safely be set to 1, as the browser stored the zoom level of the previous page.

Comment 21

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

(In reply to comment #20)
> browser.zoom.siteSpecific=true implies that the zoom level for the image
> document can safely be set to 1
Why does that imply such thing?
I'd just take the new pref.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> (In reply to comment #20)
> > browser.zoom.siteSpecific=true implies that the zoom level for the image
> > document can safely be set to 1
> Why does that imply such thing?

Because the browser stored the zoom level of the previous page. In that case, bug 503729 is not an issue.

> I'd just take the new pref.

Me too, if siteSpecific=false + reset_image_document_zoom=true wasn't broken.

Comment 23

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

(In reply to comment #22)
> Because the browser stored the zoom level of the previous page. In that case,
> bug 503729 is not an issue.
I have no idea how this says that browser.zoom.siteSpecific=true implies that
image document zoom should be 1. It is just a FF feature that
siteSpecific happens to want image zoom to default to 1.

> Me too, if siteSpecific=false + reset_image_document_zoom=true wasn't broken.
It isn't really broken. It has the "feature" to reset zoom if image document's "click zoom" is used. 
And in any case, the normal FF user should get the default handling:
siteSpecific+resetZoom, and non-FF should have !siteSpecific+!resetZoom.

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> I have no idea how this says that browser.zoom.siteSpecific=true implies that
> image document zoom should be 1. It is just a FF feature that
> siteSpecific happens to want image zoom to default to 1.

I just don't think loading images zoomed makes any sense, as I've said multiple times elsewhere. It seems that we only do it now because of bug 503729.

> It isn't really broken. It has the "feature" to reset zoom if image document's
> "click zoom" is used. 

Traditionally only if the image overflows the content area. Also, a zoom cursor is expected to appear and another click is expected to set the zoom level back.

Comment 25

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

Note, other gecko users may want to disable the whole image document zoom and
just use full page zoom. And in that case loading images zoomed, if the
previous page was zoomed makes a lot of sense (because full page zoom is per docshell not per page by default).

Comment 26

[Dão Gottwald [:dao]]

browser.reset_image_document_zoom is not what these gecko users need, though, is it?

Comment 27

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

No, but FF needs that.

Comment 28

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

Because it has its own zoom handling, which depends on .siteSpecific, private browsing mode, image document, and print preview.

Comment 29

[Dão Gottwald [:dao]]

FF would be satisfied if that code just read browser.zoom.siteSpecific. It seems that another pref could be added for comment 25 independently.

Comment 30

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

But image documents don't have anything to do with "siteSpecific".
It is just a firefox feature to provide zoom level 1 for image documents.

Comment 31

[Dão Gottwald [:dao]]

It is indeed a Firefox feature. But another browser could implement a similar feature and set the pref. The site-specific behavior relates to bug 503729, which relates to image documents, but we already discussed this.

Comment 32

[Dão Gottwald [:dao]]

(In reply to comment #23)
> And in any case, the normal FF user should get the default handling:
> siteSpecific+resetZoom, and non-FF should have !siteSpecific+!resetZoom.

So is there a use case for siteSpecific+!resetZoom or !siteSpecific+resetZoom? If not, why allow it? There's also no migration path for users with !siteSpecific.

Comment 33

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

siteSpecific is Firefox only and has nothing to do with image documents (except in Firefox)

Comment 34

[Dão Gottwald [:dao]]

I suggest we make siteSpecific not Firefox-only. As I said in comment 31, another browser could implement this and accordingly set the pref. If there is no use for the resetZoom pref without the site-specific behavior, then the extra pref will just annoy users.

Comment 35

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

Well are you willing to implement siteSpecific in gecko? Or are you saying
we should just add that pref to gecko, although gecko knows nothing about
siteSpecific? I guess the latter, which doesn't make sense, IMO.

I agree adding new prefs sucks, but in this case I haven't figured out anything
better.

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #35)
> Well are you willing to implement siteSpecific in gecko? Or are you saying
> we should just add that pref to gecko, although gecko knows nothing about
> siteSpecific?

I suggest we make browser.zoom.siteSpecific the way for the Gecko user to tell Gecko "I'm implementing site-specific zoom", and that Gecko in that case loads images with a zoom level of 1, knowing that this won't cause bug 503729, because the Gecko user implemented site-specific zoom.

> I guess the latter, which doesn't make sense, IMO.

In contrast I think supporting browser.reset_image_document_zoom alone doesn't make sense. Flipping it without doing site-specific zoom would cause bug 503729.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 398243 [details] [diff] [review]
compomise

I keep saying that this produces broken behavior (second part of comment 24), and I don't think it's reasonable to expect existing and new users of browser.zoom.siteSpecific to figure out that they have to flip browser.reset_image_document_zoom. (Heck, it wasn't even immediately obvious to me when I tried the patch the first time. I expected it to do something reasonable with any combination of the two prefs.)

At that point me would have made these users jump through enough hoops that they'd be better served by an extension, so I'd rather remove browser.zoom.siteSpecific altogether if we can't fix it reasonably (although I think we can, see previous comment).

Comment 38

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

Attachment: using the siteSpecific pref in gecko

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 399174 [details] [diff] [review]
using the siteSpecific pref in gecko

Image zoom still doesn't quite work with siteSpecific=false. For example, if you zoom in on a page and load a large image, it will be resized as if the zoom level was 1 and thus still overflow the content area initially rather than fitting into it. Pretty useless. The browser-specific changes look fine though, and image zoom with siteSpecific=true works like a charm.

Comment 40

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

(In reply to comment #39)
> (From update of attachment 399174 [details] [diff] [review])
> Image zoom still doesn't quite work with siteSpecific=false. For example, if
> you zoom in on a page and load a large image, it will be resized as if the zoom
> level was 1 and thus still overflow the content area initially rather than
> fitting into it.
That is how it should work, when gecko doesn't change zoom level between document
loads.

Comment 41

[Dão Gottwald [:dao]]

From a user's perspective, it's hard to understand why the browser just zoomed that image to 37% if it still overflows the content area. Even if you manage to understand it, it doesn't seem useful. I'd expect to see the image either fitting into the content area or at its original size with the appropriate zoom level (1 or the unmodified one).

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 399174 [details] [diff] [review]
using the siteSpecific pref in gecko

>   _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue, aBrowser) {
>     var browser = aBrowser || gBrowser.selectedBrowser;
> 
>-    var resetZoom = (!this.siteSpecific || gInPrintPreviewMode ||
>-                     browser.contentDocument instanceof Ci.nsIImageDocument);
>+    if (!this.siteSpecific && !this._inPrivateBrowsing)
>+      return;

This could now be moved above the var browser declaration.

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 399174 [details] [diff] [review]
using the siteSpecific pref in gecko

sr=jst, and I think we should take this for 1.9.2.

Comment 44

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

http://hg.mozilla.org/mozilla-central/rev/82575e77ef96

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #42)
http://hg.mozilla.org/mozilla-central/rev/8afb65205203

Comment 46

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

Attachment: 1.9.2

Comment 47

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3f43e96a2da4