Closed Bug 793366 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Patch v.1 (obsolete) — Splinter Review
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)
Attachment #663644 - Flags: review?(jaws)
Attached image Screenshot
Assignee: nobody → dolske
Attachment #663647 - Flags: ui-review?(shorlander)
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.
Attachment #663644 - Flags: review?(jaws) → review+
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 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.
Attachment #663647 - Flags: ui-review?(shorlander) → ui-review+
Attached patch Patch v.2Splinter Review
Move stuff to the media subdir, and fixed up reftests.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f429d0c48b84
Attachment #663644 - Attachment is obsolete: true
Attachment #665185 - Flags: review?(jaws)
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.
Attachment #665185 - Flags: review?(jaws) → review+
Try run was green, I think checking this in just got forgotten. Although I see that the patch is missing summary and author details.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Yeah, was just waiting for Try and ui-r. Thanks whomever lands this! :)
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.
Attachment #665185 - Flags: review?(joe)
Attachment #665185 - Flags: review?(bzbarsky)
Comment on attachment 665185 [details] [diff] [review]
Patch v.2

r=me
Attachment #665185 - Flags: review?(bzbarsky) → review+
Attachment #665185 - Flags: review?(joe) → review+
https://hg.mozilla.org/mozilla-central/rev/3685d18499d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 848018
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).
Depends on: 850504
You need to log in before you can comment on or make changes to this bug.