322890 - Input type="file" has opaque/white background

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: testcase

Compare Camino's rendering to Firefox's

Comment 2

[Chris Lawson (gone)]

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

[Chris Lawson (gone)]

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

cl

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: patch v2

The braintrust on irc decided that "transparent" was more appropriate than "inherit"; the same caveats apply.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 7

[Chris Lawson (gone)]

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

[Chris Lawson (gone)]

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

Comment 9

[Chris Lawson (gone)]

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

Comment 10

[Chris Lawson (gone)]

Tweaking summary to reflect comment 7.

cl

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Chris Lawson (gone)]

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

cl

Comment 13

[Chris Lawson (gone)]

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

Comment 14

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

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.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: "Even more expanded testcase" as we currently see it in Camino

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: "Even more expanded testcase" with Chris's hacked forms.css

Comment 18

[Chris Lawson (gone)]

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

[Chris Lawson (gone)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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... ;)

Comment 21

[Samuel Sidler (old account; do not CC)]

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

[Chris Lawson (gone)]

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

cl

Comment 23

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

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 24

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

Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

minusing for lack of response to questions

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

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

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

Comment 27

[Samuel Sidler (old account; do not CC)]

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

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

Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

ok, r+sr=dbaron, with comments above

Comment 29

[Samuel Sidler (old account; do not CC)]

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.

Comment 30

[Samuel Sidler (old account; do not CC)]

Reassigning this bug to bz since it's his patch.

Comment 31

[Mike Pinkerton (not reading bugmail)]

can someone poke drivers? we won't hold for this, but....

Comment 32

[Samuel Sidler (old account; do not CC)]

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

Comment 33

[Mark Mentovai]

Checked in on CAMINO_1_0_BRANCH.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Boris Zbarsky [:bzbarsky]]

Fixed on the 1.8.1 branch.  Please do file a bug on fixing this on trunk, ok?

Comment 36

[Samuel Sidler (old account; do not CC)]

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

[Samuel Sidler (old account; do not CC)]

Comment on attachment 208354 [details] [diff] [review]
prevents painting a background on the file widget

Approval should be on the other patch.

Comment 38

[Samuel Sidler (old account; do not CC)]

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.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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 40

[Daniel Veditz [:dveditz]]

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

not a 1.8.0.x blocker

Comment 41

[Samuel Sidler (old account; do not CC)]

bz, can you please request approval for the 1.8.0 branch using words more eloquent than mine? Thank you.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.

Comment 43

[Samuel Sidler (old account; do not CC)]

We'll need to take this on a minibranch for 1.0.1.

Comment 44

[Samuel Sidler (old account; do not CC)]

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.

Comment 45

[Tim Riley [:timr]]

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

Important camino patch.  a=timr for drivers.

Comment 46

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

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?

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch to do that (for 1.8/1.8.0 branches only)

Comment 49

[Boris Zbarsky [:bzbarsky]]

Checked in additional patch to 1.8.1.  Waiting for that to get approval before landing both patches on 1.8.0.x.

Comment 50

[Darin Fisher]

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!

Comment 51

[Boris Zbarsky [:bzbarsky]]

> 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

[Darin Fisher]

Thanks, that answers the question.  The relationship between the patches wasn't clear to me.

Comment 53

[Samuel Sidler (old account; do not CC)]

Mark, when you get a chance, can you land the last patch here on CAMINO_1_0_1_MINIBRANCH?

Comment 54

[Mark Mentovai]

Attachment 210774 [details] [diff], attachment 217354 [details] [diff] [review] landed on CAMINO_1_0_1_MINIBRANCH off of 1.8.0.2.

Comment 55

[Daniel Veditz [:dveditz]]

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

Comment 56

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0.3