1333482 - [css-ui] Implement 'appearance:auto | none', with '-webkit-appearance' as an alias.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Mats Palmgren (inactive)]

I've been investigating the 'appearance' property and it seems to me
we can implement what the spec says and unship -moz-appearance.

Edge is implementing -webkit-appearance:none and no other values, so I'm
assuming that's web-compatible.  I think we should follow their lead and
implement 'appearance:auto | none' as the spec says.  So the plan is:

1. implement a new property 'appearance' with values 'auto | none'
2. mark -moz-appearance as UA-sheet-and-Chrome-only
3. add an accessor function for getting the used value for layout code
   to use.  It will return 'none' for 'appearance:none' and the value of
   '-moz-appearance' otherwise.
4. (optional) add '-webkit-appearance' as an alias for 'appearance'
   if needed for web-compat.

https://drafts.csswg.org/css-ui-4/#appearance-switching

Comment 1

[Mats Palmgren (inactive)]

Would it be possible to figure out if 4. is needed or not?
IOW, does web authors generally always include 'appearance:none' in the same rule
as '-webkit-appearance:none' etc?  Is there a database of "web CSS rules" somewhere
that one could query for that sort of thing?

Comment 2

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c419b608f7b8a0f6844a1601c2f54dbaf54a10

Comment 3

[Mike Taylor [:miketaylr]]

(In reply to Mats Palmgren (:mats) from comment #1)
> Would it be possible to figure out if 4. is needed or not?
> IOW, does web authors generally always include 'appearance:none' in the same
> rule
> as '-webkit-appearance:none' etc?  Is there a database of "web CSS rules"
> somewhere
> that one could query for that sort of thing?

Experience shows that there's a lot of -webkit-prefixed CSS that does not include unprefixed properties, so yeah, 4 is probably needed.

A quick GitHub search confirms that as well: https://github.com/search?l=CSS&q=webkit-appearance+none&ref=searchresults&type=Code&utf8=%E2%9C%93

There's some interesting research and usage @ https://github.com/whatwg/compat/issues/6 as well.

Comment 4

[Mats Palmgren (inactive)]

I looked through the first few pages of the first link above and it shows that
most uses are actually for the misspelled property "webkit-appearance: none"
which is rejected in Chrome at least.   Some of those rules have "border:none"
though, which will trigger non-themed styling.

But yes, if I search for "-webkit-appearance none" then it shows that most
rules only have that property, but maybe some of these files (the ones github
marks it as Sass, Less, Stylus, SCSS etc) are post-processed so that the final
style sheet will have an unprefixed copy too?
https://github.com/search?p=2&q=-webkit-appearance+none&ref=searchresults&type=Code&utf8=%E2%9C%93

Comment 5

[Mike Taylor [:miketaylr]]

(my bad on the typo link)

Yeah, I wouldn't worry about post/pre-processed CSS stuff. Those tend to be fine. I'd say the most common CSS prefixing patterns are the following (looking only at first 10 pages of just the CSS results, so take with a grain of salt...):

https://github.com/search?l=CSS&q=-webkit-appearance+none&ref=searchresults&type=Code&utf8=%E2%9C%93



29 instances of -webkit-appearance only.
-webkit-foo

31 instances of -webkit- + -moz-, no unprefixed
-webkit-foo
-moz-foo

37 instances instances of -webkit- + unprefixed, optionally with -moz
-webkit-foo
(-moz-foo)
foo

If you waive a magic wand and pretend that sample is representative of the web, that's a lot of CSS w/o an unprefixed appearance.

Comment 6

[Mats Palmgren (inactive)]

Yeah, I think you're right.  OK, I'll add a -webkit- prefixed alias, which means we
probably need bug 605985 too.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-ui] Introduce the 'appearance: auto | none' property.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-ui] Change all consumers of StyleDisplay::mAppearance to use the accessor UsedAppearance() instead, and make mAppearance/mMozAppearance private.

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-ui] Amend all uses of '-moz-appearance:none' in tests to also specify 'appearance:none' (automated change).

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-ui] Manually tweak some tests for 'appearance' changes.

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-ui] Amend DevTools themes with 'appearance:none' where needed so that they'll work with the pref on/off.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 8 - [css-ui] Introduce '-webkit-appearance' as an alias for 'appearance' using the same pref.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 9 - [css-ui] Regenerate the DevTools properties database.

Comment 16

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8534c19831922ef926c21026a052c2d1f89b11d2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=018525f1748789b0f92034defdb47154a47730e4

Comment 17

[Mats Palmgren (inactive)]

I'll send an intent-to-ship to dev.platform in a moment...

Comment 18

[Mats Palmgren (inactive)]

Intent to implement and ship sent:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/oZ9cPF8Y1pE

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8835964 [details] [diff] [review]
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.

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

r=me with nits addressed:

::: layout/style/nsCSSPropList.h
@@ +475,5 @@
> +    appearance,
> +    Appearance,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME,
> +    "",

Nit: The CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME flag doesn't make sense in this patch, since there's no pref here (it's enabled everywhere at this point)

You probably want to leave that flag out here, and add it in part 3 (which is where you [correctly] add it for -moz-appearance, incidentally).

::: layout/style/nsStyleStruct.h
@@ +2850,5 @@
> +    if (mAppearance == NS_THEME_NONE) {
> +      return NS_THEME_NONE;
> +    }
> +    MOZ_ASSERT(mAppearance == NS_THEME_AUTO);
> +    return mMozAppearance;

This is kinda subtle (particularly due to how easy it is to assume "-moz-foo" is an alias for "foo").  An explanatory comment here would be nice.

I think you're preferentially honoring "appearance" when it's at its non-default value -- but when it's at its default, we return one of the broader spectrum of values that are encoded in moz-appearance, so that layout/painting can take them into consideration. Right?  Anyway, some comment to that effect would be great.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8835967 [details] [diff] [review]
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.

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

r=me

::: layout/style/nsCSSPropList.h
@@ +476,5 @@
>      appearance,
>      Appearance,
>      CSS_PROPERTY_PARSE_VALUE |
>          CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME,
> +    "layout.css.appearance.enabled",

(As noted for part 1, you should probably add the CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME flag in this patch rather than in part 1 -- i.e. add it for both properties at once, atomically with adding their pref.)

::: layout/style/test/property_database.js
@@ +7788,5 @@
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "radio", "menulist", "button", "checkbox", "textfield",
> +                    "textfield-multiline", "meterbar", "progressbar", "range", 
> +                    "range-thumb", "spinner-upbutton", "spinner-downbutton", 

stray space at end of the last 2 lines here.

::: modules/libpref/init/all.js
@@ +2624,5 @@
>  
>  // Is support for CSS display:flow-root enabled?
>  pref("layout.css.display-flow-root.enabled", true);
>  
> +// Is support for CSS [-moz-]appearance enabled?

Perhaps:
 s/enabled/enabled for web content/

(since, unlike most properties, we intend to keep these ones on for chrome CSS regardless of the pref-setting)

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8835968 [details] [diff] [review]
part 4 - [css-ui] Amend all uses of '-moz-appearance:none' in tests to also specify 'appearance:none' (automated change).

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

Could you explain the strategy/idea behind this part & part 5?  They don't entirely make sense to me.

Questions, in particular:
 - Right now, this part (part 4) is upgrading all usages of "-moz-appearance:none" to *also* mention "appearance:none".  But why not just do a direct replacement? (Why keep around the old prefixed styles?)  IIUC, the prefixed version will become non-functional as part of this patch-stack (because it will be preffed off by default). So I'm not clear why you're leaving it there.  Maybe you're wanting to keep these tests working in case we need to revert this change via a pref-tweak? If so, please file a followup on eventually cleaning up the redundant CSS (with an automated patch) after we're sure that "appearance" is sticking.


 - Right now, part 5 is preffing *on* moz-appearance, and preffing *off* appearance, for a bunch of other tests which do things like "-moz-appearance:button".  Do we really need to tweak both prefs? I'd expect that in most/all cases, it'd be sufficient to just enable the -moz-appearance pref. There's no need to care about the state of the "appearance" pref, right? (since it's not used in those tests)

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8835970 [details] [diff] [review]
part 6 - [css-ui] Manually tweak some tests for 'appearance' changes.

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

r=me on part 6, one nit:

::: devtools/client/responsive.html/index.css
@@ +78,5 @@
>  }
>  
>  select {
>    -moz-appearance: none;
> +       appearance: none;

Looks like this is tweaking real browser UI, not a test. So, please move this to part 7 ("Amend DevTools themes"), unless I'm misunderstanding.)

(This also seems like a place where we should drop the prefixed version in a followup -- maybe the same followup suggested above -- as soon as we're sure the unprefixed version has stuck.)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8835971 [details] [diff] [review]
part 7 - [css-ui] Amend DevTools themes with 'appearance:none' where needed so that they'll work with the pref on/off.

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

r=me on part 7 (though it'll probably want to poach one change from part 6, as noted in previous comment)

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8835974 [details] [diff] [review]
part 8 - [css-ui] Introduce '-webkit-appearance' as an alias for 'appearance' using the same pref.

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

r=me on part 8. one observation (no action needed):

::: layout/style/nsCSSPropAliasList.h
@@ +262,5 @@
>  
> +CSS_PROP_ALIAS(-webkit-appearance,
> +               appearance,
> +               WebkitAppearance,
> +               "layout.css.appearance.enabled") // same pref as for 'appearance'

Observation: Ideally, we'd have this controlled by WEBKIT_PREFIX_PREF ("layout.css.prefixes.webkit"), like all the other webkit prefixed aliases in this file. I guess we don't have the ability to have properties controlled by multiple prefs, though. (But if we get rid of the appearance.enabled pref in the near term, though, we should probably just swap in the layout.css.prefixes.webkit pref here.  I suspect it might be valuable to keep the webkit pref around for a while longer, to assist with diagnosing site issues, so it'd be nice to make it control this alias too, when that's possible.)

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8835976 [details] [diff] [review]
part 9 - [css-ui] Regenerate the DevTools properties database.

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

r=me on part 9

Comment 26

[Daniel Holbert [:dholbert]]

(needinfo for questions about parts 4 and 5, in comment 21. I think I answered my first question (on part 4), as long as we're planning on removing all of the -moz versions there once this has stuck. But I'm not clear on the part 5 question -- why we need to bother disabling the no-prefix appearance pref for all those other tests.)

Comment 27

[Mats Palmgren (inactive)]

Right, for part 4, this is just a temporary addition so that the tests
pass also if we need to revert the pref settings for some reason;
to avoid causing churn in that case.  The intention is to remove
-moz-appearance from these tests after we've shipped this and are
confident we don't need to revert it.

Part 5 OTOH, are tests that need to set '-moz-appearance' in some way
that cannot be done with 'appearance'.  These tests would either fail
or not test what they were supposed to test without enabling that pref.
This change is intended to stay even after we remove the 'appearance'
pref.  I suspect we need to keep the '-moz-appearance' pref forever,
unless we want to convert these tests to use chrome sheets somehow
(which doesn't seem worth it).

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #27)
> Part 5 OTOH, are tests that need to set '-moz-appearance' in some way
> that cannot be done with 'appearance'.  These tests would either fail
> or not test what they were supposed to test without enabling that pref.

Right -- I'm not asking about the *enabling* pref. I'm asking about the additional *disabling* of the unprefixed "appearance" pref.  The second "pref()" line here, not the first one:
 pref(layout.css.moz-appearance.enabled,true) pref(layout.css.appearance.enabled,false) load 593526.html

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8835969 [details] [diff] [review]
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

>Bug 1333482 part 5 - [css-ui] Enable '-moz-appearance' support for some tests.  r=dholbert
[...]
>+++ b/gfx/tests/crashtests/crashtests.list
>@@ -81,20 +81,20 @@ load 532726-1.html
[...]
>-load 593526.html
>-load 593526.xul
>+pref(layout.css.moz-appearance.enabled,true) pref(layout.css.appearance.enabled,false) load 593526.html
>+pref(layout.css.moz-appearance.enabled,true) pref(layout.css.appearance.enabled,false) load 593526.xul

e.g. can't these just be:
pref(layout.css.moz-appearance.enabled,true) load 593526.html
pref(layout.css.moz-appearance.enabled,true) load 593526.xul

?

Comment 30

[Mats Palmgren (inactive)]

I guess so, sure.

Comment 31

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

Comment 32

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

(missed one)

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8835968 [details] [diff] [review]
part 4 - [css-ui] Amend all uses of '-moz-appearance:none' in tests to also specify 'appearance:none' (automated change).

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

r=me on part 4

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8837441 [details] [diff] [review]
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

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

r=me on part 5

Comment 35

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/moz-appearance-property-has-been-removed/

Comment 36

[Mats Palmgren (inactive)]

(In reply to Kohei Yoshino [:kohei] from comment #35)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-CA/docs/2017/moz-appearance-property-has-
> been-removed/

Thanks Kohei-san, that's a good idea.  We should probably also point out that
style sheet files loaded with a chrome: URL can continue to use -moz-appearance
as before, so there's no need to change those.  But, -moz-appearance style that
are inline in XUL, XML, JS files etc will now be ignored.  The recommendation is
to update those, like so: "-moz-appearance: none; appearance: none", or move that
style to a separate style sheet file if using values other than 'none' are needed.

Comment 37

[Mats Palmgren (inactive)]

The latest Try run shows that this causes a fatal assertion for stylo builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ed2e5ea888c841765683d33a7cfc5130591a43
Do you want me to comment out that assertion for now? (or demote it to a non-fatal one?)
Or can you give me a quick instruction on how to add "appearance: none | auto" to
the stylo backend?  (assuming it's super-easy and can be done in < 30 minutes)

Comment 38

[Mats Palmgren (inactive)]

Jorge, is there anything else we can do to announce this change to addon devs,
or help them in some way?

Comment 39

[Mats Palmgren (inactive)]

Boris, it looks to me as inline (X)HTML <style> elements in addon XUL files also
counts as chrome sheets, so would be unaffected by this change.  Can you confirm
this observation?  (it might affect the advice we give to addon devs)
So it's really just style attributes that are affected, right?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Inline <style> elements in a chrome://-URL document would count as a "chrome sheet", yes.

Comment 41

[Jorge Villalobos [:jorgev] (he/him)]

Best way to help is not break things until 57 :). When that's not possible, setting the addon-compat flag and the milestone is enough for us to include it in the add-on developer comms, so we're good here.

Comment 42

[Kohei Yoshino [:kohei]]

(In reply to Mats Palmgren (:mats) from comment #36)
> (In reply to Kohei Yoshino [:kohei] from comment #35)
> > Posted the site compatibility doc:
> > https://www.fxsitecompat.com/en-CA/docs/2017/moz-appearance-property-has-
> > been-removed/
> 
> Thanks Kohei-san, that's a good idea.  We should probably also point out that
> style sheet files loaded with a chrome: URL can continue to use
> -moz-appearance
> as before, so there's no need to change those.

I'm aware of that, but it's a site compatibility note for web developers, so I guess the title and description are okay.

Comment 43

[Bobby Holley (:bholley)]

(In reply to Mats Palmgren (:mats) from comment #37)
> The latest Try run shows that this causes a fatal assertion for stylo builds:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=59ed2e5ea888c841765683d33a7cfc5130591a43
> Do you want me to comment out that assertion for now? (or demote it to a
> non-fatal one?)
> Or can you give me a quick instruction on how to add "appearance: none |
> auto" to
> the stylo backend?  (assuming it's super-easy and can be done in < 30
> minutes)

Manish, is this a thirty minute task?

Comment 44

[Manish Goregaokar [:manishearth]]

Yep, it's quick.

You'll need to change -moz-appearance[1] to target mMozAppearance, and add another call to helpers.single_keyword for appearance with just auto/none. Also mark -moz-appearance as `internal="True"` so that it only works in UA sheets.



 [1]: https://github.com/servo/servo/blob/05623b36a15b594bbc690fcd8e3b642995618de1/components/style/properties/longhand/box.mako.rs#L1894

Comment 45

[Bobby Holley (:bholley)]

Also, feel free to just write it as a patch against mozilla-central and needinfo a stylo dev to take care of landing it into servo/. If it needs a coordinated landing (i.e. neither side of the change is correct without the other), we will need to time the landings.

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8835967 [details] [diff] [review]
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.

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

::: dom/ipc/ContentPrefs.cpp
@@ +96,5 @@
>    "javascript.options.werror",
>    "javascript.use_us_english_locale",
>    "jsloader.reuseGlobal",
>    "layout.css.all-shorthand.enabled",
> +  "layout.css.appearance.enabled",

The modifications in ContentPrefs.cpp need DOM Peer review now, according to a disclaimer that was recently added to the top of this file:
https://hg.mozilla.org/mozilla-central/annotate/e1135c6fdc9bcd80d38f7285b269e030716dcb72/dom/ipc/ContentPrefs.cpp#l10

Based on a discussion with billm/bz in #developers just now, IIUC this file only needs to care about properties that are used in UA Stylesheets (or that otherwise get used "early").  Clearly "appearance" & "-moz-appearance" are both in this category, so your changes here make sense, I think.

I'm tagging billm for additional-review on this part, anyway, to satisfy the new DOM peer review requirement.

Comment 47

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs. Enable both by default.

:bz was concerned about web compat risk and suggested that we ship all
three properties initially, and then disable -moz-appearance in a future
release:

https://groups.google.com/d/msg/mozilla.dev.platform/oZ9cPF8Y1pE/RL9sx-RyBwAJ
"We should ship the no-prefix version and the -webkit version.  Then we should get people to switch to them.  Then we can remove -moz-appearance."

That sounds reasonable to me, so this patch does that.

(Also, it appears all the layout.* prefs are no longer present in
dom/ipc/ContentPrefs.cpp so I'm no longer them there.  I guess
that's unnecessary now after bug 1343677?  Seems to work without it...)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e235ced98e514436cdfbf4784afbde140d068d

Comment 48

[Daniel Holbert [:dholbert]]

Comment on attachment 8850124 [details] [diff] [review]
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.  Enable both by default.

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

r=me

(And yeah, I think we don't need the ContentPrefs.cpp changes anymore.)

Comment 49

[Mats Palmgren (inactive)]

Attachment: part 10 - [css-ui] Add 'appearance' property to Stylo (with '-webkit-appearance' alias).

Here's my feeble attempt at making it work for stylo, but there's still
a (stylo-only) error that I don't understand:
TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'number-input' on '-moz-appearance' - didn't expect "", but got it
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e235ced98e514436cdfbf4784afbde140d068d&selectedJob=85129623

Comment 50

[Manish Goregaokar [:manishearth]]

Comment on attachment 8850130 [details] [diff] [review]
part 10 - [css-ui] Add 'appearance' property to Stylo (with '-webkit-appearance' alias).

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

note that the servo stuff will have to be landed as a PR to servo, autoland can't handle it yet

::: servo/components/style/properties/longhand/box.mako.rs
@@ +1882,5 @@
>      }
>  </%helpers:longhand>
>  
> +${helpers.single_keyword("appearance",
> +                         """auto none""",

no need for a docstring

@@ +1918,2 @@
>                           gecko_constant_prefix="NS_THEME",
>                           products="gecko",

internal="True" as well

Comment 51

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #49)
> Here's my feeble attempt at making it work for stylo, but there's still
> a (stylo-only) error that I don't understand:
> TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting
> 'number-input' on '-moz-appearance' - didn't expect "", but got it
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e235ced98e514436cdfbf4784afbde140d068d&selectedJob=85129623

Maybe "number-input" was simply missed in servo/stylo when -moz-appearance parsing support was added there?

Current mozilla-central doesn't really test -moz-appearance parsing very thoroughly (and doesn't test for "number-input" support) -- we only start having good tests for it as of the property_database.js changes in your "part 3" patch here (specifically the new entries in "other_values").

I'll bet we would hit that same stylo failure on current trunk with just your enhanced -moz-appearance tests.  If you can verify that that's the case, I'd say we should just spin off a followup bug for that, and comment out /*number-input*/ in your new property_database.js lines in part 3 here, so we can get this landed without gating it on probably-independently-broken stuff.

Comment 52

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #51)
> Maybe "number-input" was simply missed in servo/stylo when -moz-appearance
> parsing support was added there?

(I don't really know how stylo CSS parsing works, but I'm guessing that "number-input" needs to be listed here, for that value to be supported in -moz-appearance.  Other valid values are listed there, at least.
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/servo/components/style/properties/longhand/box.mako.rs#1933-1953
)

Comment 53

[Mats Palmgren (inactive)]

Right, it seems it's missing here:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/servo/components/style/properties/longhand/box.mako.rs#1938

I added it, let's see what breaks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cccd6de04dc85baff91a279533082a791fe4f2a

Comment 54

[Boris Zbarsky [:bzbarsky]]

> and comment out /*number-input*/ in your new property_database.js

Better to add the failure to layout/style/test/stylo-failures.md instead.

Comment 55

[Mats Palmgren (inactive)]

Attachment: part 10 - [css-ui] Add 'appearance' property to Stylo (with '-webkit-appearance' alias).

> internal="True" as well

I did so... but it caused a lot of test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cccd6de04dc85baff91a279533082a791fe4f2a&selectedJob=85717034
so I removed it again, and added the missing 'number-input' value too,
and that made it all nice and green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffdd4babb43d8a5e62eef79193101b68aa3acce6

> note that the servo stuff will have to be landed as a PR to servo, autoland
> can't handle it yet

Can you take care of landing this part on that side please?

Comment 56

[Manish Goregaokar [:manishearth]]

Comment on attachment 8850197 [details] [diff] [review]
part 10 - [css-ui] Add 'appearance' property to Stylo (with '-webkit-appearance' alias).

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

Okay. Let me know when you need this landed.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d1accaeddb
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7e5efb1bd8
part 2 - [css-ui] Change all consumers of StyleDisplay::mAppearance to use the accessor UsedAppearance() instead, and make mAppearance/mMozAppearance private.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5483a82f7ce9
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.  Enable both by default.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4511a175f2f
part 4 - [css-ui] Amend all uses of '-moz-appearance:none' in tests to also specify 'appearance:none' (automated change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8c03090d34
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d569fc566e43
part 6 - [css-ui] Manually tweak some tests for 'appearance' changes.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c957d8c0281e
part 7 - [css-ui] Amend DevTools themes with 'appearance:none' where needed so that they'll work with the pref on/off.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a39aaecd10f7
part 8 - [css-ui] Introduce '-webkit-appearance' as an alias for 'appearance' using the same pref.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8828e22dbaa
part 9 - [css-ui] Regenerate the DevTools properties database.  r=dholbert

Comment 58

[Manish Goregaokar [:manishearth]]

Servo side at https://github.com/servo/servo/pull/16110

Comment 59

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e389879425a7
Backed out 9 changesets for stylo test failures and this should be landed to autoland

Comment 60

[Bobby Holley (:bholley)]

The backout happened because this push went orange on inbound. The servo-side changes were never actually landed, and even if they were, they would have ended up on autoland, rather than inbound.

For any change touching servo, the procedure is:
(1) Land the servo-side PR
(2) Once it gets synced to autoland, land the gecko-side changes to autoland as well.

That limits the bustage to one cycle, which still isn't great, but the best we have given the tooling.

Since I know mats would rather not deal with this, I've fixed up the PR and am landing it. Once it lands, ihsiao will re-push the commits to autoland. Assuming there's no additional bustage here, that should take care of it.

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51cff1ed410b
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/cf701cfc0249
part 2 - [css-ui] Change all consumers of StyleDisplay::mAppearance to use the accessor UsedAppearance() instead, and make mAppearance/mMozAppearance private.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/af3341f27744
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.  Enable both by default.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/62474fd399f8
part 4 - [css-ui] Amend all uses of '-moz-appearance:none' in tests to also specify 'appearance:none' (automated change).  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2a92996b50fe
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6c1b4f18f221
part 6 - [css-ui] Manually tweak some tests for 'appearance' changes.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4f3bd2974af3
part 7 - [css-ui] Amend DevTools themes with 'appearance:none' where needed so that they'll work with the pref on/off.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b753e0d29901
part 8 - [css-ui] Introduce '-webkit-appearance' as an alias for 'appearance' using the same pref.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/675ff60e7653
part 9 - [css-ui] Regenerate the DevTools properties database.  r=dholbert

Comment 62

[Bobby Holley (:bholley)]

Results on autoland look good, should be all squared away here.

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51cff1ed410b
https://hg.mozilla.org/mozilla-central/rev/cf701cfc0249
https://hg.mozilla.org/mozilla-central/rev/af3341f27744
https://hg.mozilla.org/mozilla-central/rev/62474fd399f8
https://hg.mozilla.org/mozilla-central/rev/2a92996b50fe
https://hg.mozilla.org/mozilla-central/rev/6c1b4f18f221
https://hg.mozilla.org/mozilla-central/rev/4f3bd2974af3
https://hg.mozilla.org/mozilla-central/rev/b753e0d29901
https://hg.mozilla.org/mozilla-central/rev/675ff60e7653

Comment 64

[Mats Palmgren (inactive)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #60)
> The backout happened because this push went orange on inbound.

FTR, I was told on irc that temporary orange for stylo on inbound was acceptable.
It appears I was mislead about what the proper landing procedure is.

Comment 65

[Mats Palmgren (inactive)]

Comment on attachment 8835964 [details] [diff] [review]
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: we're trying to minimize the churn for web developers from this change and related changes in bug 605985 etc, by putting them in the same release, so this bug needs to be uplifted to Aurora v54 (at least)
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: TBD
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:low from a code POV, the risk here is web compat
[Why is the change risky/not risky?]:
[String changes made/needed]:none

Comment 66

[Bobby Holley (:bholley)]

(In reply to Mats Palmgren (:mats) from comment #64)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #60)
> > The backout happened because this push went orange on inbound.
> 
> FTR, I was told on irc that temporary orange for stylo on inbound was
> acceptable.

I think it boils down to the interpretation of "temporary". Having it go orange on autoland for a few cycles is fine/expected when one piece has landed but not the other. But the servo pieces can only land on autoland, so landing bustage to inbound means that everything needs to get merged around for things to (hopefully) go green again, which is too wide a window for additional bustage to creep in.

Comment 67

[Mats Palmgren (inactive)]

Fine, but that was not what I was told on irc. Go check the archive if you don't believe me.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Mats, I don't think anyone is accusing your of anything here.  Bobby was just trying to explain why this had to be backed out, because it's not obvious: the code is correct and so forth.  No blame is being attached, and we really appreciate you fixing the stylo piece in addition to the Gecko parts!

Comment 69

[Sebastian Zartner [:sebo]]

Added notes at https://developer.mozilla.org/en-US/docs/Web/CSS/appearance and https://developer.mozilla.org/en-US/Firefox/Releases/55.

Sebastian

Comment 70

[Kohei Yoshino [:kohei]]

(In reply to Sebastian Zartner [:sebo] from comment #69)
> Added notes at https://developer.mozilla.org/en-US/docs/Web/CSS/appearance
> and https://developer.mozilla.org/en-US/Firefox/Releases/55.

This actually goes to 54. See the uplift request in Comment 65. My fxsitecompat note has been updated accordingly.

Comment 71

[Sebastian Zartner [:sebo]]

(In reply to Kohei Yoshino [:kohei] from comment #70)
> (In reply to Sebastian Zartner [:sebo] from comment #69)
> > Added notes at https://developer.mozilla.org/en-US/docs/Web/CSS/appearance
> > and https://developer.mozilla.org/en-US/Firefox/Releases/55.
> 
> This actually goes to 54. See the uplift request in Comment 65. My
> fxsitecompat note has been updated accordingly.

Thank you for the hint! Though the uplift request was obviously not approved yet.
Anyway, since the fxsitecompat note already says 54, I've updated the notes on MDN now, too. See:

https://developer.mozilla.org/en-US/docs/Web/CSS/appearance
https://developer.mozilla.org/en-US/Firefox/Releases/54

@Mats: As far as I can see you only requested to uplift part 1 of your patches. Is that intended?

Sebastian

Comment 72

[Mats Palmgren (inactive)]

The uplift request is for all patches that landed in comment 63 + whatever is needed on
the github side for stylo.

Comment 73

[Gerry Chang [:gchang]]

Comment on attachment 8835964 [details] [diff] [review]
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.

Fix a web-compat related issue. Aurora54+.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift. Might as well fold them into one patch for uplift while you're at it :)

Comment 75

[Mats Palmgren (inactive)]

Attachment: Part 1 to 9 for Aurora

Looks green so far, except for stylo which is expected without part 10:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=622b48b55665958594f295843ee248cffbe50e96

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffeb9bf30934

Comment 77

[Aris]

Can somebody check and remove the changes added in the last few days causing the add-ons validator to throw warnings about -moz-appearance usage?

"-moz-appearance can only be used in chrome stylesheets"

http://i.imgur.com/IjvdkKp.png

Comment 78

[Karl Dubost💡 :karlcow]

Aris, 
anything which blocks you to use `appearance` instead of `-moz-appearance`?

Comment 79

[Aris]

For me "appearance" does not work at all. Tested up to todays Nightly css code using appearance does nothing.

Try this example by adding:

toolbarbutton {
  appearance: button !important;
}

to userChrome.css, userContent.css, a Stylish style or to your add-ons css files.

This example works fine though:

toolbarbutton {
  -moz-appearance: button !important;
}

Comment 80

[Boris Zbarsky [:bzbarsky]]

Right, the only valid values of "appearance" are "auto" and "none".

-moz-appearance does allow other values, as before, but is being restricted to UA sheets and sheets loaded from chrome:// URIs.

What is the URI of the sheet in question in the extension in question?

Comment 81

[Aris]

Validation results
https://addons.mozilla.org/en-us/developers/addon/classicthemerestorer/file/631507/validation

Add-ons css file
https://addons.mozilla.org/en-us/firefox/files/browse/631507/file/content/css/alt_optionspage2.css#L59
https://addons.mozilla.org/en-us/firefox/files/browse/631507/file/content/css/appbutton2.css#L21

Example urls, if Classic Theme Restorer is installed:
chrome://classic_theme_restorer/content/css/alt_optionspage2.css
chrome://classic_theme_restorer/content/css/appbutton2.css


Wouldn't it make more sense to run those checks only for WebExtensions and ignore them inside legacy add-ons? Firefox 52ESR will still be around and up-to-date for over a year from now, so there is no need to show those warning for add-ons, that are still supporting Firefox 45-56 / 45ESR / 52ESR. Imagine the amount of warnings AMO editors might have to look through every time an add-on update is uploaded to AMO.

The add-ons validator shows 315 warning for Class Theme Restorer at the moment.

Comment 82

[Boris Zbarsky [:bzbarsky]]

> Example urls, if Classic Theme Restorer is installed:

OK, for those urls using this property is fine and hence the addon validator really should not be warning here.  Jorge, do you know who's responsible for that part?

Comment 83

[Jorge Villalobos [:jorgev] (he/him)]

The test was introduced in https://github.com/mozilla/amo-validator/issues/524. Since we have to add lots of these, we usually go for the broader and easier to implement tests. Multiple false positives are expected for this and many other tests. If someone wants to submit a PR to refine this test, we can look at it.

Comment 84

[Boris Zbarsky [:bzbarsky]]

> Multiple false positives are expected for this and many other tests.

Non-actionable false positives?  As in, ones that the addon author cannot eliminate while preserving functionality?

Comment 85

[Jorge Villalobos [:jorgev] (he/him)]

Yes. The validator throws a great deal of warnings for potential issues which may or may not be real. For this particular case, the warning message indicates that it's still okay to use -moz-appearance in chrome:// stylesheets.

Comment 86

[Aris]

But wouldn't it make more sense to show only warnings, if it would be "not okay to use -moz-appearance" in that case?
We handled this very good a couple of times in the last few years, where warnings got shown indicating a specific "moz" prefix will not be supported from version XX on?

To be honest as an add-on dev seeing these warnings lets me think I did something wrong. Some add-on reviewers/editors might think like this too when they see those validation reports.

Comment 87

[Leif-AMO]

Experienced the same thing as Aris and wound up here, commenting on the validator GitHub discussion, as this change just adheres to spec, so comments about this change would be more appropriate on the spec discussion, but likely be ignored entirely at this point.

Comment 88

[Marat Tanalin]

Looks like the `-moz-appearance` property’s values like `button` stopped to work (both for web content and WebExtensions) due to this.

Flipping `layout.css.appearance.enabled` (+ browser restart) does not help.

Does not work   in Firefox 55.0a1 (Nightly)  20170506030204.
Works correctly in Firefox 54.0a2 (Dev. Ed.) 20170506004004.

Comment 89

[Boris Zbarsky [:bzbarsky]]

> Looks like the `-moz-appearance` property’s values like `button` stopped to work 

On elements styled with "appearance: auto"?  Or on elements styled with "appearance: none" or without any appearance styles?

-moz-appearance should work on things that have "appearance: auto" but not on things with "appearance: none".

Comment 90

[Marat Tanalin]

(In reply to comment #89)

I’m not sure what the standard unprefixed `appearance` property and the `auto` value have to do with this since I explicitly mentioned the prefixed `-moz-appearance` property and its `button` value.

An example that worked in Firefox 54 and stopped to work in Firefox 55:

    <style>
        A {-moz-appearance: button; }
    </style>

    <a href="#">Demo</a>

Comment 91

[Mats Palmgren (inactive)]

We're intentionally removing that feature.  You can for a transition period use
A {-moz-appearance: button; appearance:auto; } but please note that we plan to
stop supporting the -moz-appearance property in an upcoming release.

Comment 92

[Boris Zbarsky [:bzbarsky]]

> I’m not sure what the standard unprefixed `appearance` property and the `auto` value have to do with this 

-moz-appearance was changed to only have an effect on elements with "appearance: auto".  This was a purposeful change, and as Mats said is temporary until we completely remove -moz-appearance.

Comment 93

[Marat Tanalin]

(In reply to comment #91)
(In reply to comment #92)

Thanks, Mats and Boris. That’s OK.

What is the exact version of Firefox planned to be the one with `-moz-appearance` completely removed?

Is an alternative for `-moz-appearance: button` planned to be implemented?

Fwiw, I don’t use/need the feature, I just need to know this to provide full and clear information in my blog post about various ways of styling a link as a button:

http://tanalin.com/en/blog/2013/03/link-button/

Comment 94

[Mats Palmgren (inactive)]

> What is the exact version of Firefox planned to be the one with `-moz-appearance` completely removed?

Not decided yet, but soon I hope (bug 1351745).  It will be announced on dev-platform.

> Is an alternative for `-moz-appearance: button` planned to be implemented?

Not anything beyond what you get with appearance:none|auto (with -webkit-appearance
as an alias).  IOW, you can toggle the native theme on/off for form controls using
those properties.

I think any future style features for form controls needs to be standardized
by the CSSWG first if we're going to implement it.

Comment 95

[Marat Tanalin]

(In reply to comment #94)
Fwiw, Chromium (Blink) does support values like `button` for the `-webkit-appearance` property. So for now, supporting `-webkit-appearance` in Firefox without supporting its values like `button` means that support is incomplete.

Are you aware whether Chromium team plans to remove the feature too?

Comment 96

[Mats Palmgren (inactive)]

Right, I'm aware of that, but please note that Edge only supports -webkit-appearance:none
and no other values, and since they have shipped that we believe it's web-compatible.

I'm not aware of their plans, but I will certainly try to convince them to remove all
values except auto/none after we have removed -moz-appearance.

Comment 97

[Mats Palmgren (inactive)]

We have backed out all of this (in bug 1365614) because we found out that
it caused to many regressions on web sites.  It's simply not web-compatible
in the way we first thought and how the CSS UI spec describes it.
Sorry for the churn everyone. :(

I've filed bug 1368555 to alias it to -moz-appearance instead and adding
support for all -webkit-appearance keywords this time.

Comment 98

[Mats Palmgren (inactive)]

Future work on -webkit-appearance will happen in bug 1368555, not here.

Comment 99

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This was backed out in bug 1365614 (along with other bugs).

Comment 100

[Karl Dubost💡 :karlcow]

Chromium is shipping it.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/C5afPWCPXp8/5-uxJaSBAQAJ