Closed Bug 928891 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: dholbert, Assigned: arnaud.bienner)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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).
(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)
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.)
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.
OS: Linux → All
Hardware: x86_64 → All
(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.
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
Assignee: nobody → arnaud.bienner
Status: NEW → ASSIGNED
Attachment #822550 - Flags: review?(dholbert)
Attached patch Add input color to form.css (obsolete) — Splinter Review
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.
Attachment #822564 - Flags: review?(dholbert)
Second patch pushed to try (on top of the first one of course):
https://tbpl.mozilla.org/?tree=Try&rev=f7dbf60e0cad
Blocks: 930277
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
Attachment #822550 - Flags: review?(dholbert) → review+
(Needs a commit message, too)
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.
Attachment #822564 - Flags: review?(dholbert) → review+
(RE the sizing followup: please also CC :shorlander, for visual-design input)
Updated version of the patch.
Marking it as r+ directly as it applies remarks from comment 8.
Attachment #822550 - Attachment is obsolete: true
Attachment #822831 - Flags: review+
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)
Attachment #822831 - Attachment is obsolete: true
Attachment #822833 - Flags: review+
Attached patch Add input color to form.css v2 (obsolete) — Splinter Review
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.
Attachment #822564 - Attachment is obsolete: true
Attachment #822835 - Flags: review+
Oops, I forgot to update reftests in the previous patch.
Attachment #822835 - Attachment is obsolete: true
Attachment #822836 - Flags: review+
Blocks: 931619
Blocks: 547004
Attachment #822833 - Attachment description: Split button properties in form.css: v3 → Split button properties in forms.css: v3
Attachment #822836 - Attachment description: Add input color to form.css v3 → Add input color to forms.css v3
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
https://hg.mozilla.org/mozilla-central/rev/42cf7d24609d
https://hg.mozilla.org/mozilla-central/rev/fffe266c93bf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 949403
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
Depends on: 1007278
I was surprised that there was no automated test ;( 

Please back this out from Firedox29.0.1 before Mozilla release it.
Flags: needinfo?(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?
Flags: needinfo?(arnaud.bienner)
(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.
(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.
(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.)
You need to log in before you can comment on or make changes to this bug.