Closed Bug 376997 Opened 17 years ago Closed 13 years ago

Render standalone images against a dark gray background

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla11

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(4 files, 13 obsolete files)

Carlo's Patch v5 (rebased)
20.75 KB, patch
Details | Diff | Splinter Review
Patch with test fixes
85.69 KB, patch
bzbarsky
: review+
dholbert
: feedback+
Details | Diff | Splinter Review
Android bustage fix
1.99 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
background checkered
5.40 KB, image/png
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

It would be great if "direct" images and videos could be rendered against a neutral (gray?) background (instead of the white one used now) and maybe centered in the window.

Reproducible: Always

Steps to Reproduce:
1.Navigate directly to an image or video

Actual Results:  
Image is rendered against a white background in the top-left corner of the window.

Expected Results:  
Image should be rendered against a neutral background, centered in the window.
Confirming as a valid RFE since I don't see any obvious dupes. This probably isn't the right component, but I'm not really sure what is...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: Images/videos shall be rendered against a neutral background → Images/videos should be rendered against a neutral background (gray?)
Version: unspecified → Trunk
"direct videos" are handled by plugins, at least until we have in-browser video support, so there's not much we can do about that. "Direct images" can be modified by changing style rules in nsImageDocument.
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
I finally managed to implement this as an extension. Actually, I even gave the possibility to zoom and drag around images (even if I had to insert a few hacks to workaround browser.enable_automatic_image_resizing).
It's currently available within the sandbox on AMO https://addons.mozilla.org/en-US/firefox/addon/3683, or at http://cafxx.strayorange.com/ImageTweak
I hope this may help in case someone is willing to contribute some code for this RFE.
Tested on linux-x64.
It may make more sense to set a class on all nsImageDocument so that it can be styled via the global stylesheet.
This patch changes the appearance of nsMediaDocument so that
- the background is dark (#333)
- the image/video/plugin is centered in the window
Tested on linux-x64
Attachment #529675 - Attachment is obsolete: true
Ok, so... any comments on the patch?
Comment on attachment 529727 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

You should request feedback/reviews using flags on the patch, preferably from specific people. This and other pages on MDC may be useful: https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch
Attachment #529727 - Flags: feedback?
Comment on attachment 529727 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

Not sure who's familiar with this code, picking someone so this gets on someones radar.

Will also probably want some input from UX.
Attachment #529727 - Flags: ui-review?(faaborg)
Attachment #529727 - Flags: feedback?(jst)
Attachment #529727 - Flags: feedback?
As a side note, much of the code in nsImageDocument (at least the part that deals with resizing) could probably be replaced by a few of lines of CSS.
Just so that I know it... how often is considered good practice to harass reviewers to get them to look at my patch?
Maybe after 2-3 weeks with no activity.
Comment on attachment 529727 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

Looks good to me, requesting review from bz since this triggers a stylesheet load...
Attachment #529727 - Flags: review?(bzbarsky)
Attachment #529727 - Flags: feedback?(jst)
Attachment #529727 - Flags: feedback+
Comment on attachment 529727 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

The C++ part is OK.  The CSS doesn't work correctly in combination with the existing image resizing code and needs UI-review for the new behavior before I review the details of how the behavior is achieved.
Attachment #529727 - Flags: review?(bzbarsky) → review-
And in particular, please test the behavior of large images with "browser.enable_automatic_image_resizing" set to both true and false....
Fixed and tested wrt browser.enable_automatic_image_resizing and browser.enable_click_image_resizing. Zooming should now behave as it did before.
nsImageDocument will probably need some cleanup afterwards (a few things could be done way more simply). nsVideoDocument still won't allow fitting the video to the window if the video is bigger than the window.
Attachment #529727 - Attachment is obsolete: true
Attachment #533611 - Flags: ui-review?(faaborg)
Attachment #533611 - Flags: review?(bzbarsky)
Attachment #529727 - Flags: ui-review?(faaborg)
So with that patch, if "browser.enable_automatic_image_resizing" is false, is an image larger than the viewport shown at its intrinsic size by default?  Given the top/right/bottom/left styles, I wouldn't think so....
Yes it is shown at its intrinsic size, with scrollbars and all.
Oh, I guess because the rules for abs pos replaced elements are just weird like that, ok.

I'd still like ui-review on the general approach before reviewing the code details...
Whoops I just noticed I accidentaly deleted a CSS declaration that was needed for videos... I'll post it ASAP, along the change to fit videos to the window if bigger than the window.
Latest version with the changes mentioned in my previous comment
Attachment #533611 - Attachment is obsolete: true
Attachment #533641 - Flags: ui-review?(faaborg)
Attachment #533641 - Flags: review?(bzbarsky)
Attachment #533611 - Flags: ui-review?(faaborg)
Attachment #533611 - Flags: review?(bzbarsky)
Blocks: 655594
Comment on attachment 533641 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

Please request code review once the UI behavior has been cleared....
Attachment #533641 - Flags: review?(bzbarsky)
Comment on attachment 533641 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

I think this approach works well for video, which is meant to be immersive, and sort of viewed in a theater environment.  But I'm not as sure about images.  Some of them are fantastic photography which you want to view in that theater environment.  But some are sections of web pages that have an alpha channel and the user's goal isn't to view them as art much as to extract them from the page.

So let's center both, and keep a white background for images for now, and a dark background for video.  I'll discuss the dark background on images with the rest of the team to see what they think as well.

A few comments for possible follow up bugs:

-When the image is larger than the window size, it will push to the far left corner before adding a horizontal scrollbar, right?

-We might want to consider platform specific themeing, for instance on OS X it seems like the background should be black (quicktime title bar, etc.)

-I'm very interested in us getting zoom in and out animations for images in a follow up bug
Attachment #533641 - Flags: ui-review?(faaborg) → ui-review+
cc'ing fellow UX people for additional thoughts
Alex, I will rework the patch as soon as I have some free time from my thesis, this way others can provide additional thoughts.
Speaking of which, maybe you can try https://addons.mozilla.org/en-US/firefox/addon/imagetweak/ and see if something else may be ported over to the patch.
:faaborg or other UX people: any additional thoughts on this? Should I simply rework the patch with what's already been suggested?
(In reply to comment #23)
Finally over with my MEng, yay!

> I think this approach works well for video, which is meant to be immersive,
> and sort of viewed in a theater environment.  But I'm not as sure about
> images.  Some of them are fantastic photography which you want to view in
> that theater environment.  But some are sections of web pages that have an
> alpha channel and the user's goal isn't to view them as art much as to
> extract them from the page.
If there's an alpha channel, isn't a white background as arbitrary as any other color? And, besides, isn't the "photography" use-case (where the white background is rather detrimental) even more important?

> A few comments for possible follow up bugs:
> 
> -When the image is larger than the window size, it will push to the far left
> corner before adding a horizontal scrollbar, right?
If automatic_image_resizing is false and the image is larger than the window, the image is initially placed at the top-left corner of the window (as usual).
When zooming in via mouse-click, the click location is kept fixed.
Sorry for the delay on providing feedback.  Can you work with frank yan to get this landing on the UX branch so that we can test it out and discuss?
Not sure what that's supposed to mean (see that lovely green badge on comment #21?) but I can try. Should I rebase and export the patch?
Alex, does the patch need reviews before landing on the UX branch, or does the UX branch not feed directly into m-c?
My understanding is that we review after landing on UX branch and before central (it doesn't feed directly), but I could be wrong.  Frank?
(In reply to comment #31)
> My understanding is that we review after landing on UX branch and before
> central (it doesn't feed directly), but I could be wrong.  Frank?

UX Branch wikipage: https://wiki.mozilla.org/UX_Branch
(In reply to comment #31)
> My understanding is that we review after landing on UX branch and before
> central (it doesn't feed directly), but I could be wrong.  Frank?

Yep. Still, make sure the patch compiles and doesn't break basic browsing functionality.
Hey Carlo, thanks for volunteering to step forward and work on this :)

I'll push your changes to our UX build tonight so they show up in tomorrow's build. From there we can make sure that it "feels right". If all goes well then hopefully we can get this landed in mozilla-central soon.
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attached patch Patch for bug 376997 (obsolete) — — Splinter Review 
I've unbitrotted the patch and landed it on the UX branch.

https://hg.mozilla.org/projects/ux/rev/b69bcb41ac36
Attachment #533641 - Attachment is obsolete: true
Attachment #551999 - Flags: ui-review?(faaborg)
Why isn't this assigned to Carlo?
(In reply to Dão Gottwald [:dao] from comment #37)
> Why isn't this assigned to Carlo?

Good catch, I have assigned it to Carlo.
Assignee: jwein → bugzilla
UX win32 hourly build http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ux-win32/1312946625/

Video controls is missing. I get following message:

Error: this.Utils is undefined
Source File: chrome://global/content/bindings/videocontrols.xml Line: 931
Error: this.Utils is undefined
Source File: chrome://global/content/bindings/videocontrols.xml Line: 927
Error: this.Utils is undefined
Source File: chrome://global/content/bindings/videocontrols.xml Line: 234
Error: Permission denied for <URL> to get property Object.dynamicControls
Source File: chrome://global/content/bindings/videocontrols.xml Line: 348
(In reply to ek from comment #39)
> UX win32 hourly build
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ux-win32/
> 1312946625/
> 
> Video controls is missing. I get following message:
> 
> Error: this.Utils is undefined
> Source File: chrome://global/content/bindings/videocontrols.xml Line: 931
> Error: this.Utils is undefined
> Source File: chrome://global/content/bindings/videocontrols.xml Line: 927
> Error: this.Utils is undefined
> Source File: chrome://global/content/bindings/videocontrols.xml Line: 234
> Error: Permission denied for <URL> to get property Object.dynamicControls
> Source File: chrome://global/content/bindings/videocontrols.xml Line: 348

I am not able to reproduce the issue you are seeing. Further, I don't see the changes from this patch in the build you are referencing.

These are the steps that I performed to try to reproduce.
1) Downloaded http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ux-win32/1312946625/firefox-8.0a1.en-US.win32.zip
2) Extracted the zip and ran "firefox.exe -P clean -no-remote", where clean = new profile.
3) Visited http://www.webmfiles.org/wp-content/uploads/video/big-buck-bunny_trailer.webm and http://people.opera.com/howcome/2010/video/norway/00313-n.webm

Neither video showed a grey background, nor had the video centered. Further, the controls appeared correctly. Can you please provide some steps to reproduce the issue that you are seeing?
Hmm, I'm not able to reproduce this with clean profile. I've found that the culprit is HTTPS-Everywhere 1.0.0  extension and this problem is not occur with previous UX hourly build.

STR
1. Do step 1&2 on comment 40
2. installed http://www.eff.org/files/https-everywhere-1.0.0.xpi
3. visited http://www.webmfiles.org/wp-content/uploads/video/big-buck-bunny_trailer.webm

Thank you, Jared Wein
(In reply to ek from comment #41)
> Hmm, I'm not able to reproduce this with clean profile. I've found that the
> culprit is HTTPS-Everywhere 1.0.0  extension and this problem is not occur
> with previous UX hourly build.

Great find ek. I am curious if it is because this change introduced an external stylesheet and HTTPS-Everywhere is not happy with it.

There was also a problem with the nightly build that is unrelated to HTTPS-Everywhere. I don't think the nsMediaDocument.css file got packaged up in the installer. I'm working on trying to narrow down the cause and will update when I have more information.
(In reply to Jared Wein [:jwein] from comment #36)
> I've unbitrotted the patch and landed it on the UX branch.
> 
> https://hg.mozilla.org/projects/ux/rev/b69bcb41ac36

Carlo, there were some test failures on the UX branch because of this patch, so you'll also want to look into fixing those before requesting code review. You can see the failures here: http://tbpl.mozilla.org/?tree=UX&rev=0dd43b629ef1
ui-review is just waiting on this working on ux branch so we can try it out.
Comment on attachment 551999 [details] [diff] [review]
Patch for bug 376997

Removing this from pending review since there is more work that needs to be done to update tests.
Attachment #551999 - Flags: ui-review?(faaborg)
No longer blocks: 472942
Just a quick note to say that I plan to keep working on the patch as soon as I will be back from my vacations... I was also contacted by another person that is willing to help fix this.
browser/installer/package-manifest.in will likely need to be updated to include a reference to nsMediaDocument.css
Carlo: Has there been any more progress on this bug?
Unfortunately none, but I'd like to continue from where I left off. I just need to learn a little about the test infrastructure because I never used it.
I'm running a test right now on a clean copy of the trunk, I'll try to unbitrot the patch soon and see which tests are failing (unfortunately the link in comment #43 doesn't show the failures anymore).
Attached patch Second unbitrotted patch (obsolete) — — Splinter Review 
Brought up-to-date to today's trunk. Obsoletes Jared's patch (even if bugzilla doesn't allow me to mark it as such).
Attachment #551999 - Attachment is obsolete: true
Sorry, I forgot to commit a couple of changes. This one should be the correct one.
Attachment #560816 - Attachment is obsolete: true
Some further cleanups
Attachment #560817 - Attachment is obsolete: true
Ok... so, the patch works correctly locally (ubuntu 11.04 x64) but causes a helluva fails in the test-suite (this is mainly due to the fact that all tests expect the image in the top-left corner with a standard margin; this isn't the case with the proposed patch). Jared Wein suggested, after talking with Dolske, to have a flag disable the patch when running the test-suite (the other options being ripping out all failing tests or rewriting them).
The problem with the flag approach is that I don't really know enough about the test-suite to attempt such a thing... any suggestions?
It looks like we will need to add a pref to browser/app/profile/firefox.js and mobile/app/mobile.js to declare the new pref and the default value. Then in runreftest.py we will want to toggle the pref within createReftestProfile
We should just fix the tests.  Testing something other than what we ship to users is pointless.
I thought so. I'll try to fix or rip out as required. Can anybody just tell me which test-suite contained the failing tests?

p.s. I'm assuming that the new UX is deemed correct because I'd rather avoid fixing hundreds of tests *twice*.
Yeah, getting sign-off on the UX first is definitely imperative.

At that point, you can probably ask for review on the code in parallel with fixing up the tests....

I do wonder which tests depend on the behavior here in a non-transparent way (e.g. reftests should change the test and reference the same way, so should just work).
And in particular, if we're talking hundreds of tests, then maybe adjusting the tests is not the right thing to do, depending on what they're testing.  If you tell me which tests were failing, I can tell you which testsuite they're in...
I'm probably a bit out of the loop here, but if the current code on the UX branch is the same as is being discussed here, OGG videos seem to be lacking video controls altogether, e.g:

http://videos-cdn.mozilla.net/serv/air_mozilla/07272011_brownbag.ogg

Looks good apart from that issue, which may not be related to your changes here at all. :)
The video style was changed in bug 472942. I also see a lack of controls. *sigh*

I'll file a followup.
Carlo: Can you please upload your latest patch and I can push it to the Try server for you so we can figure out which tests are failing?
Darn, the previous patch has rotted (someone's been doing part of the same work my patch is doing; at least for videos - BTW wasn't there a style limit on row length?).

I just finished merging my patch and the current trunk; it's compiling right now. I'll post it ASAP.
Attached patch Patch v3 (obsolete) — — Splinter Review 
Unbitrotted patch. In the current trunk VideoDocument has a background pattern; I applied it to ImageDocument as well (for consitency, can obviously be changed; mind that the pattern it's barely noticeable).

Compiles and works as expected locally.
Attachment #560997 - Attachment is obsolete: true
Comment on attachment 563412 [details] [diff] [review]
Patch v3

>--- /dev/null
>+++ b/layout/style/MediaDocument.css

>+  background:url(chrome://global/skin/icons/tabprompts-bgtexture.png) #333;

Don't reference this image here. Themes have consistent URIs for stylesheets, but not for images. So this image won't necessarily exist. (And even when it exists, you're obviously misusing it.)

>+  width:100%;
>+  height:100%;
>+  margin:0;
>+  padding:0;

nit: always put a space after the colon
(In reply to Dão Gottwald [:dao] from comment #64)
> Comment on attachment 563412 [details] [diff] [review] [diff] [details] [review]
> Patch v3
> 
> >--- /dev/null
> >+++ b/layout/style/MediaDocument.css
> 
> >+  background:url(chrome://global/skin/icons/tabprompts-bgtexture.png) #333;
> 
> Don't reference this image here. Themes have consistent URIs for
> stylesheets, but not for images. So this image won't necessarily exist. (And
> even when it exists, you're obviously misusing it.)

That style declaration comes straight from the current trunk version of VideoDocument.cpp; I merely moved it to the CSS file. What should I do, drop it?

> 
> >+  width:100%;
> >+  height:100%;
> >+  margin:0;
> >+  padding:0;
> 
> nit: always put a space after the colon

will do
(In reply to Carlo Alberto Ferraris from comment #65)
> That style declaration comes straight from the current trunk version of
> VideoDocument.cpp; I merely moved it to the CSS file.

I know, it was already wrong there. That code was temporary, though.

> What should I do, drop it?

Maybe you can add an image to layout/style/? Or use a data: URI?
Attached patch Patch v4 (obsolete) — — Splinter Review 
whoops, forgot the quotes around the data: uri
Attachment #563412 - Attachment is obsolete: true
Comment on attachment 563450 [details] [diff] [review]
Patch v4

Review of attachment 563450 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/MediaDocument.css
@@ +38,5 @@
> +  and that this child is the image/video/plugin the MediaDocument was 
> +  istantiated for.
> +  TODO: Much of the machinery in MediaDocument and especially in ImageDocument
> +        could be get ridden of and replaced by some appropriately crafted CSS.
> +        This is tracked in bug XXXXXX.

Is there other machinery to edit, or should these XXXXXX be replaced with 376997? If it is a different bug, please file it so we can keep track of it.

@@ +89,5 @@
> +}
> +
> +/* 
> +  Styles originally defined in VideoDocument.cpp 
> +  FIXME: should they be applied to images as well?

The textured background and box-shadow should only be applied to videos.
Attached patch Patch v5 (obsolete) — — Splinter Review 
Removed texture when viewing images (not sure if this is a great idea, but still)

The bug marked with XXXXXX is yet unfiled: basically it's about the fact that most of the code in ImageDocument.cpp could be replaced by few lines of CSS.
Attachment #563450 - Attachment is obsolete: true
I have pushed this patch to the try server so we can see which tests are broken:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ae9ff7fd97da
(In reply to Jared Wein [:jwein] from comment #70)
> I have pushed this patch to the try server so we can see which tests are
> broken:
> https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ae9ff7fd97da

Right, so...

* there are 8 mochitest-1 failing (I can fix those manually)
* there's (at least) a crash in mochitests-3 (I think I need help troubleshooting this one)
* there's a lot of fails in reftests involving png image comparison (might be due to the fact that the background has changed from white to #333; how should I proceed with these?)
* Android builds are failing (looks unrelated)
* there are leaks on Mac (might be unrelated as well)
* there are leaks on (Lin/Mac) debug x64 (could be related - even if I have no clue how)
(In reply to Carlo Alberto Ferraris from comment #71)
> * there are 8 mochitest-1 failing (I can fix those manually)
> * there's (at least) a crash in mochitests-3 (I think I need help
> troubleshooting this one)

Do you have a debug build? Can you run the failing test locally? We'll figure this out.

> * there's a lot of fails in reftests involving png image comparison (might
> be due to the fact that the background has changed from white to #333; how
> should I proceed with these?)

It looks like these are just testing png decoding. We should be able to change them from comparing a png file directly to a suitable html file containing the png as an img.

> * Android builds are failing (looks unrelated)

Probably unrelated.

> * there are leaks on Mac (might be unrelated as well)
> * there are leaks on (Lin/Mac) debug x64 (could be related - even if I have
> no clue how)

These are probably intermittent random.
(In reply to Timothy Nikkel (:tn) from comment #72)
> (In reply to Carlo Alberto Ferraris from comment #71)
> > * there are 8 mochitest-1 failing (I can fix those manually)
> > * there's (at least) a crash in mochitests-3 (I think I need help
> > troubleshooting this one)
> 
> Do you have a debug build? Can you run the failing test locally? We'll
> figure this out.

Here is a link to the builds from the try server run:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-ae9ff7fd97da/

Thanks for your help Timothy :)
So in the crash we are in nsImageLoadingContent::OnStartContainer and looping over the observers and we have observer having mObserver as the pointer 0x100000001, which is non-null (we null check it) but not valid. I wonder how that happens.
Just wanted to record somewhere what the crashing test is since the try server results aren't available anymore. dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html
Rebased the patch. I'll push to try server shortly.
Attachment #563520 - Attachment is obsolete: true
Try run for 78ff2a1ef6f4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=78ff2a1ef6f4
Results (out of 131 total builds):
    success: 93
    warnings: 38
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-78ff2a1ef6f4
The image inserted as data stream in the MediaDocument.css is probably build using "Adobe%20Photoshop%20CS5%20Macintosh". This can be seen in the datastream itself.
A good idea would be to pngcrush/gauntlet/optim/etc to remove the unneccessary Adobe/Mac resource info from the image file before converting it to a css data stream.
The image cleaned with pnggauntlet and converted to base64 data stream is 662 bytes opposed to the original 3382:

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABMAAAATCAMAAABFjsb+AAAAhFBMVEXl5eXN1+XI0+PE0N/G0eHW2+XDzt7d4OW9yNi8x9fK1eWdqLi0wNCtuMjS2eW4w9Ortsazvs5/ipqRnKyirb3h4uWPmqq/ytrb3+XBzNzY3eWyvc3AzNuptMSmscHj4+WSna3f4eWUn6+6xdWhrLyfqrrQ2OW5xNR+iZmNmaicp7dteIiLLzq/AAAALHRSTlMaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGhoaGtsI/XQAAADdSURBVHheXZDlagRhEARrPpV131OXyPu/X7glJJf8GxqGri6UVqJ0SsaktJxK0G8zzQj9FboNvnWgZhPEum1Zbp2VYGZ1QQjvUGSQFTBGpEU1wqqyPgRvq9VBGgV6tFQfUNeQVaw7DQlXWGJ+GvKILRxpT2KbefId7HJ8NiFHTF8S6hN5w1AHyt6AuT6zgeaZxSX793ubkPNPx/DSoTtLlf1lUZtv5vhkRjYKhDAelm23323eRFm7qSwnt5ZovHqg25mmg75/ceUWp2KMLE6duiNcWtgf4byH9sH98wtOlQ5jxZmREQAAAABJRU5ErkJggg==
I have broken this bug up in to three parts.

The first part (bug 700854) is to add the ability to link to an external stylesheet. The second part (bug 700586) is to move the current styles to the new external stylesheets. This bug will then be about simply changing the styles to render images/videos on a neutral background as well as fix any tests that are reliant on the previous behavior.
Sorry, part 2 should have been linked to bug 700856.
Simplified patch based dependent upon bug 700856
No longer blocks: 693958
Attachment #573600 - Flags: feedback?(fryn)
Comment on attachment 573600 [details] [diff] [review]
Simplified patch based dependent upon bug 700856

It was very surprising to me that position: absolute; top/right/bottom/left: 0; centers the image on both axes, but I guess it works since the script explicitly sets the width and height attributes on the image. Pretty cool stuff. :D

We don't need the lines:
+  width: 100%;
+  height: 100%;
+  margin: 0;
+  padding: 0;
for body though, since absolute positioning won't take those into account.
Attachment #573600 - Flags: feedback?(fryn) → feedback+
By the way, I think the background color should be darker;
I recommend #222 instead of #333.
Jared discovered that we need to explicitly set margin to zero.

This is because the code that resizes the image when the viewport is resized takes the margin (and padding, border, etc.) of the body into account when sizing the image.

We could also change that resizing code, but it is easiest and least risky simply to set the margin.
(In reply to Carlo Alberto Ferraris from comment #27)
> If there's an alpha channel, isn't a white background as arbitrary as any
> other color? And, besides, isn't the "photography" use-case (where the white
> background is rather detrimental) even more important?
We don't select any arbitrary colors. It's a Platform Default background color, so users have chosen the color. Firefox even have a override settings in the app. (see Tools > Options > Content > Fonts & Colors > Colors)
Why do you think it's better to imposing your (or UX team's, or whoever other than user) taste than respecting user's choice?
(In reply to Masatoshi Kimura [:emk] from comment #87)
> (In reply to Carlo Alberto Ferraris from comment #27)
> > If there's an alpha channel, isn't a white background as arbitrary as any
> > other color? And, besides, isn't the "photography" use-case (where the white
> > background is rather detrimental) even more important?
> We don't select any arbitrary colors. It's a Platform Default background
> color, so users have chosen the color. Firefox even have a override settings
> in the app. (see Tools > Options > Content > Fonts & Colors > Colors)
> Why do you think it's better to imposing your (or UX team's, or whoever
> other than user) taste than respecting user's choice?

The setting Tools > Options > Content > Fonts & Colors > Colors appears to affect all pages on the internet, unless the author of that page has explicitly set a background. I don't think we should be concerned about that setting for when directly-viewed images are displayed. This would simply be another case of an explicitly-defined background.
(In reply to Boris Zbarsky (:bz) from comment #58)
> And in particular, if we're talking hundreds of tests, then maybe adjusting
> the tests is not the right thing to do, depending on what they're testing. 
> If you tell me which tests were failing, I can tell you which testsuite
> they're in...

When running just the reftests under /image/test/reftest, I have found the following test suites to have errors.

pngsuite-basic-n/*
pngsuite-basic-i/*
pngsuite-basic-ancillary/*
pngsuite-chunkorder/*
pngsuite-filtering/*
pngsuite-gamma/*
pngsuite-oddsizes/*
pngsuite-palettes/*
pngsuite-zlib/*
gif/test_bug641198.html
gif/webcam.html
icon/win/bug415761.sjs
encoders-lossless/*

I will try to get these tests fixed before looking for other test suites with breakage.

These tests fail because the reference HTML file does not account for the image being centered in the screen and the change of the background color.

I think we can fix the pngsuite tests by replacing the *.png with a simple HTML file that has the png as the src of an image, like so:

<html><head><title>ccwn2c08.html</title>
<body><img src="ccwn2c08.png" width="32" height="32"/></body>
</html>

Unfortunately, I couldn't find a tool that was used to generate these tests. I hope that if we decide to follow this route that they won't have to be created by hand, but if that is what is required then that will be fine too.
(In reply to Jared Wein [:jwein and :jaws] from comment #89)
> I think we can fix the pngsuite tests by replacing the *.png with a simple
> HTML file that has the png as the src of an image, like so:
> 
> <html><head><title>ccwn2c08.html</title>
> <body><img src="ccwn2c08.png" width="32" height="32"/></body>
> </html>

I spoke with dholbert offline and he recommended that I add a stylesheet reference to the preexisting HTML files instead of adding new HTML files. The new stylesheet will match the ImageDocument.css file closely (using |table| instead of |img|).
Updated the title of the bug since bug 472942 already updated video documents.
Summary: Images/videos should be rendered against a neutral background (gray?) → Images should be rendered against a neutral background (gray?)
(In reply to Jared Wein [:jwein and :jaws] from comment #90)
> I spoke with dholbert offline and he recommended that I add a stylesheet
> reference to the preexisting HTML files instead of adding new HTML files.
> The new stylesheet will match the ImageDocument.css file closely (using
> |table| instead of |img|).

At least, that seems like the simplest tweak (just adding a single new stylesheet based on the existing ImageDocument.css, and add a single line to each reference case to reference it).

The replace-testcases-with-basic-HTML-file-using-<img> strategy could be useful too, though, to test the "as basic as possible" rendering.  Either (or both? :) ) would be reasonable ways forward.
Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=8295fc2c875a

Requesting feedback? from dholbert on the test changes. Some of the tests run an HTTP server that does not allow folder traversal to read the CSS file located in the higher-up folder, so in these cases I copied the CSS file to the specific directories.
Attachment #573600 - Attachment is obsolete: true
Attachment #580202 - Flags: feedback?(dholbert+bmo)
Attachment #580202 - Flags: review?(bzbarsky)
Comment on attachment 580202 [details] [diff] [review]
Patch with test fixes

(sorry for the delay on this!)

Wow, there are a bunch of different places that need (their own variant of) the CSS file, I guess.  Oh well, I guess that level of test-rework is to be expected when we're changing how a whole class of content is presented. :)

Just one feedback nit:
>+++ b/image/test/reftest/encoders-lossless/encoder.html
>@@ -1,11 +1,12 @@
> <html class="reftest-wait">
> <head>
>-<title>Image reftest wrapper</title>
>+  <title>Image reftest wrapper</title>
>+  <link rel="stylesheet" href="ImageDocument.css" />

 - In HTML (as opposed to XHTML), the trailing "/" in <link ... /> is unnecessary and technically invalid, I'm pretty sure, though our parser is forgiving & handles it (by just ignoring the slash).  That may be might be worth fixing before checking this in... I think you could fix most or all of them in one fell swoop by opening the patch file itself in an editor and doing a search-and-replace operation.

Other than that, looks sensible. feedback+! :)
Attachment #580202 - Flags: feedback?(dholbert) → feedback+
Comment on attachment 580202 [details] [diff] [review]
Patch with test fixes

r=me.  Thank you for your patience here!

Not sure what the User line should really look like here....  Adjust as needed?
Attachment #580202 - Flags: review?(bzbarsky) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/97b8cff2764f

Thanks for your contributions Carlo! :)
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/97b8cff2764f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(In reply to Carlo Alberto Ferraris from comment #0)
> Expected Results:  
> Image should be [...] centered in the window.

Is it expected to keep the image centered when zooming in?
Attached patch Android bustage fix (obsolete) — — Splinter Review 
This caused image reftests on Android to fail because TopLevelImageDocument.css is not included on Android.  I think this should fix it.
Attachment #582363 - Flags: review?(mark.finkle)
Updated to apply to both XUL and native Android fennec.
Attachment #582363 - Attachment is obsolete: true
Attachment #582368 - Flags: review?(mark.finkle)
Attachment #582363 - Flags: review?(mark.finkle)
Backed out on m-c until the Android failures are fixed:
https://hg.mozilla.org/mozilla-central/rev/cbb0233c7ba8

Pushed to Try with the Android fix; this can re-land with that patch once it's green on Try (and the Android fix is reviewed).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
(By the way, the Try push has a misleading commit message; despite appearances it does not actually include the backout.)
Whiteboard: [fixed-in-fx-team]
The fix looks green on Try (aside from a Mac mochitest failure that I don't think can possibly be related to my mobile-only patch).
What about comment 80?
The data url in MediaDocument.css is ridiculous large.
The containd png can be crushed to a quarter of its size,
but probably a -moz-gradient-image thing could be used instead of this image data url construct.
Attachment #582368 - Flags: review?(mark.finkle) → review+
Re-landed with mobile fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a47e6a308fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0b56de8e9e

(In reply to Alfred Kayser from comment #105)
> What about comment 80?
> The data url in MediaDocument.css is ridiculous large.
> The containd png can be crushed to a quarter of its size,
> but probably a -moz-gradient-image thing could be used instead of this image
> data url construct.

Could you file a followup bug (with a patch, if you'd like!) to fix this?
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Here is most suitable color - #eceeef
And maybe add this code too (or similar)?

html:not([xmlns]):not([dir]):not([lang]) head + body > img:first-child:last-child:only-of-type:hover {
background: url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAUCAAAAACo4kLRAAAAG0lEQVR42mO4AwLvQADBookggo9g0UJw+PkIAJdvZeAMVgxlAAAAAElFTkSuQmCC") !important;
}
>>Here is most suitable color - #eceeef

Or color should depend on the setting:

user_pref("browser.display.background_color", "#eceeef");
A button to toggle (and remember) between dark and light background would be perfect. Many images is not suitable for dark background, specially with transparency.
I think that there should be a white layer behind the image so that images with transparency are displayed on a white background, rather than a dark gray one. This issue can be seen here, for example: http://ompldr.org/vYnRxcw
This bug is closed.

Please open a new bug if you find any issue (Logan, I think you found one), and use the mailing-list to discuss.
Or use same technique of this addon https://addons.mozilla.org/en-US/firefox/addon/dominant-color/ to change background color.
This fix leads to issues on one hand with images having alpha-transparency and
for contrast-impaired people on the other. I think the solution implemented
is suboptimal, and raises more issues than the previous behaviour. The
accessibility part is big enough to trigger a revamp IMHO.
Blocks: 713383
This bug is not fixed as it does not achieve the prime goal - 'Images should be rendered against a neutral background'.

Neutral is a term not only applied to hue, but also to contrast/tonal value and #333333 or RGB 51,51,51 is obviously not a neutral tonal value by any stretch of the imagination. In fact, because perceived hue/contrast/tone changes according to the area covered by a colour, #333333 will appear black (#000) across such a large area, as a browser window. 

If the desire here is to use a neutral background, then you need to be looking at #7A7A7A (RGB 122,122,122) at the very darkest.

However, I should perhaps mention that there is a reason why real life artists do not use shades like this when framing their pictures and that reason is a simple one - it looks rubbish and does not enhance either the colour balance or tonal values of the picture.

I would suggest something light with a tint (just grey is very dull) i.e. something like - #E8EAF4
Nope. We're done here. Open new bugs if you have issues.
Status: RESOLVED → VERIFIED
(In reply to Justin Dolske [:Dolske] from comment #117)
> Open new bugs if you have issues.

Not my problem, not going to be me that users will be screaming at. You want to think that near black counts as a neutral background then you go right on with it. :)
Blocks: 713487
Frank, Justin wasn't saying we shouldn't change the color.  He was saying that any followup work needs to happen in a separate bug so that things can be tracked properly in terms of releases and whatnot.
Boris, thanks for the clarification, it is appreciated.
(In reply to Boris Zbarsky (:bz) from comment #119)
> Frank, Justin wasn't saying we shouldn't change the color.  He was saying
> that any followup work needs to happen in a separate bug

(For reference -- see in particular bug 713555, where the background color may end up being chosen dynamically.)
Blocks: 713555
Blocks: 713904
Attached image background checkered — 
background should be in the checkered as it is for example in Gimp
.svg images not in center.
(In reply to fx4waldi from comment #123)
> background should be in the checkered as it is for example in Gimp

Please don't post requests for alternate behavior on this bug.  This bug is closed; further work should be tracked in followup bugs.

(In reply to Andrew from comment #124)
> .svg images not in center.

That's correct & expected.

SVG has its own expressive and complicated semantics for how it is sized and positioned in an arbitrary viewport like a browser window.  If we imposed an additional centering rule, we'd break that.

(Also, when viewed directly, .svg "images" really behave more like HTML documents than images -- e.g. they can run javascript, modify their size, pop up alert()s, etc.  So this bug doesn't apply to them just like it doesn't apply to HTML documents.)
(In reply to Boris Zbarsky (:bz) from comment #119)
> Frank, Justin wasn't saying we shouldn't change the color.  He was saying
> that any followup work needs to happen in a separate bug so that things can
> be tracked properly in terms of releases and whatnot.

I agree with Frank Lion on Comment 116 100%, Justin actually said this bug was "done" in my opinion half ***ly, because the color chosen isn't no way a "neutral background (gray?)", this bug was prematurely marked "VERIFIED FIXED" when the goal of this bug wasn't clearly accomplished. Don't say "file another bug# if you have problem with it", since that doesn't seem to have done anyone else any good, since the choose of using "adaptive color"(bug 713555 was marked RESOLVED WON'T FIX) or to make the background color actually configurable(bug 713904 was marked RESOLVED DUPLICATE).

Note: It's pretty sad that we has users are now forced to use either a extension or a userstyle(CSS code) to be able to view the majority images properly. The rushed completion of a filed bug has of this one, makes me wonder how many other rash decisions are not being overseen and analyzed to ensure the complete-fulness of a filed bug's goal are actually being met?
And firstly the color now used is not gray...
It's more like black, light black...We should use neutral gray like #999999 for example...
(In reply to Virtual_ManPL from comment #127)
> It's more like black, light black...We should use neutral gray like #999999
> for example...

File a bug.
(In reply to Dão Gottwald [:dao] from comment #128)
> (In reply to Virtual_ManPL from comment #127)
> > It's more like black, light black...We should use neutral gray like #999999
> > for example...
> 
> File a bug.

It had already been filed bug 376997, but apparently someone doesn't know the difference between "light black" or "neutral gray", any new bug# will mostly likely be marked a "Duplicate" or "Resolved Won't Fix" anyways. This bug should of been done wright in the first place, so there wouldn't be a need of filing a new bug.

New bugs filed:
bug 717226
bug 713230
Summary: Images should be rendered against a neutral background (gray?) → Render standalone images against a dark gray background
Depends on: 717226
Depends on: 717624
No longer depends on: 717624
Blocks: 718376
>This fix leads to issues on one hand with images having alpha-transparency and
>for contrast-impaired people on the other. I think the solution implemented
>is suboptimal, and raises more issues than the previous behaviour. The
>accessibility part is big enough to trigger a revamp IMHO.

Can you file a specific bug describing the "contrast-impaired people" + "accessibility part"? Perhaps mark it as a regression as well.
Blocks: 720286
Depends on: 720906
FWIW Adobe have also taken to using a similar grey for the background in Photoshop: http://youtu.be/pBIf9KljT68
I still don't understand how this bug was fixed. It seems to me the fix chose
the best way to maximize the number of people being unhappy with it. I still
don't understand why it was difficult to implement a hidden preference for
the #222 background color and create a style element on the fly with

  body { background-color: <that value> }

instead of a fixed hard-coded value. That way, Firefox could move to the
#222 background color for image documents and people unhappy with it - OR
THIRD-PARTY EMBEDDERS NEEDING ANOTHE BACKGROUND - could work around it using
the hidden preference.

Honestly, implementing that preference would not be a luxury here, and would
solve most of the accessibility issues related to the new behaviour.
(In reply to Daniel Glazman from comment #132)

Hi Daniel, thanks for your interest in this bug.

For matters of tracking bug fixes and their related discussion, we should move discussions of these sorts to a separate bug. Can you please take a look at the bugs that are already filed against this change (bug 713230, bug 713383, bug 713487, bug 720286, bug 472942, bug 655594, bug 713555, bug 713904, bug 718376)?

If you don't see one that is duplicating your request, then please file a new bug that is dependent on this bug.
No longer blocks: 713383
Depends on: 713383
I wholeheartedly concur that this IS NOT FIXED via a static, unchangeable embedded CSS stub. 

It should be a hidden preference at worst, if not a Visible Preference, probably under Preferences > Content > Colors
Depends on: 735641
This RFE is based on a very wrong assumption.  Stand-alone images do NOT appear against a white background.  They appear against a background using what the user has specified as a default background color.  You have imposed a background contrary to what users have set.
(In reply to David E. Ross from comment #135)
> You have imposed a background contrary to what users have set.

I also find this very irritating. Years of behavior down the tubes because all of a sudden Firefox thinks my preferences don't matter anymore.

(In reply to Daniel Holbert [:dholbert] from comment #125)
> Please don't post requests for alternate behavior on this bug.  This bug is
> closed; further work should be tracked in followup bugs.

Filed Bug 737320
"Nice fix." If it isn't broke, DO NOT fix it. 
Instead of provide configuration options.

If you're considering making background color configurable, please do the same for alignment.

You have single-handedly broken my workflows.

There are many technological and psychological cases why image needs to be on the left and on white background (to save time on simple but neccesary tasks and not to overstress brain). Please, try to understand how brain works. While "center stylish gray" has now other value than being beautiful (and even beauty, you know, is very arguable value).
I got this patch with an auto upgrade this morning. Now all the transparency diagrams I've created for my class are unreadable. When the same upgrade happens at my university, all of my students will be unable to view the transparencies.

The reason I used transparent diagrams and Firefox as the default browser was so that students with dyslexia could alter the background colour to reduce glare. This is a standard solution for dyslexia sufferers. Now all of the machines at my university will require an additional plug-in installed to make transparencies visible from Firefox.

This problem is going to hit every teacher and student in the world currently using Firefox to view transparencies.
For people trying to fix this change, there's an addon called Old Default Image Style. 

(Whatever happened to about:config?)
(In reply to candylandish from comment #139)
> (Whatever happened to about:config?)

Recap:
* Using an adaptive background color was rejected (bug 713555)
* Adding a new preference was rejected (bug 713230 comment 24)
* Undoing the regression by rendering the existing preference useful again was also rejected (bug 713904, bug 718376, bug 737320)
* Using a lighter shade of grey was rejected (bug 717226)
This is just horrible. You broke something that no one would ever imagine was possible to break. Changed something that browser users use so many times a day that it was like breathing, without ever even looking at a png with alpha channels or anything. SHAME ON YOU.
Depends on: 738948
Depends on: 739052
(In reply to Paul Rouget [:paul] from comment #113)
> Please open a new bug if you find any issue

(In reply to Justin Dolske [:Dolske] from comment #117)
> Nope. We're done here. Open new bugs if you have issues.

(In reply to Daniel Holbert [:dholbert] from comment #125)
> Please don't post requests for alternate behavior on this bug.  This bug is
> closed; further work should be tracked in followup bugs.

(In reply to Dão Gottwald [:dao] from comment #128)
> File a bug.

(In reply to Jared Wein [:jaws] from comment #133)
> If you don't see one that is duplicating your request, then please file a
> new bug that is dependent on this bug.

I really wonder why the people with [:usernames] insist that a new bug should be opened, while others keep saying that this bug is not actually fixed as they think it is. Can someone explain why opening a new bug is so important? Would the developers think of the problem differently if filed as a separate bug, and not otherwise?
(In reply to Seungbeom Kim from comment #142)
> Can someone explain why opening a new bug is so
> important?

Because the alternative (discussing everything & fixing fallout & getting everything perfect to everyone's satisfaction on the original bug) is untenable and produces unreadable, impossible-to-follow bug pages (which this page is well on its way, at 142 comments).  It also makes for difficult release-management, if a series of patches land over a long period of time all from the same bug. (and/or a bug is closed / reopened / closed / reopened repeatedly)

It's much saner to track individual issues (and their individual patches / small groups of patches) in their own self-contained  bugs. (which can be linked to their causes via the "blocks" / "depends on" fields)
I consider this "feature" of viewing standalone images centered and on a black background to be a bug. The image should be simply displayed (not centered) with the default background.

I specifically signed up to Bugzilla just to make this comment, because this "feature" is so annoying. I thought it was some website nonsense, until I discovered no... it's Firefox itself doing it.
This black background is not a feature, it's a bug indeed and should be fixed. I wish I could vote _against_ it (is there any corresponding bug, which is not closed yet?). :(
Customization of it would be a perfect solution.
unsubscribe.
People that was to change that background color can most probably do so by editing userChrome.css or userContent.css in their profile, whichever fits the bill.
Depends on: 740131
I consider implementation of the fix for this bug to be a serious regression. My eyes beg to be gouged out of their sockets every time I see a transparent png. I request the regression be fixed.

I'd like to vote against this bug.

I saw no community comment period for the non-changeable color choice.  To press us to use addons or scrips is outrageous.  At least add a about:config flag.

I've never been so bothered by a bug"fix" in my life.
Since I haven't seen anyone mention it, I'd like to point out that most animated gifs with transparency look very bad on a non-white background. Also, personally whenever I edit images for the web I edit them with the assumption that they will be on a white background, as all major browsers (Firefox, IE, Chrome, Safari, Opera) up until now have done so.

Perfect example right here on bugzilla: https://bugzilla.mozilla.org/extensions/BMO/web/images/mozchomp.gif
(Screenshots: http://i.imgur.com/WMfY9.png http://i.imgur.com/EoRXg.png)

Regardless of what the background color is by default, there absolutely without question should be a preference in the options to change both the background color and the centering of images.
(In reply to mixxster from comment #149)
> I'd like to vote against this bug.

bug 713230 looks like the right place to do that (can anyone suggest somewhere better?).
Newsgroups would be better than the bug because Bugzilla is not a discussion forum.
As mixxster says, this is a regression from the perspective of many (if not most) users, so a bug seems appropriate.

We're not asking for much - just an option to restore the original behaviour.  You could even hide it away in about:config.  I really can't understand the attitude from the FF devs on this one.
Besides, mixxster and MySh were specifically looking for somewhere to vote.  Are votes on bugs not welcome anymore?
(In reply to Mark Goodhand from comment #154)
> Besides, mixxster and MySh were specifically looking for somewhere to vote. 
> Are votes on bugs not welcome anymore?

From this page "Importance: 	with 16 votes (vote)"
Searching this page for "vote" would have been welcome before adding an offtopic comment.
Wow, what a lovely community we have here.

mixxster and MySh don't want to vote for this bug, they want to vote against it.  Or rather, they want to vote for a preference to restore the old behaviour.  The best way I could see to do that was to vote for bug 713230 (which is currently, inexplicably, WONTFIXed).

So I'd say that yours was the off-topic comment.
Voting against something never existed. 
If you want to discuss it, search for the bug report that requests it.
Nearly there ...

Yes, that's exactly what I suggested - vote for bug 713230.  It seems to be the one that all of the other complaints have been resolved as a duplicate of.  If there's a different bug we should be voting for, please let me know.
My vote is for bug 738948. Bug 713230 doesn't address the issue of images that are intended as a transparent overlay of a custom selected user background. Instead it requires changing only the background colour within the existing fixed and non configurable border colour. This will still obscure transparencies (diagrams or other illustrations) that are drawn to the edge of the image. For a large collection of such images, see the numerous chemical, mathematical, geometrical, statistical, and electrical diagrams used at Wikipedia.
I filed a separate Bug 743474 for the problem with transparent PNGs and GIFs. None of the other bugs on this topic seem to address this directly. Unlike the others, this is not a matter of a user preference. It is a matter of breaking many sites that provide bare links to transparent PNGs, of which Mr. Sinnet is right in saying that Wikipedia is a significant member. The problem is not the gray border, but the fact that the image itself has the wrong colored background. And I'll point out that this is something Boris himself thinks is something to do (which is mentioned in the bug report)

Also, I agree this bug is not fixed, as a dark gray background is not neutral. Neutral gray is either #808080 (gamma 2.2) or #939393 (gamma 1.8), not #333333.
Hi!
Here is the easy and fast solution (at least here on Win 7 and Firefox 11).
Spread the word on the other bug-entries about this mal-feature. 
The "fix" for the "fix", for the "won't fix", for the "isn't broken", for the "it's the new standard", for the "don't open a duplicate bug" and for the "open a duplicate bug"!

1. Download WinRar. (WinRar asked to update an archive, is a file was changed.)
2. ! Close All Firefox Instances.
3. Install and run WinRar.
4. Navigate to your Firefox installation folder (for example 'c:/programs/firefox browser' or 'c:/programs (x86)/firefox browser') in WinRar.
5. Find the file "omni.ja". This is an archive itself. Rightclick on it and select show file (in my German version of WinRar it's "Datei anzeigen").
6. Now WinRar shows the omni.ja archive: Select folder "res".
7. Scroll down to the file "TopLevelImageDocument.css". Doubleclick it to open it in an editor. (At least it should open it in an editor or ask in which program to open it.)
8. Comment out the block starting with "@media not print {" by adding /* in front and */ at the last } of that block. (Step 8 is the most difficult if you haven't ever done anything in the field of programming or website writing.)
9. Try to use "Save" in the editor. Now WinRar should show a message asking if it should update the archive. Select yes. (Maybe the WinRar message doesn't go into the front, so you have to select the WinRar-Icon in the Windows program bar to see the message.)
10. Close WinRar. 
You have done it. =)

I was a bit disappointed about the Addon that is on the popular list - Propably it's not possible for an addon to change something in the omni.ja file.

If you like this solution, give me a link to my (quite inactive) webpages: Tageskurier.de Recreation.de DieterHansen.de Stefanie.de

Have fun!
I knew i forgot something: Don't try to email me, please. Neither on the email I registered here, which is only a one time email-adress, nor at my websites, which have so much spam. ):

Cheers up!
Good workaround, Dieter.  It's unreasonable to have to hack this, but preferable to installing an Add-on, IMO.

Some points to note:

1) On OSX the file is at /Applications/Firefox.app/Contents/MacOS/omni.ja

2) According to the `file` command, this file is "Zip archive data, at least v2.0 to extract", so you can edit it by telling vim to treat .ja as zip, or by renaming to .zip

3) When performing such hacks, it's a good idea to take a backup of the original file :-)

4) This workaround might need to be updated when bug 713487 is fixed (but hopefully we'll have a proper fix by then).
(In reply to Dieter Hansen from comment #161)
I'm not really familiar with how the Firefox update process works but I suspect that hand-modifying omni.ja will cause it to fail spectacularly.
With bug 713487 landed, themers can now provide their own styling for image and video display. So, please stop any discussion here, this bug s closed.
In general, this is an improvement, but black-on-transparent images are now unviewable.  It would be best if the background color could be toggled, so images with transparency can still be viewed correctly.
One could use the something like the following to give transparent images a patterned background (like in image editors):
img {
    background-color: -moz-Dialog;
    color: -moz-DialogText;
    background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQAQMAAAAlPW0iAAAABlBMVEWgoKClpaUrSK7yAAAAAnRSTlNkFG3Yt+YAAAAPSURBVHheY2D4jxXhEgYAfr8P8SM7T18AAAAASUVORK5CYII=);
}

(I've tried -moz-linear-gradient instead of a image, but that is really slow)
(In reply to Carlo Alberto Ferraris from comment #164)
> I'm not really familiar with how the Firefox update process works but I
> suspect that hand-modifying omni.ja will cause it to fail spectacularly.

Not likely.  The worst that will happen is that the manually-modified omni.ja will get overwritten during the next update installation and one would need to repeat the manual edits.  The file resides in the program executables directory, not the user profile, so updates will overwrite it with impunity.  It would, however, be a good idea to back up omni.ja before attempting to edit it.
(In reply to Andrew Poth from comment #168)
> Not likely.  The worst that will happen is that the manually-modified
> omni.ja will get overwritten during the next update installation and one
> would need to repeat the manual edits.  The file resides in the program
> executables directory, not the user profile, so updates will overwrite it
> with impunity.  It would, however, be a good idea to back up omni.ja before
> attempting to edit it.

I just tried and it makes the incremenal update fail. You get the "incremental update has failed" message and then firefox has to re-download the full package. So no big deal. The reason for that is that incremental updates are made by diffing the file in the program directory and then patching them at update-time. If you modify one of them, the patching will fail, so Firefox has to re-download and overwrite everything.
Please don't discuss this here. This is not a discussion forum.
Now that themes can change the background styling, use a theme to do that.
Depends on: 754133
Blocks: 754539
Blocks: 788418
No longer blocks: 788418
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: