931619 - Review/improve input[type=color] sizing (in forms.css)

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Arnaud Bienner]

As mentioned in bug 928891, we need to discuss if the current default styling we have for input type color is OK or, otherwise, how it can be improved.

Currently, we have a button with a colored rectangle inside.
Dimensions are similar to what Chrome has, except for the border of the outer div.
Also, the inner div doesn't have borders currently, while Chrome's one has a small one. I don't know if this is useful or not.
Opera's swatch doesn't have borders, and the inner div is quite small by default which makes the color harder to see IMHO.

Comment 1

[Arnaud Bienner]

Adding Daniel to CC as he suggested to open this bug, and Stephen as suggested by Daniel, for visual design input.

Comment 2

[Arnaud Bienner]

Stephen, what do you think about the current visual of input type color?
You can try it on http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/forms/input/color/input-color-1.html

Comment 3

[Stephen Horlander [:shorlander]]

(In reply to Arnaud Bienner from comment #2)
> Stephen, what do you think about the current visual of input type color?
> You can try it on
> http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/forms/
> input/color/input-color-1.html

In the context of the rest of the input controls I think it's fine as is.

Comment 4

[Arnaud Bienner]

Daniel, do you think we should involve someone else?
If not, I guess we can close this bug: we might open other specific bugs depending on feedback we will get.

One thing though, I'm wondering if it wouldn't be nice to add a tiny border to the color swatch, like Chrome does: IMHO it makes the color easier to see, as it isolates it from the button.

Something like:
input[type=color]::-moz-color-swatch {
border-style:solid;
border-width:1px;
border-color: grey;
}

What do you think?

Comment 5

[Daniel Holbert [:dholbert]]

That sounds reasonable, though note that we'll also need "-moz-box-sizing: border-box", or else this would end up giving us a border-box that sticks out of the space we're supposed to occupy, since we have width and height set to 100% [1] and normally the border will add on top of that, unless you set box-sizing to border-box.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=2721917a1b53&mark=463-464#461

Comment 6

[Daniel Holbert [:dholbert]]

(And no, I don't think we need to bother asking anyone else about this; shorlander's visual design feedback is sufficient, I think. :) )

Comment 7

[Florian Bender]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> That sounds reasonable, though note that we'll also need "-moz-box-sizing:
> border-box"

How about just using outline?

Comment 8

[Daniel Holbert [:dholbert]]

Outline doesn't help; we'd still paint a color-swatch-box (including the border/outline/whatever) whose width and height are both 2px + 100%-of-the-available-space, which is too big.

Comment 9

[Florian Bender]

Well, I just tested it in Fx, and outline does not influence the box size at all (and that's why I mentioned it). So it should be 100% and no extra for outline vs. 100% + 2px for border.

Comment 10

[Daniel Holbert [:dholbert]]

Those outline pixels have to come from somewhere; they're not free.  Try making the outline 5px wide, and you'll see what I mean about it making the swatch "too big" -- it ends up stomping on all of the button's padding.

Unless we use "box-sizing", I'm pretty sure we'll end up with a swatch that has a content-box that occupies 100% of the available width and height, and a border (or an outline) that takes up additional space beyond that.

Comment 11

[Arnaud Bienner]

Attachment: Add a tiny border to the color swatch

Daniel, could you please review this patch?

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 828745 [details] [diff] [review]
Add a tiny border to the color swatch

>+  -moz-box-sizing: border-box;
>+  border-style: solid;
>+  border-width: 1px;
>+  border-color: grey;

Condense those last 3 lines to:
 "border: 1px solid grey;"

r=me with that, but I have one concern that I'd like to track in a followup -- I suspect it might not be wise to hardcode "grey" here.

Suppose e.g. someone's system theme happens to have exactly grey-colored buttons -- then this border will be imperceptible.  Or (worse) what if their theme has a color-scheme such that this particular "grey" looks gross (e.g. maybe their buttons are *almost* grey-colored, but not quite, and the distinction between the two adjacent colors is jarring.)

I suspect we should be using a color that derives from the system theme, like e.g. "-moz-FieldText" or "InactiveBorder" (probably not exactly either of those ones; those are just examples of these system colors).

It looks like most of these system colors are listed here (with sample values):
 http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_platform_colors.xul#27

Could you file a followup on figuring out a better color than "grey" here?

Comment 13

[Florian Bender]

I think you may want to add the unprefixed box-sizing, just to make sure it does not break when the prefixed one gets removed. 

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Those outline pixels have to come from somewhere; they're not free.  Try
> making the outline 5px wide, and you'll see what I mean about it making the
> swatch "too big" -- it ends up stomping on all of the button's padding.
TBH, I did not test it on the shadow tree itself. But well, we have box-sizing and that works.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Florian Bender from comment #13)
> I think you may want to add the unprefixed box-sizing, just to make sure it
> does not break when the prefixed one gets removed.

That's not necessary. The patch (on bug 243412 or followups) that actually removes support for the prefixed version of that property should absolutely include a search-and-replace across our source tree to switch to the unprefixed property.  (Note that the earlier patches there assume that this is the case -- see last chunk of bug 243412 comment 87.)

So, just use "-moz-box-sizing"; don't include both.

Also: we need to adjust the reftests' "reference-style.css" file to add a border there, too, right?

Comment 15

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Could you file a followup on figuring out a better color than "grey" here?

Good point.
However, I'm not sure about this. My idea was to have a grey border mainly to differentiate the color swatch from the rest of the button, and I wanted a light color, so we can see the chosen color well. As the color inside the color swatch has nothing to do with the user's theme, it makes sense to have a hard-coded color IMHO.
FYI, it seems that Webkit uses a hard-coded color for the border too.
If you're still not convinced, I will open a follow-up bug as you proposed, so we can discuss this in this bug.

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Also: we need to adjust the reftests' "reference-style.css" file to add a
> border there, too, right?

Ah, yes you're right, I forgot this: I will update this in my next patch.

Comment 16

[Daniel Holbert [:dholbert]]

I'm not convinced that a hard-coded color is right for the swatch-border (as opposed to a system-theme-defined color) but I also don't feel strongly enough about it to care very much, so given that you disagree, I'm fine just not worrying about it. :)

Comment 17

[Arnaud Bienner]

Attachment: Add a tiny border to the color swatch v2

Here is an updated version of the patch, with reftests updated.

Daniel: I've put r=you as this patch includes the changes you wanted in comment 12. Don't hesitate to let me know if this is not OK or you think something should be changed in this patch.

Comment 18

[Daniel Holbert [:dholbert]]

Looks good to me!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10c1f9be1b44

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/10c1f9be1b44