452969 - [FIX]File upload control has text too gray, barely visible

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marius Hudea]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

The name of the file uploaded in an file upload control is shown with a very light gray, which is barely noticeable. People that change gamma setting may not even see it. See picture. 

Reproducible: Always

Steps to Reproduce:
1.
2.
3.


Expected Results:  
Font color should be darker, if not normal black.

Comment 1

[Marius Hudea]

Attachment: Example of the default font, default color in upload file form

Comment 2

[Marius Hudea]

That control actually has these css rules attached:

	background:#252721 none repeat scroll 0%;
	border-color:#000000 rgb(70, 70, 70) rgb(70, 70, 70) rgb(0, 0, 0);
	border-style:solid;
	border-width:1px;
	color:#E7E7E7;
	font-family:Verdana,Arial,Helvetica,sans-serif;
	font-size:13px;

In Firefox 2 the rules worked fine, now in Firefox 3 the background is always white but the color of text is enforced, which makes it barely visible.

I can only imagine how many websites would have this problem and how many programmers/designers don't know yet that this issue messes up the page design and accessibility.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This seems to be a native-theming screwup.  In particular, adding "-moz-appearance: none" to the CSS for that input makes everything work fine.

Really, there are two bugs here as far as I can tell:

1)  We don't drop native theming for the kids of the file control.  We probably
    should, but native-theming-dropping doesn't handle inheritance very well,
    apparently.
2)  The native theme only paints backgrounds and lets colors be painted by other
    layout code.  The result, in this case, is this bug.

Really, I doubt we can fix #2, so I guess we better fix #1.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to do the trick

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 336426 [details] [diff] [review]
This seems to do the trick

>+               // Have to do this check right here, so that later UA rules
>+               // won't clobber values[i] with eCSSUnit_Dummy.

I'm having trouble understanding this comment.  (The code makes sense, but I can't make sense of the comment.)

>+               childValues[i]->GetUnit() == eCSSUnit_DummyInherit))

It also seems like you could just have a single array, and when you're done, if you have any dummy inherits, you clear all the dummy inherits back to null and set everything else to dummy when you step up to the parent.  And this way, you could probably do what you're doing as a recursive step as a loop instead.

Does that make sense?  (Maybe it doesn't, in which case I'll reconsider...)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I'm having trouble understanding this comment. 

Initially I didn't do the early return there, just kept going and had the return in the default: case in the switch, but that doesn't work, of course, because a lower-precedence UA rule would change "some other value" into eCSSUnit_Dummy.  I can remove the comment or reword; I agree that as written it's confusing.

> seems like you could just have a single array ...

Hmm.  Yeah, that seems like it should work.  You're right that it's better that way; I'll make that change.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: With that approach

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

If you have any bright ideas on testing this, let me know... putting a text and button input next to each other doesn't look quite the same.  :(

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 338421 [details] [diff] [review]
Same as diff -w

r+sr=dbaron.  Sorry for the delay.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 177b174c83e4.  I have no idea how to write a test for this.  :(

Comment 12

[philippe (part-time)]

Boris, is it possible to backport this to the 1.9.0 branch ?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, but I'm not sure we should be doing this: this isn't seriously breaking large numbers of sites, and it's not security-related in any way.

If someone wants to try to convince drivers to take this on branch, go for it.