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

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?

(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.

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

(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.

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

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

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

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

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 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 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 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 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 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 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

(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.)

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).

(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 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 ?

I guess so, sure.

(missed one)

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 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

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

(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.

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)

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

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?

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

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.

(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.

(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?

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

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 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.

: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 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.)

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 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

(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.

(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 )

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

> and comment out /*number-input*/ in your new property_database.js Better to add the failure to layout/style/test/stylo-failures.md instead.

> 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 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.

Pushed by mpalmgren@mozilla.com: 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

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

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

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.

Pushed by bholley@mozilla.com: 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

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

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

(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 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

(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.

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

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!

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

(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.

(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

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

Comment on attachment 8835964 [details] [diff] [review] part 1 - [css-ui] Introduce the 'appearance: auto | none' property. Fix a web-compat related issue. Aurora54+.

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

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

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

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

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; }

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?

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.

> 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?

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.

> 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?

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.

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.

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.

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.

> 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".

(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>

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.

> 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.

(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/

> 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.

(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?

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.

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.

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

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