Last Comment Bug 712347 - "The image ... cannot be displayed because it contains errors." error message is hard to read on the dark background (imageDocument)
: "The image ... cannot be displayed because it contains errors." error message...
Status: RESOLVED FIXED
[good first bug][mentor=jwein][lang=css]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Greg Karz
:
: Jet Villegas (:jet)
Mentors:
http://twimg0-a.akamaihd.net/profile_...
: 741530 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 09:54 PST by Greg Karz
Modified: 2012-04-02 16:04 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected


Attachments
image.png (13.97 KB, image/png)
2011-12-20 09:54 PST, Greg Karz
no flags Details
Added color:#eee; and text-align:center; (693 bytes, patch)
2011-12-21 16:50 PST, Greg Karz
roc: review+
dao+bmo: feedback+
Details | Diff | Splinter Review
Screenshot of patch (333.40 KB, image/png)
2011-12-21 16:56 PST, Jared Wein [:jaws] (please needinfo? me)
shorlander: ui‑review+
Details

Description Greg Karz 2011-12-20 09:54:23 PST
Created attachment 583206 [details]
image.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:11.0a1) Gecko/20111220 Firefox/11.0a1
Build ID: 20111220031159

Steps to reproduce:

Open a malformatted image


Actual results:

"The image "..." cannot be displayed because it contains errors." message is hard to see on the dark background


Expected results:

It would be nice to have a lighter color for that text
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2011-12-20 13:22:51 PST
In the case of an image decoding error, error text is set as the "alt" text of the image. This alternative text is then displayed as the image.

It appears that we can fix this by setting a CSS |color| property on the |img| tag.

In /layout/style/TopLevelImageDocument.css, we may want to add the following inside the img rule:

img {
 color: #eee;
 ... other rules
}

Additionally, we may want to set some default font-families for this text. The current implementation doesn't allow for platform-specific themes, so we may want to wait, but it could be worth an investigation.
Comment 2 Greg Karz 2011-12-21 09:31:54 PST
Sorry to ask this here (couldn't get help on the irc), I have the firefox sources from mozilla-central, I added the color: #eee; to the |img| tag. What should be my next actions? How do I add it to mozilla-central? Or what is the procedure now? I would really like to help fix bugs.
Comment 3 Paul Rouget [:paul] 2011-12-21 09:41:26 PST
(In reply to Greg Karz from comment #2)
> Sorry to ask this here (couldn't get help on the irc), I have the firefox
> sources from mozilla-central, I added the color: #eee; to the |img| tag.
> What should be my next actions? How do I add it to mozilla-central? Or what
> is the procedure now? I would really like to help fix bugs.

Great! First, you need to build Firefox. Read this: https://developer.mozilla.org/En/Simple_Firefox_build

Then, you need to understand how to hack Firefox. Read this: https://developer.mozilla.org/en/Hacking_Firefox

Your goal is to create a patch and post it here.

I strongly encourage you to join us on IRC and ping "jaws".
Comment 4 Greg Karz 2011-12-21 16:50:09 PST
Created attachment 583679 [details] [diff] [review]
Added color:#eee; and text-align:center;

I added text-align:center so that the message is centered as an image would be. Is this a bad idea? (please warn me if I did something wrong)
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2011-12-21 16:56:32 PST
Created attachment 583683 [details]
Screenshot of patch
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2011-12-21 16:59:27 PST
(In reply to Greg Karz from comment #4)
> Created attachment 583679 [details] [diff] [review]
> Added color:#eee; and text-align:center;
> 
> I added text-align:center so that the message is centered as an image would
> be. Is this a bad idea? (please warn me if I did something wrong)

Nice job on your first patch! I've added a screenshot of it to this bug and requested UI-review from Stephen Horlander on the design.

I'm not sure if I like the text-align: center, but the lack of margins on the body push me towards liking text-align: center.

Also, I don't think this bug exists in the beta channel. The general way that we approach bugs in release/beta/aurora, is to get the bug fixed in trunk (mozilla-central), and then request approval for whatever branches that are also affected.
Comment 7 Greg Karz 2011-12-22 10:01:47 PST
Thanks! And btw, what is your IRC channel, or how can I access it (sry, I'm a newbie to IRC too) and how can I create a patch for another bug, so that it doesn't contain the fix in this patch?
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-12-22 10:13:39 PST
Either #introduction or #developers would be fine choices. For working on multiple patches, learning how to use Mercurial Queues (https://developer.mozilla.org/en/Mercurial_Queues) is strongly encouraged. #introduction is a great place to ask followup questions about general development practices that aren't related to a particular bug.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2011-12-22 10:17:23 PST
There is also more information about IRC here: https://wiki.mozilla.org/IRC

The IRC server is located at irc.mozilla.org
Comment 10 Stephen Horlander [:shorlander] 2011-12-22 16:15:02 PST
Comment on attachment 583683 [details]
Screenshot of patch

This looks good given the constraints. 

We should probably consider something nicer at some point though :)
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2011-12-22 16:23:55 PST
Comment on attachment 583679 [details] [diff] [review]
Added color:#eee; and text-align:center;

I think we should add a comment to these lines of CSS so people understand why we have set a color and text-align on the img, but other than it "looks good to me!".

I've requested feedback from Dao to get his input on the CSS change and review from :roc since this is in /layout
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-01-04 14:11:13 PST
Greg: Can you please follow the steps in Marco's blog post about readying your patch for check-in? http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-01-10 23:16:37 PST
Greg: Do you have any questions about making the patch ready for check-in? I'll go ahead and fix it up on Friday if there isn't any movement here.
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-01-13 16:32:15 PST
https://hg.mozilla.org/integration/fx-team/rev/7f728aef5ad8
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-01-14 20:33:53 PST
Congratulations on your first patch Greg!
Comment 16 Tim Taubert [:ttaubert] 2012-01-16 02:32:11 PST
https://hg.mozilla.org/mozilla-central/rev/7f728aef5ad8
Comment 17 Thomas Ahlblom 2012-04-02 16:04:57 PDT
*** Bug 741530 has been marked as a duplicate of this bug. ***

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