Input type="file" has opaque/white background

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: bz)

Tracking

({fixed1.8.0.4, fixed1.8.1})

1.8 Branch
mozilla1.8.1
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [camino-1.0][camino-1.0.1])

Attachments

(6 attachments, 4 obsolete attachments)

2.88 KB, text/html
Details
95.06 KB, image/png
Details
96.55 KB, image/png
Details
1.12 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.49 KB, patch
Details | Diff | Splinter Review
1.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
If the parent/ancestor element of an <input type="file"> element has a background color other than white specified and the <input type="file"> element has no bgcolor specified, the area between the button and the input, as well as the "corners" of the gfx button area, still appear white.  These areas should inherit the bg color of the parent/ancestor.  Fx does this correctly.
Created attachment 208056 [details]
testcase

Compare Camino's rendering to Firefox's

Comment 2

12 years ago
As you can see in the testcase, this is only a problem with INPUT TYPE=FILE and not with other input boxes or buttons.

cl

Comment 3

12 years ago
Another forms.css issue. Smokey's got the patch on this one.

cl
Assignee: mikepinkerton → alqahira
Created attachment 208062 [details] [diff] [review]
patch

So this fixes the issue by adding

input[type="file"] {
  background-color: inherit;
}

to the top of the "input" rules in the Cocoa section of forms.css so as not to mess with Fx or the rest of our cascade.

I haven't run this through the battery of form widget tests I used with the last bug yet, so I'm not requesting reviews until I do some testing later in the week (plus I'd like some thoughts on whether this could potentially break anything, but so far Chris and I haven't come up with anything).
Created attachment 208064 [details] [diff] [review]
patch v2

The braintrust on irc decided that "transparent" was more appropriate than "inherit"; the same caveats apply.
Attachment #208062 - Attachment is obsolete: true
Comment on attachment 208064 [details] [diff] [review]
patch v2

This doesn't work, though.  Either it needs to special-case the text part of the input, or we need to go back to v1.

Right now the text field is transparent, which is bad.  Sorry for the bugspam.
Attachment #208064 - Attachment is obsolete: true

Comment 7

11 years ago
Line 556 in forms.css is what broke this.

http://lxr.mozilla.org/mozilla/source/layout/style/forms.css#556

We force the -moz-field background colour on the all input elements, including input type=file.

For reasons I don't yet understand, overriding it in line 684/685 by inserting

background-color: transparent;

doesn't work.

cl

Comment 8

11 years ago
Created attachment 208175 [details]
Expanded testcase including all possible form input elements

As you can see in this testcase, we also do the wrong thing on disabled INPUT TYPE=IMAGE backgrounds. We should try to fix this as well.

cl
Attachment #208056 - Attachment is obsolete: true

Comment 9

11 years ago
Created attachment 208179 [details]
Even more expanded testcase

Adds untyped INPUT element (defaults to TEXT according to spec) to testcase.

I've sent a doctored forms.css file to Smokey for his perusal/testing. Will make a patch (or Smokey can) around the weekend.

cl
Attachment #208175 - Attachment is obsolete: true

Comment 10

11 years ago
Tweaking summary to reflect comment 7.

cl
Summary: Input type="file" has opaque/white background in Camino → Input type="file" and disabled input type="image" has opaque/white background in Camino
I think Sam's screenshot http://www.samuelsidler.com/camino/bugs/322890.png shows that Chris was right and this really is a Core bug.  

That said, it's barely noticeable on WinXP (which is the only other place it appears so far), so we may still want to consider fixing this ourselves in the Cocoa section because it really affects Camino and makes things ugly.

I'm going to give Chris's latest forms.css a spin later tonight.

Comment 12

11 years ago
--> Core per josh on IRC, after seeing the screenshot in comment 11.

cl
Component: HTML Form Controls → Layout: Form Controls
Product: Camino → Core
Version: unspecified → Trunk

Comment 13

11 years ago
Hardware/OS -> All/All, also per comment 11.

This affects Camino the worst, and affects XP stuff as well. It will probably affect Firefox/Mac when the switch to Cocoa widgets is complete.

cl
OS: MacOS X → All
Hardware: Macintosh → All
So, it seems like the obvious solution here would be to add 'background: transparent' to the input[type="file"] rule in forms.css and remove the 'background-color: inherit' from the input[type="file"] > input[type="text"][disabled] rule.  That would change the color in any pages that specify a background color for a file input, though.  I'm not sure if anyone cares.  I certainly don't; I'd rather all these rules were !important.
Sam asked me to post some screenshots of what we currently see in Camino and then what we see with Chris's hacked forms.css; those are forthcoming.

The hacked forms.css I think is generally in line with dbaron's suggestion in comment 14 (though there are lots of tiny changes in there messing with line numbers and making it hard for me to tell for sure)--but with those rules applied only in the Cocoa section--plus a Cocoa-section fix for the white/opaque bg in disabled input[type=image].

Since this is now a Core bug that affects multiple products and platforms, I don't think it's appropriate for me to own it any longer.
Assignee: alqahira → nobody
QA Contact: layout.form-controls
Created attachment 208258 [details]
"Even more expanded testcase" as we currently see it in Camino
Created attachment 208259 [details]
"Even more expanded testcase" with Chris's hacked forms.css

Comment 18

11 years ago
The patch that bz gave me on IRC fixes the original issue (FILE input).

We can take the second issue separately in Camino.

cl

Comment 19

11 years ago
Created attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

This is the patch bz gave me on IRC last night. It fixes the issue in a trunk build of Camino that I built. Unfortunately, that's all the testing I can offer at this time. (I don't have access to a Windows machine for building, although I'd be happy to test a custom build if someone else can build it.)

Could someone more knowledgeable about Windows widgets please have a look at the SUBMIT button in the screenshot in comment 11? I can't tell if the white line under the button is supposed to be there or not.

cl
Attachment #208354 - Flags: review?
Attachment #208354 - Flags: superreview?(dbaron)
Attachment #208354 - Flags: review?(bzbarsky)
Attachment #208354 - Flags: review?
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

It's pretty silly for me to review a patch I basically wrote... ;)
Attachment #208354 - Flags: review?(bzbarsky) → review?(dbaron)
dbaron: We'd like this on the 1.8 branch and, if possible, on the 1.8.0 branch, though I know that's less likely. It should be low risk though.

Comment 22

11 years ago
Fixing summary to reflect current scope of bug. Apologies for the bugspam.

cl
Summary: Input type="file" and disabled input type="image" has opaque/white background in Camino → Input type="file" has opaque/white background
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Paint layers aren't really the ideal way to do this, but probably the best one available, at least until roc's rework lands.  But please add a comment explaining that the background is inherited to the text input.  But what about border and padding?  Have you tested the effects this has one those?
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

minusing for lack of response to questions
Attachment #208354 - Flags: superreview?(dbaron)
Attachment #208354 - Flags: superreview-
Attachment #208354 - Flags: review?(dbaron)
Attachment #208354 - Flags: review-
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Sorry.  Didn't realize those were directed at me.

The relevant rule in forms.css is:

309 input[type="file"] {
...
315   padding: 0 !important;
316   border-style: none !important;
317 }

So there are no borders or padding.
Attachment #208354 - Flags: superreview?(dbaron)
Attachment #208354 - Flags: superreview-
Attachment #208354 - Flags: review?(dbaron)
Attachment #208354 - Flags: review-
Given that roc's patch is going to land very soon, probably better to wait, since the solution once that lands will be far superior (you'll actually be able to skip the background and border explicitly).
(In reply to comment #26)
> Given that roc's patch is going to land very soon, probably better to wait,
> since the solution once that lands will be far superior (you'll actually be
> able to skip the background and border explicitly).
> 

I think we'd like to get this on the branch, if possible. It really looks horrible in Camino with our Cocoa widgets. Taking it on a minibranch is acceptable, but not desired. It would improve Fx as well.
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

ok, r+sr=dbaron, with comments above
Attachment #208354 - Flags: superreview?(dbaron)
Attachment #208354 - Flags: superreview+
Attachment #208354 - Flags: review?(dbaron)
Attachment #208354 - Flags: review+
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Requesting approval for both branches. This is a low risk-patch which helps form widgets visually and will especially help cocoa widgets (i.e. Camino). It's not being landed on the trunk for reasons explained in comment 26.
Attachment #208354 - Flags: approval1.8.1?
Attachment #208354 - Flags: approval1.8.0.2?
Reassigning this bug to bz since it's his patch.
Assignee: nobody → bzbarsky
can someone poke drivers? we won't hold for this, but....
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Moving request to branch-1.8.1.

dbaron, since you own this, do you mind approving this for the 1.8.1 branch? (Still waiting on drivers for the 1.8.0 branch.)
Attachment #208354 - Flags: approval1.8.1? → branch-1.8.1?(dbaron)

Comment 33

11 years ago
Checked in on CAMINO_1_0_BRANCH.
Whiteboard: [camino-1.0]
Created attachment 210774 [details] [diff] [review]
Updated to dbaron's comments -- check in this one

Has someone filed a followup on doing this the right way on trunk?  If not, please do so.
Attachment #210774 - Flags: branch-1.8.1+
Attachment #208354 - Flags: branch-1.8.1?(dbaron) → branch-1.8.1-
Fixed on the 1.8.1 branch.  Please do file a bug on fixing this on trunk, ok?
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1
(In reply to comment #35)
> Fixed on the 1.8.1 branch.  Please do file a bug on fixing this on trunk, ok?
> 

Filed bug 326019 to evaluate how to fix it on the trunk.
Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Approval should be on the other patch.
Attachment #208354 - Flags: approval1.8.0.2?
Comment on attachment 210774 [details] [diff] [review]
Updated to dbaron's comments -- check in this one

Requesting approval for this patch. See comment 29 for more information. r+sr=dbaron per comment 28.
Attachment #210774 - Flags: approval1.8.0.2?
Samuel, please make sure to check that patch in when it gets approved... I won't get bugmail for it, since I didn't make the request.
Comment on attachment 210774 [details] [diff] [review]
Updated to dbaron's comments -- check in this one

not a 1.8.0.x blocker
Attachment #210774 - Flags: approval1.8.0.2? → approval1.8.0.2-
bz, can you please request approval for the 1.8.0 branch using words more eloquent than mine? Thank you.
I'm not sure I can, if nothing else because I have no idea what the scope of the problem is.  Someone who does and feels this is appropriate for a stability branch could mail drivers and explain why.
We'll need to take this on a minibranch for 1.0.1.
Comment on attachment 210774 [details] [diff] [review]
Updated to dbaron's comments -- check in this one

Per conversation with dveditz, requesting approval for 1.8.0.3.

This was tested in Camino (and is even in Camino 1.0) and was also tested in Firefox/Win and Firefox/Lin. It is low risk, visually affecting Camino-only (due to how Cocoa widgets are rendered), but will help Camino not minibranch for each release.
Attachment #210774 - Flags: approval1.8.0.3?

Comment 45

11 years ago
Comment on attachment 210774 [details] [diff] [review]
Updated to dbaron's comments -- check in this one

Important camino patch.  a=timr for drivers.
Attachment #210774 - Flags: approval1.8.0.3? → approval1.8.0.3+
I just realized this patch should be updated so that nsFileControlFrame::CanPaintBackground returns false.  Otherwise an expose that's entirely within the bounds of a file control could paint garbage.  Right?
Yes.
Created attachment 217354 [details] [diff] [review]
Additional patch to do that (for 1.8/1.8.0 branches only)
Attachment #217354 - Flags: superreview?(roc)
Attachment #217354 - Flags: review?(roc)
Attachment #217354 - Flags: approval1.8.0.3?
Attachment #217354 - Flags: approval-branch-1.8.1?(roc)
Attachment #217354 - Flags: superreview?(roc)
Attachment #217354 - Flags: superreview+
Attachment #217354 - Flags: review?(roc)
Attachment #217354 - Flags: review+
Attachment #217354 - Flags: approval-branch-1.8.1?(roc)
Attachment #217354 - Flags: approval-branch-1.8.1+
Checked in additional patch to 1.8.1.  Waiting for that to get approval before landing both patches on 1.8.0.x.
Flags: blocking1.8.0.3?

Updated

11 years ago
Flags: blocking1.8.0.3? → blocking1.8.0.3+

Comment 50

11 years ago
Comment on attachment 217354 [details] [diff] [review]
Additional patch to do that (for 1.8/1.8.0 branches only)

can someone comment on the risk/reward of this patch w.r.t. to the 1.8.0.x branch?  how badly do we want this?  is this also only going to affect builds with cocoa widgets?  thanks!
> can someone comment on the risk/reward of this patch w.r.t. to the 1.8.0.x
> branch? 

For the patch in question, the risk is 0 -- the "can paint background" thing is an optimization.

The reward is that we can land the already-approved patch in this bug.

Or is the question what the risk/reward analysis is for the already-approved patch?

Comment 52

11 years ago
Thanks, that answers the question.  The relationship between the patches wasn't clear to me.
Mark, when you get a chance, can you land the last patch here on CAMINO_1_0_1_MINIBRANCH?

Comment 54

11 years ago
Attachment 210774 [details] [diff], attachment 217354 [details] [diff] [review] landed on CAMINO_1_0_1_MINIBRANCH off of 1.8.0.2.
Whiteboard: [camino-1.0] → [camino-1.0][camino-1.0.1]
Comment on attachment 217354 [details] [diff] [review]
Additional patch to do that (for 1.8/1.8.0 branches only)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #217354 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fixed on 1.8.0.3
Keywords: fixed1.8.0.3
Version: Trunk → 1.8 Branch
You need to log in before you can comment on or make changes to this bug.