Last Comment Bug 376997 - Render standalone images against a dark gray background
: Render standalone images against a dark gray background
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement with 15 votes (vote)
: mozilla11
Assigned To: Carlo Alberto Ferraris
:
: Jet Villegas (:jet)
Mentors:
http://www.mozilla.com/img/home/main-...
Depends on: 738948 700854 700856 713383 717226 720906 735641 739052 740131 754133
Blocks: 754539 472942 655594 713230 713487 713555 713904 718376 720286 783874
  Show dependency treegraph
 
Reported: 2007-04-10 00:57 PDT by Carlo Alberto Ferraris
Modified: 2012-09-05 00:32 PDT (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change the background color of nsImageDocument to #333 (586 bytes, patch)
2011-05-03 04:26 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Dark background and central alignment in nsMediaDocument (1.76 KB, patch)
2011-05-03 08:33 PDT, Carlo Alberto Ferraris
bzbarsky: review-
jst: feedback+
Details | Diff | Splinter Review
Dark background and central alignment in nsMediaDocument (7.73 KB, patch)
2011-05-19 05:26 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Dark background and central alignment in nsMediaDocument (8.08 KB, patch)
2011-05-19 07:40 PDT, Carlo Alberto Ferraris
faaborg: ui‑review+
Details | Diff | Splinter Review
Patch for bug 376997 (10.98 KB, patch)
2011-08-09 20:33 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Second unbitrotted patch (10.00 KB, patch)
2011-09-18 13:55 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Second unbitrotted patch (with fixes) (9.90 KB, patch)
2011-09-18 14:06 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Second unbitrotted patch (with fixes and cleanups) (11.66 KB, patch)
2011-09-19 13:16 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Patch v3 (13.88 KB, patch)
2011-09-29 08:13 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Patch v4 (17.17 KB, patch)
2011-09-29 10:23 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Patch v5 (18.41 KB, patch)
2011-09-29 12:24 PDT, Carlo Alberto Ferraris
no flags Details | Diff | Splinter Review
Carlo's Patch v5 (rebased) (20.75 KB, patch)
2011-10-26 17:11 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Simplified patch based dependent upon bug 700856 (1.07 KB, patch)
2011-11-10 12:33 PST, Jared Wein [:jaws] (please needinfo? me)
fryn: feedback+
Details | Diff | Splinter Review
Patch with test fixes (85.69 KB, patch)
2011-12-08 14:42 PST, Jared Wein [:jaws] (please needinfo? me)
bzbarsky: review+
dholbert: feedback+
Details | Diff | Splinter Review
Android bustage fix (1.04 KB, patch)
2011-12-16 12:49 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
Android bustage fix (1.99 KB, patch)
2011-12-16 13:01 PST, Matt Brubeck (:mbrubeck)
mark.finkle: review+
Details | Diff | Splinter Review
background checkered (5.40 KB, image/png)
2011-12-29 05:10 PST, fx4waldi
no flags Details

Description Carlo Alberto Ferraris 2007-04-10 00:57:25 PDT
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.
Comment 1 Samuel Sidler (old account; do not CC) 2007-05-30 02:59:18 PDT
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...
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-05-30 09:56:00 PDT
"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.
Comment 3 Carlo Alberto Ferraris 2008-02-05 15:57:30 PST
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.
Comment 4 Carlo Alberto Ferraris 2011-05-03 04:26:00 PDT
Created attachment 529675 [details] [diff] [review]
Change the background color of nsImageDocument to #333

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.
Comment 5 Carlo Alberto Ferraris 2011-05-03 08:33:18 PDT
Created attachment 529727 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

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
Comment 6 Carlo Alberto Ferraris 2011-05-05 00:48:36 PDT
Ok, so... any comments on the patch?
Comment 7 [:Aleksej] 2011-05-05 04:00:15 PDT
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
Comment 8 Timothy Nikkel (:tnikkel) 2011-05-05 08:36:21 PDT
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.
Comment 9 Carlo Alberto Ferraris 2011-05-05 09:01:52 PDT
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.
Comment 10 Carlo Alberto Ferraris 2011-05-11 23:20:32 PDT
Just so that I know it... how often is considered good practice to harass reviewers to get them to look at my patch?
Comment 11 Timothy Nikkel (:tnikkel) 2011-05-12 09:15:37 PDT
Maybe after 2-3 weeks with no activity.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-18 09:09:19 PDT
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...
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 09:22:14 PDT
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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 09:25:15 PDT
And in particular, please test the behavior of large images with "browser.enable_automatic_image_resizing" set to both true and false....
Comment 15 Carlo Alberto Ferraris 2011-05-19 05:26:56 PDT
Created attachment 533611 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

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.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 06:45:43 PDT
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....
Comment 17 Carlo Alberto Ferraris 2011-05-19 06:57:14 PDT
Yes it is shown at its intrinsic size, with scrollbars and all.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 07:07:11 PDT
Why?
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 07:08:00 PDT
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...
Comment 20 Carlo Alberto Ferraris 2011-05-19 07:10:06 PDT
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.
Comment 21 Carlo Alberto Ferraris 2011-05-19 07:40:37 PDT
Created attachment 533641 [details] [diff] [review]
Dark background and central alignment in nsMediaDocument

Latest version with the changes mentioned in my previous comment
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-05-27 23:31:07 PDT
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....
Comment 23 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 11:32:03 PDT
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
Comment 24 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 11:33:39 PDT
cc'ing fellow UX people for additional thoughts
Comment 25 Carlo Alberto Ferraris 2011-06-09 11:06:24 PDT
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.
Comment 26 Carlo Alberto Ferraris 2011-07-15 05:14:22 PDT
:faaborg or other UX people: any additional thoughts on this? Should I simply rework the patch with what's already been suggested?
Comment 27 Carlo Alberto Ferraris 2011-07-30 00:10:32 PDT
(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.
Comment 28 Alex Faaborg [:faaborg] (Firefox UX) 2011-07-30 15:03:59 PDT
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?
Comment 29 Carlo Alberto Ferraris 2011-07-30 16:23:33 PDT
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?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-07-30 19:42:26 PDT
Alex, does the patch need reviews before landing on the UX branch, or does the UX branch not feed directly into m-c?
Comment 31 Alex Faaborg [:faaborg] (Firefox UX) 2011-07-30 19:53:09 PDT
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?
Comment 32 Stephen Horlander [:shorlander] 2011-08-01 21:24:37 PDT
(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
Comment 33 Frank Yan (:fryn) 2011-08-01 23:13:33 PDT
(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.
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2011-08-09 12:15:49 PDT
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.
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2011-08-09 12:16:29 PDT
*** Bug 472942 has been marked as a duplicate of this bug. ***
Comment 36 Jared Wein [:jaws] (please needinfo? me) 2011-08-09 20:33:54 PDT
Created attachment 551999 [details] [diff] [review]
Patch for bug 376997

I've unbitrotted the patch and landed it on the UX branch.

https://hg.mozilla.org/projects/ux/rev/b69bcb41ac36
Comment 37 Dão Gottwald [:dao] 2011-08-10 00:19:27 PDT
Why isn't this assigned to Carlo?
Comment 38 Jared Wein [:jaws] (please needinfo? me) 2011-08-10 00:21:46 PDT
(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.
Comment 39 Ekanan Ketunuti 2011-08-10 00:40:30 PDT
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
Comment 40 Jared Wein [:jaws] (please needinfo? me) 2011-08-10 01:02:10 PDT
(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?
Comment 41 Ekanan Ketunuti 2011-08-10 01:32:59 PDT
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
Comment 42 Jared Wein [:jaws] (please needinfo? me) 2011-08-10 08:10:18 PDT
(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.
Comment 43 :Margaret Leibovic 2011-08-11 10:09:50 PDT
(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
Comment 44 Alex Faaborg [:faaborg] (Firefox UX) 2011-08-12 11:26:48 PDT
ui-review is just waiting on this working on ux branch so we can try it out.
Comment 45 Jared Wein [:jaws] (please needinfo? me) 2011-08-12 11:33:11 PDT
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.
Comment 46 Carlo Alberto Ferraris 2011-08-27 10:39:05 PDT
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.
Comment 47 Jared Wein [:jaws] (please needinfo? me) 2011-08-31 16:24:16 PDT
browser/installer/package-manifest.in will likely need to be updated to include a reference to nsMediaDocument.css
Comment 48 Jared Wein [:jaws] (please needinfo? me) 2011-09-12 23:52:13 PDT
Carlo: Has there been any more progress on this bug?
Comment 49 Carlo Alberto Ferraris 2011-09-18 08:16:13 PDT
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).
Comment 50 Carlo Alberto Ferraris 2011-09-18 13:55:00 PDT
Created attachment 560816 [details] [diff] [review]
Second unbitrotted patch

Brought up-to-date to today's trunk. Obsoletes Jared's patch (even if bugzilla doesn't allow me to mark it as such).
Comment 51 Carlo Alberto Ferraris 2011-09-18 14:06:10 PDT
Created attachment 560817 [details] [diff] [review]
Second unbitrotted patch (with fixes)

Sorry, I forgot to commit a couple of changes. This one should be the correct one.
Comment 52 Carlo Alberto Ferraris 2011-09-19 13:16:30 PDT
Created attachment 560997 [details] [diff] [review]
Second unbitrotted patch (with fixes and cleanups)

Some further cleanups
Comment 53 Carlo Alberto Ferraris 2011-09-23 07:18:23 PDT
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?
Comment 54 Jared Wein [:jaws] (please needinfo? me) 2011-09-23 14:22:27 PDT
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
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2011-09-23 14:41:33 PDT
We should just fix the tests.  Testing something other than what we ship to users is pointless.
Comment 56 Carlo Alberto Ferraris 2011-09-25 23:10:50 PDT
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*.
Comment 57 Boris Zbarsky [:bz] (still a bit busy) 2011-09-25 23:16:10 PDT
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).
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2011-09-25 23:17:46 PDT
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...
Comment 59 Alex Limi (:limi) — Firefox UX Team 2011-09-25 23:18:31 PDT
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. :)
Comment 60 Justin Dolske [:Dolske] 2011-09-26 00:52:25 PDT
The video style was changed in bug 472942. I also see a lack of controls. *sigh*

I'll file a followup.
Comment 61 Jared Wein [:jaws] (please needinfo? me) 2011-09-27 10:52:01 PDT
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?
Comment 62 Carlo Alberto Ferraris 2011-09-29 01:23:02 PDT
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.
Comment 63 Carlo Alberto Ferraris 2011-09-29 08:13:07 PDT
Created attachment 563412 [details] [diff] [review]
Patch v3

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.
Comment 64 Dão Gottwald [:dao] 2011-09-29 08:21:18 PDT
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
Comment 65 Carlo Alberto Ferraris 2011-09-29 08:24:28 PDT
(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
Comment 66 Dão Gottwald [:dao] 2011-09-29 08:27:49 PDT
(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?
Comment 67 Carlo Alberto Ferraris 2011-09-29 10:23:12 PDT
Created attachment 563450 [details] [diff] [review]
Patch v4

whoops, forgot the quotes around the data: uri
Comment 68 Jared Wein [:jaws] (please needinfo? me) 2011-09-29 10:32:11 PDT
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.
Comment 69 Carlo Alberto Ferraris 2011-09-29 12:24:04 PDT
Created attachment 563520 [details] [diff] [review]
Patch v5

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.
Comment 70 Jared Wein [:jaws] (please needinfo? me) 2011-09-29 16:10:08 PDT
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
Comment 71 Carlo Alberto Ferraris 2011-09-29 23:15:58 PDT
(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)
Comment 72 Timothy Nikkel (:tnikkel) 2011-09-30 12:37:19 PDT
(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.
Comment 73 Jared Wein [:jaws] (please needinfo? me) 2011-10-04 16:15:49 PDT
(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 :)
Comment 74 Timothy Nikkel (:tnikkel) 2011-10-05 14:54:34 PDT
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.
Comment 75 Timothy Nikkel (:tnikkel) 2011-10-18 18:53:36 PDT
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
Comment 76 Jared Wein [:jaws] (please needinfo? me) 2011-10-26 17:11:02 PDT
Created attachment 569847 [details] [diff] [review]
Carlo's Patch v5 (rebased)

Rebased the patch. I'll push to try server shortly.
Comment 77 Jared Wein [:jaws] (please needinfo? me) 2011-10-26 17:14:45 PDT
Try server link:
https://tbpl.mozilla.org/?tree=Try&rev=78ff2a1ef6f4
Comment 78 Mozilla RelEng Bot 2011-10-27 00:00:45 PDT
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
Comment 79 Alfred Kayser 2011-11-01 05:58:27 PDT
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.
Comment 80 Alfred Kayser 2011-11-02 09:06:57 PDT
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==
Comment 81 Jared Wein [:jaws] (please needinfo? me) 2011-11-08 16:07:22 PST
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.
Comment 82 Jared Wein [:jaws] (please needinfo? me) 2011-11-08 16:08:52 PST
Sorry, part 2 should have been linked to bug 700856.
Comment 83 Jared Wein [:jaws] (please needinfo? me) 2011-11-10 12:33:23 PST
Created attachment 573600 [details] [diff] [review]
Simplified patch based dependent upon bug 700856

Simplified patch based dependent upon bug 700856
Comment 84 Frank Yan (:fryn) 2011-11-10 15:46:38 PST
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.
Comment 85 Frank Yan (:fryn) 2011-11-10 16:04:54 PST
By the way, I think the background color should be darker;
I recommend #222 instead of #333.
Comment 86 Frank Yan (:fryn) 2011-11-10 16:27:32 PST
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.
Comment 87 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2011-11-10 16:59:55 PST
(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?
Comment 88 Jared Wein [:jaws] (please needinfo? me) 2011-11-10 17:13:37 PST
(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.
Comment 89 Jared Wein [:jaws] (please needinfo? me) 2011-12-06 02:22:56 PST
(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.
Comment 90 Jared Wein [:jaws] (please needinfo? me) 2011-12-06 13:51:43 PST
(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|).
Comment 91 Jared Wein [:jaws] (please needinfo? me) 2011-12-07 03:01:00 PST
Updated the title of the bug since bug 472942 already updated video documents.
Comment 92 Daniel Holbert [:dholbert] 2011-12-07 11:51:24 PST
(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.
Comment 93 Jared Wein [:jaws] (please needinfo? me) 2011-12-08 14:42:29 PST
Created attachment 580202 [details] [diff] [review]
Patch with test fixes

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.
Comment 94 Daniel Holbert [:dholbert] 2011-12-13 11:59:46 PST
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+! :)
Comment 95 Boris Zbarsky [:bz] (still a bit busy) 2011-12-14 14:04:00 PST
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?
Comment 96 Jared Wein [:jaws] (please needinfo? me) 2011-12-14 23:43:34 PST
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/97b8cff2764f

Thanks for your contributions Carlo! :)
Comment 97 Rob Campbell [:rc] (:robcee) 2011-12-16 11:09:22 PST
https://hg.mozilla.org/mozilla-central/rev/97b8cff2764f
Comment 98 Stefan 2011-12-16 12:21:26 PST
(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?
Comment 99 Matt Brubeck (:mbrubeck) 2011-12-16 12:49:49 PST
Created attachment 582363 [details] [diff] [review]
Android bustage fix

This caused image reftests on Android to fail because TopLevelImageDocument.css is not included on Android.  I think this should fix it.
Comment 100 Matt Brubeck (:mbrubeck) 2011-12-16 13:01:16 PST
Created attachment 582368 [details] [diff] [review]
Android bustage fix

Updated to apply to both XUL and native Android fennec.
Comment 101 Matt Brubeck (:mbrubeck) 2011-12-16 13:21:37 PST
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).
Comment 102 Matt Brubeck (:mbrubeck) 2011-12-16 13:29:04 PST
(By the way, the Try push has a misleading commit message; despite appearances it does not actually include the backout.)
Comment 103 Matt Brubeck (:mbrubeck) 2011-12-16 13:30:27 PST
https://tbpl.mozilla.org/?tree=Try&rev=3f150a307df7
Comment 104 Matt Brubeck (:mbrubeck) 2011-12-16 16:23:17 PST
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).
Comment 105 Alfred Kayser 2011-12-17 08:08:14 PST
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.
Comment 106 Matt Brubeck (:mbrubeck) 2011-12-17 10:22:59 PST
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?
Comment 108 Andrew 2011-12-19 07:14:39 PST
Here is most suitable color - #eceeef
Comment 109 Andrew 2011-12-19 07:21:55 PST
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;
}
Comment 110 Andrew 2011-12-19 07:25:17 PST
>>Here is most suitable color - #eceeef

Or color should depend on the setting:

user_pref("browser.display.background_color", "#eceeef");
Comment 111 TinyButStrong 2011-12-20 06:07:43 PST
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.
Comment 112 Logan Rosen [:Logan] 2011-12-20 06:10:24 PST
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
Comment 113 Paul Rouget [:paul] 2011-12-20 06:25:16 PST
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.
Comment 114 TinyButStrong 2011-12-20 08:19:13 PST
Or use same technique of this addon https://addons.mozilla.org/en-US/firefox/addon/dominant-color/ to change background color.
Comment 115 Daniel Glazman (:glazou) 2011-12-22 10:27:07 PST
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.
Comment 116 Frank Lion 2011-12-25 07:10:56 PST
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
Comment 117 Justin Dolske [:Dolske] 2011-12-25 17:56:21 PST
Nope. We're done here. Open new bugs if you have issues.
Comment 118 Frank Lion 2011-12-25 18:25:49 PST
(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. :)
Comment 119 Boris Zbarsky [:bz] (still a bit busy) 2011-12-25 19:23:45 PST
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.
Comment 120 Frank Lion 2011-12-26 02:36:58 PST
Boris, thanks for the clarification, it is appreciated.
Comment 121 Daniel Holbert [:dholbert] 2011-12-26 20:16:02 PST
(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.)
Comment 122 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2011-12-28 13:42:11 PST
*** Bug 713904 has been marked as a duplicate of this bug. ***
Comment 123 fx4waldi 2011-12-29 05:10:44 PST
Created attachment 584733 [details]
background checkered

background should be in the checkered as it is for example in Gimp
Comment 124 Andrew 2012-01-02 17:20:51 PST
.svg images not in center.
Comment 125 Daniel Holbert [:dholbert] 2012-01-02 20:50:58 PST
(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.)
Comment 126 rob64rock 2012-01-10 21:03:12 PST
(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?
Comment 127 Virtual_ManPL [:Virtual] - (ni? me) 2012-01-10 23:45:13 PST
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...
Comment 128 Dão Gottwald [:dao] 2012-01-11 01:20:45 PST
(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.
Comment 129 rob64rock 2012-01-11 06:25:06 PST
(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
Comment 130 Gian-Carlo Pascutto [:gcp] 2012-01-20 09:10:28 PST
>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.
Comment 131 Paul [pwd] 2012-01-29 04:21:38 PST
FWIW Adobe have also taken to using a similar grey for the background in Photoshop: http://youtu.be/pBIf9KljT68
Comment 132 Daniel Glazman (:glazou) 2012-01-29 04:39:48 PST
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.
Comment 133 Jared Wein [:jaws] (please needinfo? me) 2012-01-30 05:57:34 PST
(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.
Comment 134 mozilla 2012-02-08 16:19:17 PST
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
Comment 135 David E. Ross 2012-03-15 16:24:42 PDT
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.
Comment 136 Gingerbread Man 2012-03-19 20:52:08 PDT
(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
Comment 137 v2ike6udik 2012-03-20 02:59:52 PDT
"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).
Comment 138 Paul Sinnett 2012-03-20 12:45:05 PDT
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.
Comment 139 candylandish 2012-03-22 01:12:23 PDT
For people trying to fix this change, there's an addon called Old Default Image Style. 

(Whatever happened to about:config?)
Comment 140 Gingerbread Man 2012-03-22 02:11:22 PDT
(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)
Comment 141 rupa 2012-03-22 21:55:39 PDT
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.
Comment 142 Seungbeom Kim 2012-03-25 15:31:21 PDT
(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?
Comment 143 Daniel Holbert [:dholbert] 2012-03-25 15:53:38 PDT
(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)
Comment 144 Chris Gutzman 2012-03-27 00:07:30 PDT
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.
Comment 145 MySh 2012-03-27 11:19:13 PDT
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.
Comment 146 Rob Campbell [:rc] (:robcee) 2012-03-27 12:59:56 PDT
unsubscribe.
Comment 147 Mike Hommey [:glandium] 2012-03-28 12:53:57 PDT
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.
Comment 148 Paul Rouget [:paul] 2012-03-28 13:02:10 PDT
Addons to go back to the previous behavior: https://addons.mozilla.org/en-US/firefox/addon/old-default-image-style/
Comment 149 mixxster 2012-04-01 14:57:50 PDT
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.
Comment 150 EvilHom3r 2012-04-04 15:35:51 PDT
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.
Comment 151 Mark Goodhand 2012-04-06 06:41:00 PDT
(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?).
Comment 152 Masatoshi Kimura [:emk] (use Splinter to ask me for review, see bugzil.la/1321953) 2012-04-06 06:47:47 PDT
Newsgroups would be better than the bug because Bugzilla is not a discussion forum.
Comment 153 Mark Goodhand 2012-04-06 07:48:36 PDT
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.
Comment 154 Mark Goodhand 2012-04-06 07:50:33 PDT
Besides, mixxster and MySh were specifically looking for somewhere to vote.  Are votes on bugs not welcome anymore?
Comment 155 Andre Klapper 2012-04-06 07:54:11 PDT
(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.
Comment 156 Mark Goodhand 2012-04-06 08:10:34 PDT
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.
Comment 157 Andre Klapper 2012-04-06 08:13:42 PDT
Voting against something never existed. 
If you want to discuss it, search for the bug report that requests it.
Comment 158 Mark Goodhand 2012-04-07 00:44:06 PDT
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.
Comment 159 Paul Sinnett 2012-04-07 04:41:47 PDT
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.
Comment 160 Terrell Kelley 2012-04-07 14:52:50 PDT
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.
Comment 161 Dieter Hansen 2012-04-10 19:42:18 PDT
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!
Comment 162 Dieter Hansen 2012-04-10 19:44:26 PDT
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!
Comment 163 Mark Goodhand 2012-04-11 00:34:35 PDT
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).
Comment 164 Carlo Alberto Ferraris 2012-04-11 02:16:23 PDT
(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.
Comment 165 Alfred Kayser 2012-04-11 12:56:06 PDT
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.
Comment 166 Jon B 2012-04-21 11:42:53 PDT
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.
Comment 167 Alfred Kayser 2012-04-22 08:30:05 PDT
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)
Comment 168 Andrew Poth 2012-04-23 12:03:19 PDT
(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.
Comment 169 Carlo Alberto Ferraris 2012-04-24 22:10:04 PDT
(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.
Comment 170 Alfred Kayser 2012-04-25 03:12:09 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.