Justin Dolske [:Dolske] Assignee
	Description • 12 years ago

Bug 754133 changes our ImageDocument display to use a white (#ffffff) background for images, so that transparent images assuming a white background are still readable with our stylized imagedocs.

I think we can improve this further, by adding some noise to that part (as we do with the dark background of the rest of the page). Looks a bit nicer, and acts as a (very subtle) cue that there's actually transparency, and not just a image saved with a white background.

Some previously referenced example images:

http://www.cdc.gov/nchs/data/databriefs/db88_fig1.png
http://www.mozilla.org/images/template/dino.png
http://www.mozilla.org/images/template/screen/logo_footer.png

I've also moved the data: url for the existing noise texture to an actual image file.

The image for the light noise is a copy from what's used in about:home (chrome://browser/content/abouthome/noise.png)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 2 • 12 years ago

Comment on attachment 663644 [details] [diff] [review]
Patch v.1

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

Code here looks fine. As long as the noise isn't too bothersome for the graph/chart use case I'm fine with it.

As discussed on IRC, maybe move the image files and the TopLevel*Document.css files to /toolkit/themes/*stripe/media.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 12 years ago

Oh, I forgot to mention...

This change will probably break a lot of image reftests. See https://mxr.mozilla.org/mozilla-central/source/image/test/reftest/ImageDocument.css for one of the places that will also need to be updated.

There are a couple ways to work around the image reftest situation, but I'm not sure which one we should do at the moment.

	Stephen Horlander [:shorlander]
	Comment 4 • 12 years ago

Comment on attachment 663647 [details]
Screenshot

I like it. It's subtle enough that it doesn't really affect dark on light images and yet it makes a pretty big difference in visibility of light on dark images.

	Justin Dolske [:Dolske] Assignee
	Comment 5 • 12 years ago

Move stuff to the media subdir, and fixed up reftests.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f429d0c48b84

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 12 years ago

Comment on attachment 665185 [details] [diff] [review]
Patch v.2

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

r+ as long as the reftests pass. +1 for the NB.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 12 years ago

Try run was green, I think checking this in just got forgotten. Although I see that the patch is missing summary and author details.

	Justin Dolske [:Dolske] Assignee
	Comment 8 • 12 years ago

Yeah, was just waiting for Try and ui-r. Thanks whomever lands this! :)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 9 • 12 years ago

Comment on attachment 665185 [details] [diff] [review]
Patch v.2

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

Just realized that this also needs review from someone for the /content/html and /image/test portions. Flagging as appropriate.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 years ago

Comment on attachment 665185 [details] [diff] [review]
Patch v.2

r=me

	Jared Wein [:jaws] (please needinfo? me)
	Comment 11 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3685d18499d9

	Ed Morley [:emorley]
	Comment 12 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3685d18499d9

	Patrick Storz
	Comment 13 • 12 years ago

Is this really what we want to do? Adding noise on purpose?

I strongly recommend to undo this change. I filed a bug report (bug 848018) before because I couldn't imagine this was intended behavior. Also the noisy gray site background should be changed (bug 789820).