Closed Bug 793366 Opened 12 years ago Closed 12 years ago

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


(Toolkit :: Themes, defect)

Not set





(Reporter: Dolske, Assigned: Dolske)




(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:

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 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]

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

Attachment #665185 - Flags: review?(bzbarsky) → review+
Attachment #665185 - Flags: review?(joe) → review+
Closed: 12 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.