930277 - Flip the pref to enable <input type=color> for Linux/Gtk, Windows and Mac OS

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Arnaud Bienner]

We are only missing a few color picker backend implementations for input type color.
But everything has been implemented for some platforms (namely Linux/Gtk, Windows and Mac OS).
Mobile, b2g and Windows metro haven't been implemented yet.

I suggest to turn on the dom.fomrs.color pref for platforms that are ready.

Comment 1

[Arnaud Bienner]

Attachment: flip_color_pref.patch

Daniel: I'm requesting a review from you as you were the last one involved on this. Feel free to request a review from someone else.

Comment 2

[Daniel Holbert [:dholbert]]

A few things that I think we need to consider here:

 (1) Before we're ready to pref this on (in a way that'll affect release builds), we definitely need to have a patch ready for bug 928891, so that it looks pretty.

 (2) For any platform where we're not preffing it on (e.g. b2g/mobile/metro in this patch), we'd need to disable (1)'s forms.css tweaks, so that we preserve the illusion that <input type=color> is unrecognized on those platforms.  I *think* we have the ability to do that using something like the existing "%ifdef XP_OS2" in forms.css, ugly as it is. (However, see (3) below).)

 (3) I'm hesitant to turn on a new form control (in a way that will end up enabling it in release builds, when the patch makes it through nightly/aurora/beta) if it's not ready for mobile/b2g/metro yet.  The metro impl is probably what worries me the most, since without a metro backend ready, we'll be offering an inconsistent set of form widgets depending on which mode you view a page in. (metro vs standard)  But really, Android/B2G are worrisome as well; given our current focus on mobile, I think we need to be careful not to ship features in a way that leaves those platforms straggling.  (And ideally we'd like to ship a consistent platform, regardless of OS.)

So: given the above, I'm probably not comfortable r+'ing this change given the current state of things.

I could possibly imagine landing a patch that wraps the all.js change inside of "#ifdef RELEASE_BUILD...else..." wrappers (to enable support in nightly/aurora, but prevent it from getting enabled in beta/release until we explicitly decide it's ready for that; see e.g. the patch in bug 902992). And then we'd need a followup bug for removing the RELEASE_BUILD conditional, which could land when it's actually ready to ship in release builds.  The followup bug would want to depend on the forms.css changes, and ideally it'd also depend on on having backends available for our other supported platforms.

(Mounir, if you have time, it'd be great to get your thoughts on this as well, since you have more experience with new form-control implementations than I do.)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 821345 [details] [diff] [review]
flip_color_pref.patch

Thanks for having bugs filed for the missing implementations, BTW! I was going to ask you to file those, but then I noticed they already existed. :)

One nit:
>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js
> /* new html5 forms */
> pref("dom.experimental_forms", true);
> pref("dom.forms.number", true);
>+// Don't enable <input type=color> yet as we don't have a color picker
>+// implemented for Android (bug 875751)
>+pref("dom.forms.color", false);

Bug number should be bug 875750 there (not 51)

Comment 4

[Daniel Holbert [:dholbert]]

[/me cancels needinfo, since we're not technically blocked on that feedback; just curious]

(In reply to Daniel Holbert [:dholbert] from comment #2)
>  (3) I'm hesitant to turn on a new form control (in a way that will end up
> enabling it in release builds, when the patch makes it through
> nightly/aurora/beta) if it's not ready for mobile/b2g/metro yet.

One minorly-complicating aspect to this: I don't think (from some very brief googling) we have any platform-provided color-picker dialog available on any of the missing platforms, so they'll be a little harder to finish than the GTK/Windows/Mac backends.

(However, I imagine we could write a simple, shared fallback UI with e.g. three <input type="range"> sliders for R,G,B and with a preview <div> whose background those sliders control.  We could then use that fallback UI on each of those platforms, with whatever platform-specific "dialog"-ish wiring we happen to need. [waving hands a bit])

Comment 5

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Bug number should be bug 875750 there (not 51)
Oops, I mixed the bug numbers :( should be 875750 here indeed, but 875751 in the b2g.js file.

Comment 6

[Arnaud Bienner]

I know some platforms are missing, and indeed more work is required to implement Android and Gonk color picker.
But it's a bit sad to have to wait to have the color pickers implemented everywhere before turning the pref on, as it's fully ready for some platforms.
And I don't know how long it will take before having color pickers for missing platforms.

I think Metro is indeed worrisome as you noticed.
But I don't think b2g/Android is such a blocking issue. After all, we already have some discrepancies e.g. input type date/time implemented for Android/b2g but not for desktop.

About having a nicer form.css for input type color (bug 928891), is this really mandatory? This input doesn't look that bad (IMHO only the missing "cursor:default" might be a bit confusing/not eye-candy for users; not sure the missing "-moz-appearance:button" really matters).
IIRC, Mounir told me it's not possible to have ifdef in form.css

Having a default fallback UI sounds like a good idea and might prevent us from being stuck here :) And the one your proposed sounds great and not very hard to implement.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #6)
> But it's a bit sad to have to wait to have the color pickers implemented
> everywhere before turning the pref on, as it's fully ready for some
> platforms.

I know :-/ We have to balance that against the badness of annoying web developers with an inconsistent feature-set across platforms, though. (It's frustrating to have an arbitrarily-different feature-set available on different platforms, for the same Firefox version.)

> But I don't think b2g/Android is such a blocking issue. After all, we
> already have some discrepancies e.g. input type date/time implemented for
> Android/b2g but not for desktop.

Hmm. I don't know about the date/time situation, but you do seem to be right about that. Do you know where that was decided?

> About having a nicer form.css for input type color (bug 928891), is this
> really mandatory? This input doesn't look that bad (IMHO only the missing
> "cursor:default" might be a bit confusing/not eye-candy for users; not sure
> the missing "-moz-appearance:button" really matters).

It's not just for aesthetics -- it's also sizing differences (which impact page layout, and which could break stuff if we let people use these and then make that change later on).

For example, the button family has "-moz-box-sizing: border-box" in forms.css (but plain 'input' and other textboxes don't have that) -- and that means that e.g. these render at different sizes:
>  data:text/html,<input style="height: 30px; width: 30px; border: 40px solid black">
>  data:text/html,<input style="height: 30px; width: 30px; border: 40px solid black" type="reset">

So if we add the button theming after-the-fact, we'll risk breaking sites by arbitrarily resizing their form controls.

> IIRC, Mounir told me it's not possible to have ifdef in form.css

Darn. (I'm willing to believe that. We probably should get rid of the XP_OS2 ifdefs, then, since they're unused)

If that's the case, then I think that's more support for the "only enable it when it's ready on all platforms" position, then, given my thoughts on forms.css above. (Though in a pinch, I imagine there must be another css file we could use... I'm not sure, though.)

> Having a default fallback UI sounds like a good idea and might prevent us
> from being stuck here :) And the one your proposed sounds great and not very
> hard to implement.

Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

OK, a few updates:

(In reply to Daniel Holbert [:dholbert] from comment #7)
> > IIRC, Mounir told me it's not possible to have ifdef in form.css
> 
> Darn. (I'm willing to believe that. We probably should get rid of the XP_OS2
> ifdefs, then, since they're unused)

dbaron actually says we almost certainly *can* use ifdefs in forms.css.

He says the '*' prefix in this file...
http://mxr.mozilla.org/mozilla-central/source/layout/style/jar.mn#12
...means "please preprocess this file", which means ifdefs should be honored. Note that ua.css (the other file with a star there) has "%ifdef XP_WIN" at line 161, which looks less likely to be cruft than the XP_OS2 stuff. :)

So that means we do have the option of adding platform-specific button styling for this element. We can make this somewhat elegant by using a helper-variable, e.g. something like:
  /* Set a variable to allow us to suppress <input type="color"> styling on
     platforms where it's not enabled yet, so that it renders like a normal
     input element. Note that this list should be exactly the list of
     platforms whose custom about:config pref manifest files contain a line
     like:
       pref("dom.forms.color", false);
  */
  %ifdef ANDROID
  %define INPUT_TYPE_COLOR_UNSUPPORTED
  %elifdef B2G
  %define INPUT_TYPE_COLOR_UNSUPPORTED
  %elifdef METRO
  %define INPUT_TYPE_COLOR_UNSUPPORTED
  %endif
  
  button,
  %ifndef INPUT_TYPE_COLOR_UNSUPPORTED
  input[type="color"],
  %endif
  input[type="reset"],
  input[type="button"],
  input[type="submit"] {
    /* button theming goes here */
  }

Also, Brad Lassey (a mobile platform guy, in charge of b2g/android platform stuff) says he'd be OK with going ahead and shipping this on the platforms that are ready, and then enabling it on Android/B2G/Metro when they're ready (hopefully soon).

So, given the above, I think I'd be OK with taking something like the attached patch, once forms.css is configured correctly.

Comment 9

[Daniel Holbert [:dholbert]]

(note that I'm suggesting that we use an "opt-out" variable ("UNSUPPORTED" rather than opt-in aka "SUPPORTED") just to match the "opt-out" nature of the pref-toggling in your attached patch.  This lets us more sanely keep the lists of opted-out platforms in sync.  If we happen to change the pref-toggling strategy to be "opt-in" for whatever reason, then we'd want to do the same in forms.css.)

(Also note that I'm not sure the variables (ANDROID, B2G, METRO) are exactly the right ones. :) Finding the right names may require some research.)

Anyway, all of this forms.css stuff belongs in bug 928891, though that patch probably needs to land simultaneously with this bug's patch.

Also: to sanity-check that we're preffing this on (and enabling styling) on exactly the platforms we want, I'd like for this bug to include a very simple reftest that verifies that the pref is enabled (before the "default-preferences" line), e.g. by asserting that <input type="color"> matches <button><div style="height:100%;width:100%; background:color">, with "fails-if()" annotations for the platforms we're expecting it to be disabled on.

(Those platforms will probably fail the other reftests, as well, since they'll be missing the (forthcoming) forms.css tweaks. That's probably fine, I think, as long as we annotate the failures in the manifest file and include an explanatory comment with bug numbers.)

Comment 10

[Daniel Holbert [:dholbert]]

(er, by "background:color" I of course meant "background:black")

Comment 11

[Arnaud Bienner]

Comment on attachment 821345 [details] [diff] [review]
flip_color_pref.patch

Review of attachment 821345 [details] [diff] [review]:
-----------------------------------------------------------------

Marking this patch as r-, as per comments above, it's not exactly what we would like to have.

Comment 12

[Arnaud Bienner]

Attachment: Flip input type color pref on, v2

This patch flip the pref on by default, and flip it back to "false" for B2G, Android and Metro which don't have a color picker yet.
It also disables input[type=color] styling in forms.css.

And reftests have been updated as well with "fails-if" + comments (which contain bug numbers).
The simple test you suggested already exists, so I've reused it: it was the first one. Now it is run before changing the pref, as you suggested.

Comment 13

[Arnaud Bienner]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c6fd4fb38bcc

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 822837 [details] [diff] [review]
Flip input type color pref on, v2

>+++ b/layout/style/forms.css
[...]
>+%ifdef ANDROID
>+%define  INPUT_TYPE_COLOR_UNSUPPORTED
>+%elifdef MOZ_WIDGET_GONK
>+%define  INPUT_TYPE_COLOR_UNSUPPORTED
>+%elifdef MOZ_METRO
>+%endif

The metro case is empty right now -- it needs a #define.

(Fortunately you don't need to worry about metro in the reftest.list file, because we don't run reftests on metro. (at least, not right now))

r=me with that

Comment 15

[Daniel Holbert [:dholbert]]

Oh, hmm... looks like in the Try run, the preprocessor doesn't like the %define line. Not sure what's going on there. We do a similar %define here, at least:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/downloads/downloads-aero.css#5
so it should be possible, I'd think.

But in any case, we need to sort that out before landing.  (You should be able to reproduce it locally by dropping the %ifdef ANDROID etc. and just making it an unconditional %define (for debugging purposes), I suspect.)

Comment 16

[Arnaud Bienner]

Yep, I just noticed :(
Indeed, we are doing the same in different places (e.g. xul.css), so I thought it would be OK.
I will have a deeper look.

Comment 17

[Arnaud Bienner]

There is 2 spaces between "%define" and the name, so it didn't match the regexp in http://mxr.mozilla.org/mozilla-central/source/config/Preprocessor.py#249
I did the "mistake" but, and as I copy paste this line for B2G, it didn't work here either.
Pushed to try to check it's OK: https://tbpl.mozilla.org/?tree=Try&rev=dede40460cff

Comment 18

[Arnaud Bienner]

Attachment: Flip input type color pref on, v3

New patch with only one space between %define and the name, and with %define line added for Metro.

Comment 19

[Daniel Holbert [:dholbert]]

Looks like the Try run is showing reftest failures, with Windows 7 & 8 rendering the button with the color swatch (so, the pref is enabled) but without the button look-and-feel (so, it looks like they're getting INPUT_TYPE_COLOR_UNSUPPORTED %defined in forms.css for some reason)

So I wonder if MOZ_METRO is defined (in the preprocessor) in the build that's running reftests. That's believable, but seems odd... Ally, is that supposed to be the case? (And if so, is there a better way to check for metro in preprocessor-processed JS / CSS?)

Comment 20

[Arnaud Bienner]

Discussing about this with jimm, it seems that MOZ_METRO means the Metro stuff is built: we have to check "XRE_GetWindowsEnvironment()" function to know if we are using Metro or not.
I think we can't check something like this in forms.css directly.

So a solution would be to define INPUT_TYPE_COLOR_UNSUPPORTED elsewhere, but this might start to become ugly :(
I don't know if it exists other solution.

I'm not cleaning the needinfo flag as ally might be able to provide more information.

Comment 21

[Arnaud Bienner]

Attachment: Flip input type color pref on, v4

Updated patch, without MOZ_METRO check.

Comment 22

[Daniel Holbert [:dholbert]]

(Per IRC conversation, it sounds like we're not planning on shipping the Metro UI to users quite yet (currently targeted for Firefox 28), so it's not too horrible to have the wrong fallback <input type="color"> UI there; hence the "without MOZ_METRO check" patch)

Patch looks good to me, modulo a comment-tweak in reftest.list. I went ahead and pushed to Try, with this and bug 928891's patches: https://tbpl.mozilla.org/?tree=Try&rev=91a57d956ddd

If that looks good, I'll land on inbound later today. We'll likely miss the aurora merge (I think that merge is about to happen, likely with an already-known-good commit from much earlier today), but it's feasible we can uplift this after the merge, if things are looking good.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 823498 [details] [diff] [review]
Flip input type color pref on, v4

Looks good! Just a few comment nits, which I already fixed myself (and they're fixed in the Try run) so no need for you to do anything. Just posting them here for your knowledge.

>+++ b/layout/reftests/forms/input/color/reftest.list
>@@ -1,7 +1,13 @@
>+# Simple test. Should fail on platforms where input type color isn't activated yet.
>+# Missing platforms are B2G (bug 875751), Android (bug 875750) and Metro (bug
>+# 895464).

This line is > 80 characters, and it'd also be nice to have "bug 895464" all on the same line, so it's not hard for people to copypaste into their URL bar).

I made these changes

>+# Even if the pref is turned on now, some styles from form.css are not applied
>+# for these platforms and the following tests will fail.

s/form/forms/
(I reworded the comment a bit to hopefully clarify what it's trying to say, too)

Comment 24

[Daniel Holbert [:dholbert]]

(Sorry for the straggling "I made these changes" - I meant to delete that after I added the first paragraph above)

Comment 25

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07c532718310

Comment 26

[Daniel Holbert [:dholbert]]

[I'll transfer the needinfo to Bug 932066 which now tracks the metro-specific forms.css disabling that we were wondering about]

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07c532718310

Comment 28

[Arnaud Bienner]

As we just missed the Aurora merge, we were discussing yesterday the possibility to uplift this (+ patches in bug 928891).

IIRC, we weren't sure if it's worth it, because this is no urgency. But on the other hand, it's sad to wait 6 weeks more to have this feature available to our users.

If I'm right, the pref was already turned on in Aurora (and Beta?) builds, so this gave us the chance to have some feedback about it, and no blocking problems were raised during this period. Moreover, some part of this feature are here since Firefox 24 (the content part; and Firefox 25 for most color pickers implementation). The layout was done more recently, but has been in Nightly for some time now. The only problem we might face is the default styling of the input element itself which may need to be refined (tracked by bug 931619).

Regarding all this, are we considering the uplift or not?

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #28)
> If I'm right, the pref was already turned on in Aurora (and Beta?) builds,
> so this gave us the chance to have some feedback about it, and no blocking
> problems were raised during this period.

The pref has *existed* on various builds for a time, but it's been turned off by default, so users weren't actually exercising the code paths. (and even if they found & turned on the pref, they still didn't have a usable form control until bug 875275 landed last week, IIUC)

> Moreover, some part of this feature
> are here since Firefox 24 (the content part; and Firefox 25 for most color
> pickers implementation).

That's true, but it doesn't really change the calculus here, from a "has this gotten nightly testing" perspective, since users weren't actually exercising the code.

> The only problem we might face is the default
> styling of the input element itself which may need to be refined (tracked by
> bug 931619).

I disagree; per above, this whole feature will now be getting its first real-world-user testing (and even then, only in cases when web content actually uses <input type="color">, which may not be particularly common.)

So, who knows what problems might arise. :)  (Hopefully none)

> Regarding all this, are we considering the uplift or not?

I'm leaning against it, because
 (a) there's no strong urgency (aside from the urgency of wanting to get your code out there, which I understand :)
 (b) per above, this hasn't gotten any actual real-world user-testing yet, and it would benefit from some baking on nightly to find problems. In contrast, if we uplifted, we'd be shipping it directly to Aurora users without any nightly testing at all, basically.

Comment 30

[Arnaud Bienner]

OK, let's play it safe so :)

Comment 31

[Arnaud Bienner]

One last question (I will stop bothering you after ;) this should probably go into the release notes (for the desktop release), don't you think? I remember someone put a "relnote-firefox" flag in bug 875274 for this purpose (it was useless at that time though).
Should we wait for this to reach Aurora? Or is it better to do this now before we forget?

Comment 32

[Daniel Holbert [:dholbert]]

Yes, very good point. Added keyword.

Comment 33

[Florian Bender]

I think the correct keyword is "feature" (according to https://groups.google.com/forum/#!topic/mozilla.dev.planning/_zHoo7kPIGQ). Please update, and also set the relnote? flag.

Comment 34

[Daniel Holbert [:dholbert]]

Ah, thanks. Done.

Comment 35

[u279076]

Arnaud, a few QA related questions:

1. How well is this covered by automation?
2. Do we know of any websites using this element to test?
3. What are the things we should look for in regression testing?
4. Does this even need manual QA attention?

Thanks

Comment 36

[Arnaud Bienner]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #35)
> 1. How well is this covered by automation?

Very well ;)
More precisely, content part (sanitization algorithm, etc.) is being tested mainly under content/html/content/test/forms/ (more details in the commit of the content part, which includes the test cases added: https://hg.mozilla.org/mozilla-central/rev/cb04a9a77e89)
Layout part has some reftests under layout/reftests/forms/input/color/

> 2. Do we know of any websites using this element to test?

Except demo websites, I don't have any particular in mind. I know www.skimlab.com used it some time ago, for the edit part, but I think they have fallback entirely to a javascript color picker.

> 3. What are the things we should look for in regression testing?
> 4. Does this even need manual QA attention?

The widgets don't have automatic testing. We use native widgets so we can assume they are working well. Only the interactions (getting the color selected by the user, calling the callback, etc.) might be buggy/broken and might need manual testing.

Hope it answers your questions :)
If not, don't hesitate to ask for clarifications.

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #35)
> 3. What are the things we should look for in regression testing?

If by "regression testing" you mean "regressions caused by this feature": there probably shouldn't be any; this is something that simply didn't used to be recognized (i.e. it'd just render as a normal <input> textbox), but now we'll recognize it and give it special rendering / functionality.

But (more likely) if you're asking what to watch for, to catch regressions in this feature -- the useful things to test would be:
 - visiting a site with input type="color" buttons like e.g. http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/forms/input/color/input-color-1.html
 - making sure that clicking the button pops up a color picker, and the button-color changes to whatever color is chosen
 - making sure that the button's "value" attribute (observable via e.g. right-click & "inspect element") changes to the hex representation of whatever color you chose.

(I don't think we have automated testing for any of that, since the color dialog is interactive. It'd be awesome if we could automate that, but I don't know how offhand.)

Comment 38

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #37)
>  - visiting a site with input type="color" buttons like e.g.
> http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/forms/
> input/color/input-color-1.html
>  - making sure that clicking the button pops up a color picker, and the
> button-color changes to whatever color is chosen
>  - making sure that the button's "value" attribute (observable via e.g.
> right-click & "inspect element") changes to the hex representation of
> whatever color you chose.

Yep, this should be enough. Maybe also having a onchange="console.log('Color changed')" or something like this to be sure the event is correctly fired?

Comment 39

[Paul Silaghi, QA [:pauly]]

Hello everyone,
I'm testing this in Nightly.

So, on http://diveintohtml5.info/examples/input-type-color.html if changing the button to another color and then pressing 'Go', will change the URL to http://diveintohtml5.info/examples/input-type-color.html?bg=%23ff0000, but the button will get reset back to dark. It's ok ?

Comment 40

[Arnaud Bienner]

(In reply to Paul Silaghi, QA [:pauly] from comment #39)
> the button will get reset back to dark. It's ok ?

Totally fine.

When submitting this form, I don't think the color's value sent is used in any way (including trying to reset the color to what the user selected previously by setting the "value" attribute of the color input).

Comment 41

[Catalin Varga [QA][:VarCat]]

Verified as fixed on:

Aurora FF 28.0a2
Build Id: 20140108004006

OS: Os X 10.8.5 , Ubuntu 13.04 x 64, Win 7 x64