943966 - Reducing width on input type color only reduces background

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Markus Popp]

Attachment: Shows how width only applies to background

If I set a width of lets say 20 pixels to an <input type="color"> element, the gray background gets smaller as specified. However, the colored foreground remains at the same width, as shown in the attached screenshot.

Comment 1

[Arnaud Bienner]

This comes from the fact that we have "min-width: 40px;" for input[type="color"]::-moz-color-swatch (the inner button).

We needed this before bug 928891 was implemented because otherwise, as the default "input" CSS rule applied, the input[type=color] was too small and so was the input[type="color"]::-moz-color-swatch.

Now we don't need this anymore.

IMHO we should decrease these min values (but keep them anyway because non visible input type color is useless) and add a new rule with default height and width for input[type="color"] in forms.css.

Daniel, do you agree with this? Or do you have another suggestion? 

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#459

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #1)
> IMHO we should decrease these min values (but keep them anyway because non
> visible input type color is useless) and add a new rule with default height
> and width for input[type="color"] in forms.css.

That sounds good to me.

For the swatch min-size, I'd suggest "min-width: 3px; min-height: 3px". That ensures it'll be big enough to show 1px of the selected color, with the default 1px of border on each side. (We have to account for the border in this size, because we have -moz-box-sizing:border-box)

Any minimum size above that would be a bit arbitrary. Sure, the button will be progressively harder to see as it approaches that size, but if authors really want that, we shouldn't prevent them.

Comment 3

[Arnaud Bienner]

Attachment: reduce_color_swatch_min_size.patch

Daniel, could you please review this patch?

One thing to notice: updated reftests work, but I noticed with the padding/margin test that changing this padding/margin now lead to very small swatch if we don't specify the height/width of the input type color.

I was surprised in the first place, but actually it makes sense, and if we decide to stay with a small min width for the color swatch.
Otherwise (the other way) we might want to have more decent values for min-width/height, and people who want smaller ones will have to override default rules of -moz-color-swatch.

I don't know what is best. Maybe the first (current one) as -moz-color-swatch isn't something really standard, so people will be less likely to update it than input[type=color].

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #3)
> One thing to notice: updated reftests work, but I noticed with the
> padding/margin test that changing this padding/margin now lead to very small
> swatch if we don't specify the height/width of the input type color.

Just to be sure I understand: so you're saying that, now that the button itself has a default height/width, setting a large margin or padding on the swatch will reduce the space available for the swatch.  Right?

(if so: makes sense.)

> I don't know what is best. Maybe the first (current one) as
> -moz-color-swatch isn't something really standard, so people will be less
> likely to update it than input[type=color].

I'm not sure I understand what you're saying here.

Comment 5

[Daniel Holbert [:dholbert]]

>+++ b/layout/style/forms.css
>+input[type="color"]:-moz-system-metric(color-picker-available) {
>+  width: 64px;
>+  height: 23px;
>+}

This is OK, but I'm realizing that the height of this form control will be (and already is) different from other buttons and inputs, by default.

forms.css has a comment in the "input {" selector that suggests we try to make buttons and textfields the same height. Ideally we should probably conform to that, but that doesn't have to happen here I suppose.
http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#52

Also, could you include a regression test for this bug in the patch? (a new test in the reftests directory, with a tiny height and/or width, with the reference case expecting that the color swatch also be tiny)

Comment 6

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Just to be sure I understand: so you're saying that, now that the button
> itself has a default height/width, setting a large margin or padding on the
> swatch will reduce the space available for the swatch.  Right?

Yes
 
> I'm not sure I understand what you're saying here.

Someone changing padding or margin of input type color (and only this, not the height/width) will end up with a small (too small) color swatch, because now min-width/height are very small. I just suggested we might want to have greater value for min-width/height than 3px. It would be possible for authors to override this anyway by setting rules for -moz-color-swatch (so we do not prevent authors to change this). However, authors might not figure out that there is a -moz-color-swatch they can override (so probably better to keep 3px as min-width/height).

But well, nevermind: I was just thinking about what can be the best default behavior.

(In reply to Daniel Holbert [:dholbert] from comment #5)
> This is OK, but I'm realizing that the height of this form control will be
> (and already is) different from other buttons and inputs, by default.
 
Indeed. Do you want this to be part of this patch?

> Also, could you include a regression test for this bug in the patch? (a new
> test in the reftests directory, with a tiny height and/or width, with the
> reference case expecting that the color swatch also be tiny)

OK: I will update my patch with this.

Comment 7

[Daniel Holbert [:dholbert]]

> > This is OK, but I'm realizing that the height of this form control will be
> > (and already is) different from other buttons and inputs, by default.
>  
> Indeed. Do you want this to be part of this patch?

No strong preference. This patch is a step in the right direction and fixes the bug reported here, so (with a regression test) I'm OK with it landing as-is, as long as there's a followup tracking the same-height-as-other-form-controls issue.

Comment 8

[Arnaud Bienner]

Attachment: reduce_color_swatch_min_size v2

Updated version of the patch: test case added.
Daniel, could you please review it if you think it's necessary? (except the new small test, nothing changed).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8340513 [details] [diff] [review]
reduce_color_swatch_min_size v2

>+++ b/layout/reftests/forms/input/color/custom-style-2-ref.html
>@@ -0,0 +1,20 @@
>+<!DOCTYPE html>
>+<html>
>+  <head>
>+    <title>Test for bug 943966</title>
>+	<link rel='stylesheet' type='text/css' href='reference-style.css'>

Nit: Please don't mix tabs and spaces. (That last line has a tab; the one before has spaces.)

Mozilla code uses spaces instead of tabs, generally, since they display the same in everyone's editor.

Please replace the tab characters in this testcase & reference case with spaces.

>+    </button>
>+  </body>
>+</html>
>\ No newline at end of file

^
Please add a newline at the end of both of the reftest files, too.

r=me with that. Thanks!

Comment 10

[Arnaud Bienner]

Attachment: reduce_color_swatch_min_size v3

Updated version of the patch with Daniel's comments applied.

Comment 11

[Arnaud Bienner]

Thanks Daniel for the review :)
I know about tabs/spaces (and agree mixing them is ugly :/). I didn't use the same editor as usual and just didn't notice that.
Thanks for pointing this out!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea19685002cc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea19685002cc

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Sounds like we should file a new bug for whatever followup is required.

Arnaud, can you do that?

Comment 15

[Arnaud Bienner]

(In reply to :Gavin Sharp (email [e-mail]) from comment #14)
> Sounds like we should file a new bug for whatever followup is required.
> 
> Arnaud, can you do that?

I've opened bug 949403.
Let's close this bug.