416661 - Site-specific zoom level shouldn't apply to image documents

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

It is quite annoying to see normal pages zoomed when using pagezoom to zoom 
images.
Perhaps there should be !(event.target.ownerDocument instanceof Components.interfaces.nsIImageDocument)
check somewhere?

Comment 1

[Nochum Sossonko [:Natch]]

Attachment: Like so?

This seems to work for me (if I understand this bug correctly).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

That prevents us from setting a pref *from* an image document, but I think what we really want is to avoid applying any existing prefs *to* image documents, right? That means the check needs to be in _applySettingToPref.

Shouldn't be too hard to write a browser chrome test for this as well.

Comment 3

[Nochum Sossonko [:Natch]]

Attachment: Err... gonna have to go both ways

Comment 4

[Nochum Sossonko [:Natch]]

browser chrome test coming... I'll post it seperately.

Comment 5

[Nochum Sossonko [:Natch]]

Attachment: Yet another oversight :(

Have to check in _removePref as well (good thing is I found this while preparing the browser chrome test). I'll upload the tests soon(hopefully, having some problems, gonna try to straighten them out now).

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: Here's what I've been doing so far.

Having problems with this, it doesn't work :(

Comment 7

[Nochum Sossonko [:Natch]]

BTW should this be changed for a video document (ogg video) now that it landed?

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: Utter Stupidity :( ...

Ok here's the browser chrome test, forgot to addEventListener on the linkedBrowser... :(

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #7)

If you're asking whether the bug should morph to be "...apply to video/audio/image documents", I'd think so as video fullscreening better addresses the need to enlarge playback size.  I haven't followed video controls discussions closely enough to know whether fullscreening will have obvious UI (I see none in a recent trunk build), but I'd be astounded if it won't.

Comment 10

[Nochum Sossonko [:Natch]]

(In reply to comment #9)
I don't really understand what that means, the patch in this bug doesn't disable zooming for images, it just doesn't update the zoom preference for the domain and doesn't get affected by the domain's preference. If zooming for video/audio should be entirely disabled that would probably be a different bug. Then again, maybe I'm just being dense...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 345916 [details] [diff] [review]
Utter Stupidity :( ...

>diff --git a/browser/base/content/test/browser_bug416661.js b/browser/base/content/test/browser_bug416661.js

>+var tabDoc, zoomLevel;

"tabDoc" is kind of an odd name for something that holds a <xul:tab>.

>+function start_test_prefNotSet() {

>+  FullZoom.enlarge();
>+
>+  //capture the zoom level to test later
>+  zoomLevel = ZoomManager.zoom;

Might add in a test to make sure that enlarge() caused the zoomLevel to change? And add a test for the initial zoom being 1?

>+function continue_test_prefNotSet () {

>+  var isGlobal = ZoomManager.zoom == 1;
>+  ok(isGlobal, "An image document should not be affected by the" + 
>+               " site specific zoom level ");
>+  FullZoom.reset();

Test that ZoomManager.zoom is still 1 here?

>+function end_test_prefNotSet() {
>+  isSiteSpecific = ZoomManager.zoom == zoomLevel;
>+  ok(isSiteSpecific, "The site specific zoom level should not be" +
>+                     " affected by an image document ");

Prefer is(a, b) rather than ok(a == b), since is() will print out actual/expected results for failures (this comment applies to a bunch of places in this test).

>+function test() {

>+  content.location = 
>+    "chrome://mochikit/content/browser/browser/base/content/test/zoom_test.html";

I guess this works because the content prefs service uses nsIURI.host, and for chrome URIs host is the package name ("mochikit")... It would be best to use one of the fake http://example.com URIs for this test instead, I think, so that we're testing the most common use case (and avoid problems if the CPS URI rules change).

r=me with those addressed.

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: Nits Addressed

All nits addressed, besides the one discussed on irc.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 345602 [details] [diff] [review]
Yet another oversight :(

Er, I forgot to post this earlier. I reviewed the patch and test separately, don't assume that just because I marked the test r+ I reviewed the patch as well!

>diff --git a/browser/base/content/browser-textZoom.js b/browser/base/content/browser-textZoom.js

>   _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue) {
>-    if (!this.siteSpecific || gInPrintPreviewMode)
>+    if (!this.siteSpecific || gInPrintPreviewMode ||
>+        content.document instanceof Ci.nsIImageDocument)

I wonder whether we want this change or not. Would it be useful for existing site specific prefs to apply to images by default? I suppose that might be confusing and/or annoying. r=me either way.

You should probably get ui-review for this change, not sure who would be best.

Comment 14

[Nochum Sossonko [:Natch]]

Comment on attachment 345602 [details] [diff] [review]
Yet another oversight :(

sorry about that, I had combined the two patches locally so I thought it was finished with review.
Beltzner: comment 13, do we want site-specific zoom prefs to apply to image documents?

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Does something similar to this need to be done for the new audio or video documents that we now use when directly loading video and audio files in the browser?

Comment 16

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 345602 [details] [diff] [review]
Yet another oversight :(

Giving this a review to move the bug forward, and the UI makes sense.  To follow up we need to make sure we are doing the same thing for video and audio.

Comment 17

[Nochum Sossonko [:Natch]]

Comment on attachment 347217 [details] [diff] [review]
Nits Addressed

annoyance fix.

Comment 18

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

Comment on attachment 347217 [details] [diff] [review]
Nits Addressed

a191=beltzner

Comment 19

[Nochum Sossonko [:Natch]]

filed bug 466909 as a follow-up for video/audio documents (that will have to wait though for bug 462892 and possibly others...).

Comment 20

[Nochum Sossonko [:Natch]]

Attachment: mq patch with name and message [check-in: comment 21 and 33]

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/54883d608a52

Comment 22

[Nochum Sossonko [:Natch]]

adding checkin-needed so that it doesn't get lost (for 1.9.1)

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1d09118998b9

Comment 24

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

Backed out of 1.9.1 due to Mac test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1228916771.1228921242.32424.gz

Comment 25

[Nochum Sossonko [:Natch]]

That's weird, it's passing on m-c on mac
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228949978.1228954124.20407.gz&fulltext=1

There is quite a bit of loading+load listeners and removers, but I figured that's safer than setTimeout. I guess this will just have to be bumped to 1.9.2, unless someone can figure this out.

Comment 26

[Nochum Sossonko [:Natch]]

Scratch that, comment 24 's log was from 10.5.2 while mine was from 9.2.2, still pretty weird though ;)

Comment 27

[Nochum Sossonko [:Natch]]

Are there any known weirdnesses with:

1) addEventListener,
2) the mochitest "fake" servers (e.g. localhost:8888). Maybe they don't get recognized as the same domain?
(or anything else)
I use them in the test and if it's problematic on 10.5.2 that may be the cause of the failure. I'm pretty sure the problem is with the test as the other tests were passing. Also this may be worth investigating as there is no mochitest tbox for 10.5.2 on m-c AFAICT, so it may, hypothetically, be failing there too?

Comment 28

[Dão Gottwald [:dao]]

See also bug 463923 comment 15. Might be some weirdness on that box.

Comment 29

[Jeff Walden [:Waldo]]

The proxy hack for the servers isn't going to be at fault here, or if it did we'd have huge problems with corporate areas that use proxy autoconfig.  (Not to mention it's exercised pretty heavily via the Mochitest harness anyway.)

Comment 30

[Nochum Sossonko [:Natch]]

(In reply to comment #29)
> The proxy hack for the servers isn't going to be at fault here, or if it did
> we'd have huge problems with corporate areas that use proxy autoconfig.  (Not

Not really, see below.

> to mention it's exercised pretty heavily via the Mochitest harness anyway.)

The problem probably wouldn't manifest itself in a very open manner, the proxy works perfectly, the question is does the browser see localhost:8888 as the same domain (for site specific prefs) when loaded a second time (this is specifically what is tested in this case). A good test for this would be to scrap the fake proxy and use chrome urls instead and then see if it passes, otherwise this doesn't really make sense, as it's passing on every other machine and all the tests on this machine are passing, besides the one that relies on the aforementioned.

Comment 31

[Jeff Walden [:Waldo]]

Write the test again, add dumps of interesting information and the JS stack, see what's being called wrongly (and conceivably from where).  I assert the proxying stuff is not at fault here, although what is, I don't know.

Comment 32

[Nochum Sossonko [:Natch]]

Attachment: Mochitest Debug

This applies on top of the previous patch (iow on m-c tip). Not sure if this is what you wanted, but if anyone's got a Mac (with 10.5.2) and can test this, then paste the debug information here it'd be great. ;)

Hope this helps.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b89527e65352

Comment 34

[Nochum Sossonko [:Natch]]

(In reply to comment #33)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b89527e65352

Ummm... this caused failures on the mac box last time, anything change since then?

Comment 35

[Dão Gottwald [:dao]]

Last time there was something wrong with moz2-darwin9-slave05.

Comment 36

[Nochum Sossonko [:Natch]]

Attachment: Future proof the test... [for check-in]

Just to be sure other tests don't fail unexpectedly, reset the zoom level to default.

Comment 37

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/df612b3cc3d8
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/067649fefb8d

Comment 38

[Aakash Desai [:aakashd]]

verified FIXED on builds: 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031307

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031352

Comment 39

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.