bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
General
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: smaug, Assigned: Natch)

Tracking

({polish, verified1.9.1})

Trunk
Firefox 3.6a1
polish, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy] [polish-interactive][polish-p4])

Attachments

(2 attachments, 7 obsolete attachments)

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?

Updated

11 years ago
Version: unspecified → Trunk
Keywords: polish
Whiteboard: [polish-easy] [polish-interactive]
(Assignee)

Comment 1

10 years ago
Created attachment 345428 [details] [diff] [review]
Like so?

This seems to work for me (if I understand this bug correctly).
Attachment #345428 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 3

10 years ago
Created attachment 345430 [details] [diff] [review]
Err... gonna have to go both ways
Attachment #345428 - Attachment is obsolete: true
Attachment #345430 - Flags: review?(gavin.sharp)
Attachment #345428 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

10 years ago
browser chrome test coming... I'll post it seperately.
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
(Assignee)

Comment 5

10 years ago
Created attachment 345602 [details] [diff] [review]
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).
Attachment #345430 - Attachment is obsolete: true
Attachment #345602 - Flags: review?(gavin.sharp)
Attachment #345430 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

10 years ago
Created attachment 345617 [details]
Here's what I've been doing so far.

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

Comment 7

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

Comment 8

10 years ago
Created attachment 345916 [details] [diff] [review]
Utter Stupidity :( ...

Ok here's the browser chrome test, forgot to addEventListener on the linkedBrowser... :(
Attachment #345617 - Attachment is obsolete: true
Attachment #345916 - Flags: review?(gavin.sharp)

Comment 9

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

Comment 10

10 years ago
(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 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.
Attachment #345916 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

10 years ago
Created attachment 347217 [details] [diff] [review]
Nits Addressed

All nits addressed, besides the one discussed on irc.
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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.
Attachment #345602 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Hardware: PC → All
(Assignee)

Comment 14

10 years ago
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?
Attachment #345602 - Flags: ui-review?(beltzner)
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 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.
Attachment #345602 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 17

10 years ago
Comment on attachment 347217 [details] [diff] [review]
Nits Addressed

annoyance fix.
Attachment #347217 - Flags: approval1.9.1?
Attachment #347217 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

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

Comment 20

10 years ago
Created attachment 350284 [details] [diff] [review]
mq patch with name and message [check-in: comment 21 and 33]
Attachment #345602 - Attachment is obsolete: true
Attachment #345916 - Attachment is obsolete: true
Attachment #347217 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #347217 - Attachment description: [for-checkin] Nits Addressed → Nits Addressed

Updated

10 years ago
Attachment #350284 - Attachment is patch: true
Attachment #350284 - Attachment mime type: application/octet-stream → text/plain
http://hg.mozilla.org/mozilla-central/rev/54883d608a52
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [polish-easy] [polish-interactive] → needs trunk baking [polish-easy] [polish-interactive]
Target Milestone: --- → Firefox 3.2a1

Updated

10 years ago
Flags: in-testsuite? → in-testsuite+
(Assignee)

Comment 22

10 years ago
adding checkin-needed so that it doesn't get lost (for 1.9.1)
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1d09118998b9
Keywords: checkin-needed → fixed1.9.1
Whiteboard: needs trunk baking [polish-easy] [polish-interactive] → [polish-easy] [polish-interactive]
(Assignee)

Comment 25

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

Comment 26

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

Comment 27

10 years ago
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?
See also bug 463923 comment 15. Might be some weirdness on that box.

Comment 29

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

Comment 30

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

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

Comment 32

10 years ago
Created attachment 352962 [details] [diff] [review]
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.
(Assignee)

Comment 34

10 years ago
(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?
Last time there was something wrong with moz2-darwin9-slave05.
(Assignee)

Comment 36

10 years ago
Created attachment 355073 [details] [diff] [review]
Future proof the test... [for check-in]

Just to be sure other tests don't fail unexpectedly, reset the zoom level to default.
Attachment #352962 - Attachment is obsolete: true
Attachment #355073 - Flags: review?(gavin.sharp)
Attachment #355073 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

10 years ago
Attachment #350284 - Attachment description: mq patch with name and message [for checkin] → mq patch with name and message [check-in: comment 21 and 33]
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Attachment #355073 - Attachment description: Future proof the test... → Future proof the test... [for check-in]
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.
Whiteboard: [polish-easy] [polish-interactive] → [polish-easy] [polish-interactive][polish-p4]
Depends on: 575830
No longer depends on: 575830
Blocks: 719271
You need to log in before you can comment on or make changes to this bug.