Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

(Blocks: 3 bugs, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla55
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(URL)

Attachments

(11 attachments, 4 obsolete attachments)

15.99 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
66.67 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
195.76 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.29 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
25.58 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
15.44 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
23.20 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.34 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.17 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
333.42 KB, patch
Details | Diff | Splinter Review
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?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c419b608f7b8a0f6844a1601c2f54dbaf54a10
Blocks: 1302104
(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.
Keywords: dev-doc-needed
Yeah, I think you're right.  OK, I'll add a -webkit- prefixed alias, which means we
probably need bug 605985 too.
Depends on: 605985

Updated

6 months ago
Blocks: 1336676
Blocks: 1337960
Created attachment 8835964 [details] [diff] [review]
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.
Attachment #8835964 - Flags: review?(dholbert)
Created attachment 8835966 [details] [diff] [review]
part 2 - [css-ui] Change all consumers of StyleDisplay::mAppearance to use the accessor UsedAppearance() instead, and make mAppearance/mMozAppearance private.
Attachment #8835966 - Flags: review?(dholbert)
Created attachment 8835967 [details] [diff] [review]
part 3 - [css-ui] Put 'appearance' and '-moz-appearance' behind separate prefs.
Attachment #8835967 - Flags: review?(dholbert)
Created 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).
Attachment #8835968 - Flags: review?(dholbert)
Created attachment 8835969 [details] [diff] [review]
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.
Attachment #8835969 - Flags: review?(dholbert)
Created attachment 8835970 [details] [diff] [review]
part 6 - [css-ui] Manually tweak some tests for 'appearance' changes.
Attachment #8835970 - Flags: review?(dholbert)
Created 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.
Attachment #8835971 - Flags: review?(dholbert)
Created attachment 8835974 [details] [diff] [review]
part 8 - [css-ui] Introduce '-webkit-appearance' as an alias for 'appearance' using the same pref.
Attachment #8835974 - Flags: review?(dholbert)
Created attachment 8835976 [details] [diff] [review]
part 9 - [css-ui] Regenerate the DevTools properties database.
Attachment #8835976 - Flags: review?(dholbert)
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.
Attachment #8835964 - Flags: review?(dholbert) → review+
Attachment #8835966 - Flags: review?(dholbert) → review+
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)
Attachment #8835967 - Flags: review?(dholbert) → review+
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.)
Attachment #8835970 - Flags: review?(dholbert) → review+
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)
Attachment #8835971 - Flags: review?(dholbert) → review+
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.)
Attachment #8835974 - Flags: review?(dholbert) → review+
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
Attachment #8835976 - Flags: review?(dholbert) → review+
(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.)
Flags: needinfo?(mats)

Updated

5 months ago
Keywords: site-compat
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).
Flags: needinfo?(mats)
(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.
Created attachment 8837440 [details] [diff] [review]
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.
Attachment #8835969 - Attachment is obsolete: true
Attachment #8835969 - Flags: review?(dholbert)
Attachment #8837440 - Flags: review?(dholbert)
Created attachment 8837441 [details] [diff] [review]
part 5 - [css-ui] Enable '-moz-appearance' support for some tests.

(missed one)
Attachment #8837440 - Attachment is obsolete: true
Attachment #8837440 - Flags: review?(dholbert)
Attachment #8837441 - Flags: review?(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
Attachment #8835968 - Flags: review?(dholbert) → review+
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
Attachment #8837441 - Flags: review?(dholbert) → review+
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/moz-appearance-property-has-been-removed/
Depends on: 1340014

Updated

5 months ago
Keywords: addon-compat
(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)
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
Jorge, is there anything else we can do to announce this change to addon devs,
or help them in some way?
Flags: needinfo?(jorge)
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?
Flags: needinfo?(bzbarsky)

Comment 40

5 months ago
Inline <style> elements in a chrome://-URL document would count as a "chrome sheet", yes.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(jorge)
(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?
Flags: needinfo?(manishearth)
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
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
Flags: needinfo?(manishearth)
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.

Updated

5 months ago
See Also: → bug 1246836
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.
Attachment #8835967 - Flags: review?(wmccloskey)

Updated

5 months ago
Attachment #8835967 - Flags: review?(wmccloskey) → review+
Created attachment 8850124 [details] [diff] [review]
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
Attachment #8835967 - Attachment is obsolete: true
Attachment #8850124 - Flags: review?(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.)
Attachment #8850124 - Flags: review?(dholbert) → review+
Created attachment 8850130 [details] [diff] [review]
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
Attachment #8850130 - Flags: review?(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
Attachment #8850130 - Flags: review?(manishearth) → review+
(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

Comment 54

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

Better to add the failure to layout/style/test/stylo-failures.md instead.
Created attachment 8850197 [details] [diff] [review]
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?
Attachment #8850130 - Attachment is obsolete: true
Attachment #8850197 - Flags: review?(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.
Attachment #8850197 - Flags: review?(manishearth) → review+

Comment 57

4 months ago
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

Comment 59

4 months ago
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.

Comment 61

4 months ago
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
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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
Attachment #8835964 - Flags: approval-mozilla-aurora?
(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.

Comment 68

4 months ago
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
Keywords: dev-doc-needed → dev-doc-complete
(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
Flags: needinfo?(mats)
The uplift request is for all patches that landed in comment 63 + whatever is needed on
the github side for stylo.
Flags: needinfo?(mats)

Updated

4 months ago
status-firefox54: --- → affected
Comment on attachment 8835964 [details] [diff] [review]
part 1 - [css-ui] Introduce the 'appearance: auto | none' property.

Fix a web-compat related issue. Aurora54+.
Attachment #8835964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift. Might as well fold them into one patch for uplift while you're at it :)
Flags: needinfo?(mats)
Created attachment 8852305 [details] [diff] [review]
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
Flags: needinfo?(mats)

Comment 76

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ffeb9bf30934
status-firefox54: affected → fixed
Flags: in-testsuite+
Summary: [css-ui] Implement 'appearance:auto | none' and make -moz-appearance UA-sheet only → [css-ui] Implement 'appearance:auto | none', with '-webkit-appearance' as an alias.
Blocks: 1351745
Depends on: 1355521
No longer depends on: 1355521
Depends on: 1355521
No longer depends on: 1355521

Comment 77

3 months ago
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

3 months ago
Aris, 
anything which blocks you to use `appearance` instead of `-moz-appearance`?
Flags: needinfo?(aris-addons)

Comment 79

3 months ago
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;
}
Flags: needinfo?(aris-addons)

Comment 80

3 months ago
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

3 months ago
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

3 months ago
> 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?
Flags: needinfo?(jorge)
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.
Flags: needinfo?(jorge)

Comment 84

3 months ago
> 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.

Comment 86

3 months ago
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

3 months ago
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.

Comment 89

3 months ago
> 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".
Flags: needinfo?(mtanalin)
(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>
Flags: needinfo?(mtanalin)
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

3 months ago
> 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.
Depends on: 1365614

Updated

2 months ago
Depends on: 1368457
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.

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Future work on -webkit-appearance will happen in bug 1368555, not here.
Status: REOPENED → RESOLVED
Last Resolved: 4 months ago2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.