928891 - Add forms.css styling for input[type="color"]

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Per the reasoning in bug 875275 comment 8, we're currently holding off on adding custom styling for input[type="color"] in forms.css.

(basically, this is so that we still render <input type="color"> like <input> for people with the color pref turned off.)

I'm filing this bug to track adding the input[type="color"] styling to forms.css.

For starters, we probably should share the block of style for button, input[type="reset"], input[type="submit"] (to share all of that button theming).

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> I'm filing this bug to track adding the input[type="color"] styling to
> forms.css.

...(when we're getting ready to turn the pref on.)

Also: once we have this forms.css stuff in place, we can simplify our color reftests a bit. (in particular, we can probably remove testcase-style.css and reference-style.css)

Comment 2

[Daniel Holbert [:dholbert]]

The block of style for buttons that I was hinting at in comment 0 is here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=18f85a091e65&mark=527-548#527

(There are more chunks after that which we probably want to share as well, e.g. the "button:hover, input[type="reset"]:hover, ..." chunk.)

Comment 3

[Arnaud Bienner]

I think we need to have at least "cursor: default;" property and very probably "-moz-appearance: button;".
For the other, I don't know, but I think it makes sense.
Mounir suggested in bug 547004 comment 46 - 49 we might want to have a separate block, as some properties might be unneeded. But I think he meant we need to review in details what properties we need in order to know if we can have a common block or not.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #3)
> Mounir suggested in bug 547004 comment 46 - 49 we might want to have a
> separate block, as some properties might be unneeded.

Thanks! Selectively quoting some bits of that discussion, since there's a lot of unrelated stuff in those comments:

Mounir:
> Not sure if we really want to have "color" sharing the rules of regular
> buttons.
Arnaud:
> Any particularly points which may be problematic?
> Do you think I can leave this like this for now, or should I create
> another (very similar) rule in this patch?
Mounir:
> We should not do that generally speaking. I didn't look carefully to see
> what was needed or not. It's more than likely that we could remove some
> properties from that list for color.

So: I suspect we could maybe split that existing rule into e.g. "button styling" vs. "textual button styling", and include color on the first one but not on the latter one.

> I think we need to have at least "cursor: default;" property and very
> probably "-moz-appearance: button;".

I think we should probably share everything in that style rule that's not text-related.

Comment 5

[Arnaud Bienner]

Attachment: split_button_rules_forms_css.patch

First part: split rules between text/non-text attributes for buttons.
I'm not a CSS expert, so even if I did my best, please carefully review that the split is correct.
Pushed to try to be sure I'm not breaking anything:
https://tbpl.mozilla.org/?tree=Try&rev=16ae074b5a8f

Comment 6

[Arnaud Bienner]

Attachment: Add input color to form.css

Second part: add input[type=color] where needed in form.css.
Adding conditional checks to enable this or not depending on the platform would be part of bug 930277, which should be land together with this bug's patches, as discussed.

Comment 7

[Arnaud Bienner]

Second patch pushed to try (on top of the first one of course):
https://tbpl.mozilla.org/?tree=Try&rev=f7dbf60e0cad

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 822550 [details] [diff] [review]
split_button_rules_forms_css.patch

>+++ b/layout/style/forms.css
>-/* TODO: Once we're getting ready to turn on 'dom.forms.color' by default, we
>-   probably want to add input[type="color"] to this selector list, so that it
>-   gets the same styling as other form buttons. */
>+/* Non text-related attributes for buttons : those ones are shared with input[type="color"] */

Please wrap this comment to keep lines under 80 characters long. (like the comment that was there before)
(Maybe just add a newline after "buttons"?) 

Also, s/attributes/properties/. ("attributes" has a "non-CSS" implication, i.e. DOM attributes vs. CSS properties, so better to avoid the term here since we're listing properties.)

Also, s/those/these/

>+/* Text-related attributes for buttons : those ones are not shared with input[type="color"] */

As above:
- Wrap to 80 chars
- s/attributes/properties/
- s/those/these/

r=me with that

Comment 9

[Daniel Holbert [:dholbert]]

(Needs a commit message, too)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 822564 [details] [diff] [review]
Add input color to form.css

>[mq]: add_input_color_to_form.css

(Please include a commit message in final version.)

>+++ b/layout/reftests/forms/input/color/reference-style.css
>@@ -1,19 +1,8 @@
[...]
> 
> div.input-color-swatch {
>   /* This should match the styling for ::-moz-color-swatch in forms.css. */

This file ends up with a blank line at the beginning -- drop that.

>   min-width: 50px;
>   min-height: 1em;

This reminds me -- we probably need to change these values before this hits release. Right now, the height will change as font-size changes (due to em units) but the width won't, which is slightly odd. Maybe we should just use a fixed pixel size? (and possibly something more square, like the mockup in attachment 437584 [details])  (Is there any spec guidance for this? do other browsers have intelligent behavior that we should match?)

Anyway -- we don't have to sort that out immediately, but we definitely should before long (before it hits beta), because it'll cause much more pain if we realize we need to change it after sites already depend on it.

r=me with the commit message, the blank-line removal, and with the sizing followup filed.

Comment 11

[Daniel Holbert [:dholbert]]

(RE the sizing followup: please also CC :shorlander, for visual-design input)

Comment 12

[Arnaud Bienner]

Attachment: Split button properties in form.css: v2

Updated version of the patch.
Marking it as r+ directly as it applies remarks from comment 8.

Comment 13

[Arnaud Bienner]

Attachment: Split button properties in forms.css: v3

Just removes the space before ":" in comments (this is the rule in French, but not in English IIRC... switching from a language to another is confusing sometimes :p)

Comment 14

[Arnaud Bienner]

Attachment: Add input color to form.css v2

r+ as I've applied remarks from comment 10.

I've used "min-height: 15px;" and "min-width: 40px;" (a bit less then before (50px), but I think 40px is enough anyway), so the inner element (the swatch) looks like the Chrome's one.
Opera has a smaller one, but IMHO it's too tiny, and the color chosen might hard to see.

I don't think a specific choice has been made in bug 547004. I will fill a follow-up bug as you suggested.

Comment 15

[Arnaud Bienner]

Attachment: Add input color to forms.css v3

Oops, I forgot to update reftests in the previous patch.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 822833 [details] [diff] [review]
Split button properties in forms.css: v3

>Bug 928891 - Split button properties in form.css (text vs non-text). r=dholbert

Commit message nit: s/form/forms/

Also, for future reference: on multi-patch bugs, it's nice to label patches "part 1", "part 2", etc. in the commit message.

No need to repost the patch, though -- I already fixed these things in my local copy of the patch that I intend to land later today, assuming the Try run[1] is green.

[1] https://tbpl.mozilla.org/?tree=Try&rev=91a57d956ddd

Comment 17

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/620841908404
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1dde8f2d2e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42cf7d24609d
https://hg.mozilla.org/mozilla-central/rev/fffe266c93bf

Comment 19

[Alice0775 White]

I was surprised that there was no automated test ;( 

Please back this out from Firedox29.0.1 before Mozilla release it.

Comment 20

[Arnaud Bienner]

(In reply to Alice0775 White from comment #19)
> I was surprised that there was no automated test ;( 

Indeed, me too :(
 
> Please back this out from Firedox29.0.1 before Mozilla release it.

Reverting the first patch only might work (not sure as the second one depends on the first), but reverting the two patches will break the new <input type="color"> (even worse than what the revert is supposed to fix).

Then we should also disable <input type=color>, but that sounds a bit harsh to me, as I'm not sure it's a very big issue (even if it's very unfortunate, like every regression...).

IMHO the fix is probably only one line of CSS: maybe better to uplift the fix for the next release if we manage to have one which is very simple, as I suspect we can?

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Alice0775 White from comment #19)
> I was surprised that there was no automated test ;( 

For this bug? This bug has tests, but they were added elsewhere.

> Please back this out from Firedox29.0.1 before Mozilla release it.

I think the scope of changes in 29.0.1 is already pretty fixed, and I doubt release drivers would want to add risk to it by introducing a fix for this bug.  (Feel free to correct me if I'm wrong, though.)

In any case, let's keep this discussion over on bug 1007278.

Comment 22

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> (In reply to Alice0775 White from comment #19)
> > I was surprised that there was no automated test ;( 
> 
> For this bug? This bug has tests, but they were added elsewhere.

What Alice meant if I'm right, is that there was no non-regression tests for the "button" CSS: while I believe we test <input type=color> correctly, we didn't noticed we broke another feature because of a lack of tests on this feature.

Comment 23

[Daniel Holbert [:dholbert]]

(Just noticed comment 17 has the completely wrong csets; sorry for that. Comment 18 has the right ones. Tagging comment 17 as "obsolete" in the hopes that that'll hide it by default.)