932066 - Disable forms.css' input[type="color"] styling for Metro (until we have a colorpicker backend)

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Bug 930277 is enabling <input type="color">, but keeps the pref off for platforms that don't have color picker dialogs implemented yet. (those being Android, B2G, Metro)

It also suppresses the form-control theme for these 2/3 of these platforms in forms.css, (so that we'll render <input type="color"> just like normal <input>), but we were unable to suppress the CSS for metro because we don't (AFAIK) have a symbol we can make the preprocessor check for. We tried %ifdef MOZ_METRO, but that doesn't work; that just means "does this build support metro".

Filing this bug on figuring out a workable way to suppress this styling for metro.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #20)
> 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 2

[Daniel Holbert [:dholbert]]

[comment 1 quotes a bit more info from bug 930277 comment 20]

Comment 3

[Daniel Holbert [:dholbert]]

[Transferring needinfo=ally from bug 930277 in case she knows of anything that would help here beyond the stuff in Comment 1.]

Comment 4

[Arnaud Bienner]

browser/metro/theme/content.css overrides lot of things, including styles for some input elements.

One solution would be to copy/paste the default "input"'s style from forms.css and to override it in Metro's content.css as input[type=color]. That would work as expected I think, but it means we will have to update these rules in content.css each time default input's style is modified in forms.css.
As a temporary work-around, this might be fine, but it doesn't sound like a long term solution.
So I think it depends on bug 895464 progress.

Comment 5

[Allison Naaktgeboren :ally]

talked over with dholbert, discussed with mbrubeck; let's go with Arnaud's solution. We haven't come up with anything better.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #4)
> As a temporary work-around, this might be fine, but it doesn't sound like a
> long term solution.

[That's OK; this bug, by definition, doesn't want a long-term solution. :) We just need a stopgap until bug 895464 can be fixed.]

Arnaud, did you want to take this?  (I recall you not having have Win8 available to test; if that's the case, I'm also happy to take this and try your proposed solution at some point soon.)

Comment 7

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Arnaud, did you want to take this?  (I recall you not having have Win8
> available to test; if that's the case, I'm also happy to take this and try
> your proposed solution at some point soon.)

Indeed I don't have Windows 8 so I will not be able to test the idea I proposed.
If you have computer running Windows 8 available, I would be glad if you can take care of this.

Comment 8

[Allison Naaktgeboren :ally]

dholbert, let me know if you need to borrow a spare windows machine.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks! I've got a laptop which has [rarely-booted-into] Win8 installed, which I can use.

Comment 10

[Daniel Holbert [:dholbert]]

dbaron suggests that we might be able to use a (newly-implemented) "are we in metro mode?" CSS media query to implement this, too.

(That media query addition probably would happen in a separate bug, which this bug would depend on.)

For reference, the list of currently-supported media queries are here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsMediaFeatures.cpp?rev=bfaaa5b35899#369

Comment 11

[Allison Naaktgeboren :ally]

I like dbaron's suggestion. Do you have a bug on file for this?

Comment 12

[Daniel Holbert [:dholbert]]

This is that bug. :)

Comment 13

[Daniel Holbert [:dholbert]]

Oh, sorry -- no, I hadn't yet filed an "are we metro" media-query bug yet, but I've now filed bug 936628 for that.

Comment 14

[Daniel Holbert [:dholbert]]

Also: Unfortunately, we can't use media queries to guard the input[type="color"] *selector*, like we're currently doing with %ifdefs. The media query has to wrap an entire style rule (or set of rules).

So i.e. the media query won't help us with exempting input[type="color"] from this rule, on Metro:
> 549 button,
> 550 %ifndef INPUT_TYPE_COLOR_UNSUPPORTED
> 551 input[type="color"],
> 552 %endif
> 553 input[type="reset"],
> 554 input[type="button"],
> 555 input[type="submit"] {
> 556   -moz-appearance: button;
>       [...etc...]
http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#549

I think we'll need to (temporarily) copypaste all of these style rules into input[type="color"]-specific rules, which we can then wrap in a media query for "not metro".  Then, down the line when we fix bug 895464, we can drop that media-query chunk and add input[type="color"] back into the list of the selectors here.

Comment 15

[Daniel Holbert [:dholbert]]

Though actually -- the ":-moz-system-metric" pseudo-class seems like it *would* let us style the input[type="color"] selector directly, without need for copypasting. e.g.:
> 102 .listheader-sortdirection[sortDirection="ascending"]:-moz-system-metric(windows-classic) {
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/listbox.css#102
...and...
> 37 toolbar[type="menubar"]:not([autohide="true"]):not(:-moz-lwtheme):-moz-system-metric(menubar-drag) {
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#37

Ideally, I can hopefully end up changing...
  input[type="color"],
...to...
  input[type="color"]:not(:-moz-system-metric(metro-mode)),
...in the code quoted in comment 14.

Comment 16

[Daniel Holbert [:dholbert]]

Actually, even better would be to *replace* the current 
  %ifndef INPUT_TYPE_COLOR_UNSUPPORTED
  input[type="color"],
  %endif
...with...
   input[type="color"]:-moz-system-metric(colorpicker-available),

That would be much cleaner, and allow us to enable this per-platform in a more straightforward, declarative way.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: fix v1 [stacks on top of patch in bug 936628, to provide color-picker-available]

This layers on top of bug 936628, and I'm pretty sure it should fix this. (Haven't tested it on Metro yet, though.)

Comment 18

[Daniel Holbert [:dholbert]]

Try run's looking good: https://tbpl.mozilla.org/?tree=Try&rev=d5eef3a52f78

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 829657 [details] [diff] [review]
fix v1 [stacks on top of patch in bug 936628, to provide color-picker-available]

From local testing, this seems to do the trick for Metro (and preserve existing behavior for other platforms, as shown by the Try run.

The patch just does the following:
 (a) Drop all the INPUT_TYPE_COLOR_UNSUPPORTED setting / ifndeffing (which we previously used to disable this style for b2g/android, which don't have color pickers yet)

 (b) Replaces all instances of input[type="color"] with input[type="color"]:-moz-system-metric(color-picker-available)

Comment 20

[Daniel Holbert [:dholbert]]

[Changing component to Layout: Form Controls, since this ended up being just a forms.css tweak, with a helper-bug.]

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 829657 [details] [diff] [review]
fix v1 [stacks on top of patch in bug 936628, to provide color-picker-available]

(Requested review from dbaron since he'd suggested the idea of using a media query here.  But he's traveling, so he might not get to it right away.  Arnaud, I'd be comfortable having you review this, if you're comfortable reviewing it.)

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: fix v2 [stacks on top of patch in bug 936628, to provide :-moz-system-metric(color-picker-available)]

Oops, forgot to fold in one final bit of this patch, for the ::-moz-color-swatch.

(It isn't strictly necessary to use a media query for that one, since the ::-moz-color-swatch element will only exist in environments that have the pref turned on.  Still, we might as well be consistent, particularly when it doesn't require any additional lines of code now that we're doing away with the %ifdef.)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 829693 [details] [diff] [review]
fix v2 [stacks on top of patch in bug 936628, to provide :-moz-system-metric(color-picker-available)]

Actually, I'll just have jimm review this, since he reviewed bug 936628, and he consented in IRC. :)

Comment 24

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed845769c327

Comment 25

[Ed Morley [:emorley]]

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

Comment 26

[Daniel Holbert [:dholbert]]

Note: Looks like bug 677302 is getting some traction, which would let us drop the media query and make this slightly cleaner.

(It probably doesn't matter for the purposes of this feature, though, since (hopefully) we'll have colorpicker backends available everywhere before too too long, and we can just make all the forms.css stuff unconditional.)