793366 - Add noise to the white background used for transparent images in image documents

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

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)

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Screenshot

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 4

[Stephen Horlander [:shorlander]]

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.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

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

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 8

[Justin Dolske [:Dolske]]

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Patrick Storz]

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).