1365614 - Cannot override appearance: none with appearance: checkbox

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Michael B]

Attachment: ff-bug-compare-ff-versions.jpg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170418004027

Steps to reproduce:

Set appearance: none (with prefixes) globally on all input elements, then target checkboxes and radio buttons to reset as appearance: checkbox/radio.

See JS fiddle: https://jsfiddle.net/g9qjhdew/2/

CSS to reproduce:
input {
  -webkit-appearance: none;
  -moz-appearance: none;
  appearance: none;
}
input[type="checkbox"] {
  -webkit-appearance: checkbox;
  -moz-appearance: checkbox;
  appearance: checkbox;
}


Actual results:

In Firefox Dev Edition, 54.0a2, the checkbox is completely "invisible" - zero width, height, no border or background. In the inspector, you can see that both -webkit-appearance: checkbox and appearance: checkbox are flagged as "invalid property value", while -moz-appearance: checkbox is accepted as legitimate but does not affect the display of the non-rendered checkbox.

In Firefox 53.0.2, I can confirm this "works correctly" (e.g. shows a checkbox). It shows the same invalid property value for both -webkit-appearance: checkbox and appearance: checkbox.


Expected results:

The checkbox should have had the default checkbox styles, exactly like we had never applied appearance: none in the first place.

Please note that that the bug title specifically mentions overriding appearance: none with appearance: checkbox, but this problem holds true for any prefixed version. You cannot override -webkit-appearance: none with -webkit-appearance: checkbox or -moz-appearance: none with -moz-appearance: checkbox.

Comment 1

[Mats Palmgren (inactive)]

The winning declarations there are:
  appearance: none;
  -moz-appearance: checkbox;
which we now interpret as 'none' since 'appearance' trumps '-moz-appearance'.

Sigh, I think we need to disable 'appearance' for now and figure out something
that doesn't break the web... probably just aliasing '-webkit-appearance' to
'-moz-appearance' instead (for now) and removing 'appearance' altogether.
Still with the intention of removing '-moz-appearance' eventually...

Boris, WDYT?

Comment 2

[Mats Palmgren (inactive)]

Attachment: various -webkit-appearance tests

It appears that my original investigation of Edge was flawed. :(
They actually do support all the -webkit-appearance values
(in the sense that the computed values are the specified ones).
So, Edge seems to be mostly compatible with Chrome for these cases.

Comment 3

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51314e8b939d8c4ee4ec7dc188ce9d07a7befada

Comment 4

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Likely web compat regression that needs to block release.

I don't know how similar -moz-appearance and -webkit-appearance are and hence whether aliasing them makes sense.

I agree that we should back out support for "appearance" in favor of our old behavior and go back to the working group, because clearly the spec as currently written is not web-compatible in basic cases like the one in comment 0...

Comment 5

[Gerry Chang [:gchang]]

Track 54+ as web compat regression.

Comment 6

[Mats Palmgren (inactive)]

Attachment: various -webkit-appearance tests

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 1 - Backout bug 1358840

I don't think our support for [-webkit-]appearance is viable in its current form,
so I'd prefer to just back this all out now while the patches are still mostly
revertible.

(I suspect that we'll need to support -webkit-appearance eventually (for web
compat) but next time we should probably make it an alias for -moz-appearance
and make a union of all possible values, or something like that.  I don't
think supporting an unprefixed appearance has much value given how these
properties are used in the wild, so we should probably argue for it to be
removed from css-ui and standardize -webkit-appearance instead.)

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 2 - Backout bug 1357655

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 3 - Backout bug 1333482 part 1-9

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 4 - Backout bug 1333482 part 10 (Stylo)

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

Attachment: Trunk rollup patch if needed

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8869109 [details] [diff] [review]
part 1 - Backout bug 1358840

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8869110 [details] [diff] [review]
part 2 - Backout bug 1357655

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8869114 [details] [diff] [review]
part 4 - Backout bug 1333482 part 10 (Stylo)

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8869113 [details] [diff] [review]
part 3 - Backout bug 1333482 part 1-9

There is no way I can sanely review this... rs=me if this was a clean backout; otherwise, maybe ask dholbert to check that this is the right thing, since he reviewed the thing being backed out?

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8869113 [details] [diff] [review]
part 3 - Backout bug 1333482 part 1-9

> rs=me if this was a clean backout

I had to manually fix a few trivial conflicts due to changes in surrounding code.
I would have asked dholbert, but he's on PTO (back on Monday I think).  I guess
it can wait until then.

Comment 18

[Mats Palmgren (inactive)]

Attachment: Beta v54 - part 1

I'll carry over the r+ on this part since the interdiff to Trunk is empty.

Comment 19

[Mats Palmgren (inactive)]

Attachment: Beta v54 - part 2

Ditto.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Beta v54 - part 3

Comment 21

[Mats Palmgren (inactive)]

Looks green on Beta too, except for Stylo.  The failures seems unrelated to my changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03bc7ec6988eeda899d8f9dff40ccf660603d7c0
It appears we don't run Stylo tests on Beta by default, so I assume it's OK:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta
It looks like the original Stylo part for bug 1333482 never landed on v54,
so there's nothing to back for that part on Beta.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Beta rollup patch if needed

Comment 23

[Mats Palmgren (inactive)]

Attachment: Trunk / Beta interdiff for part 3

FYI, Bugzilla's interdiff of the rollup patches is lying.
This is what the diff looks like between Trunk and Beta for part 3,
in case that helps with the review.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8869113 [details] [diff] [review]
part 3 - Backout bug 1333482 part 1-9

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

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> Comment on attachment 8869113 [details] [diff] [review]
> part 3 - Backout bug 1333482 part 1-9
> 
> There is no way I can sanely review this... rs=me if this was a clean
> backout; otherwise, maybe ask dholbert to check that this is the right
> thing, since he reviewed the thing being backed out?

I reviewed this by doing an "easy" local backout of all the patches (via "hg up [lastpatch]; hg revert -r [parent-of-firstpatch] --all; hg qnew my-backout"), and then I used a merge tool to compare this generated patch against mats's unbitrotted version here.

All this patch's differences from my clean-backout seemed sane. r=me

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8869113 [details] [diff] [review]
part 3 - Backout bug 1333482 part 1-9

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

::: layout/style/res/forms.css
@@ +599,5 @@
>  }
>  
>  %if defined(MOZ_WIDGET_ANDROID)
>  /*
> + * These platforms doesn't have any theming support and thus -moz-appearance:none

(Technically, this particular "appearance" usage [which this backout patch is changing to -moz-appearance] is *not* associated with the patches you're backing out here.  This code-comment in forms.css always used unprefixed "appearance", from when this code-comment was originally added, in bug 605985 -- and this unprefixed usage predated the patch-stack that you're backing out here by a month or so.

Having said that, I'm still fine with this being part of this patch, since it's adjusting this comment to reflect reality. Only mentioning in case you care about this patch being as "pure" of a backout as possible.)

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8869252 [details] [diff] [review]
Beta v54 - part 3

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

I reviewed the beta patch using the same process described in comment 24, and it looks fine to me.  (It's got the same not-technically-a-backout change in forms.css that I noted in comment 25, and that seems fine as noted there).

Comment 27

[Mats Palmgren (inactive)]

Attachment: part 3 - Backout bug 1333482 part 1-9

Thanks for the quick review Daniel!

Here's a rebased part 3 against m-i rev 05c1b54b0319.

Comment 28

[Mats Palmgren (inactive)]

I'm not going to try to land this myself since it involves stylo changes...

The checkin request is for part 1-4 on trunk (i.e. the patches not marked "Beta").  Thanks!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdaaaf661b0f
part 1 - Backout bug 1358840.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e528713f882c
part 2 - Backout bug 1357655.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb688fe1ba3
part 3 - Backout bug 1333482 part 1-9.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/fde1635233e9
part 4 - Backout bug 1333482 part 10 (Stylo). Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 8869249 [details] [diff] [review]
Beta v54 - part 1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1333482 
[User impact if declined]: web compat issues for content using [-webkit-]appearance
[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]: no
[List of other uplifts needed for the feature/fix]:all is included here
[Is the change risky?]:no
[Why is the change risky/not risky?]: it's a backout
[String changes made/needed]:no

This uplift request is for all three Beta v54 part 1-3 patches.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdaaaf661b0f
https://hg.mozilla.org/mozilla-central/rev/cdaaaf661b0f
https://hg.mozilla.org/mozilla-central/rev/e528713f882c
https://hg.mozilla.org/mozilla-central/rev/fde1635233e9

Comment 32

[Carsten Book [:Tomcat]]

backed out on request from glob because part 4 touches stylo and so broke which in turn broke servo-vcs-sync and files under servo/ shouldn't be touched i was told

Comment 33

[Mats Palmgren (inactive)]

That's why I explicitly said in comment 28 that I'm not landing this because
I have no clue how to land changes to stylo.  I assumed sheriffs would know
how to deal with that part.

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/349078b5daae
Backed out changeset fde1635233e9 
https://hg.mozilla.org/mozilla-central/rev/c902af1156a1
Backed out changeset bbb688fe1ba3 
https://hg.mozilla.org/mozilla-central/rev/0a0376eb366c
Backed out changeset e528713f882c 
https://hg.mozilla.org/mozilla-central/rev/8f4d2d35cb31
Backed out changeset cdaaaf661b0f on request from glob

Comment 35

[Carsten Book [:Tomcat]]

Hey Mats,

we sheriffs don't land stylo changes and even can't backout this servo-vcs-sync pushes, the stylo changes are mostly pushed by the stylo guys like bholley, heycam, xidorn etc - they know how to push. Needinfo them.

I think that this pushes happen via github but not sure

Comment 36

[Boris Zbarsky [:bzbarsky]]

Hrm.  I guess we didn't actually have any public docs on how to land stylo patches.  I added some at https://wiki.mozilla.org/Quantum/Stylo#Committing_stylo_changes

In this instance I also created https://github.com/servo/servo/pull/16996 and will land the autoland bits as needed.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I assume for the uplift things just get landed directly, by the way...

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff0bd62e461
part 1 - Backout bug 1358840.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz
https://hg.mozilla.org/integration/autoland/rev/483bc7b92cab
part 2 - Backout bug 1357655.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz
https://hg.mozilla.org/integration/autoland/rev/b1daab599fec
part 3 - Backout bug 1333482 part 1-9.  Removes support for [-webkit-]appearance for now b/c web compat issues.  r=bz

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Merge of backouts:
https://hg.mozilla.org/mozilla-central/rev/57a57857725d
https://hg.mozilla.org/mozilla-central/rev/1ff0bd62e461
https://hg.mozilla.org/mozilla-central/rev/483bc7b92cab
https://hg.mozilla.org/mozilla-central/rev/b1daab599fec

Comment 40

[Mats Palmgren (inactive)]

The backout fixes this bug.

Comment 41

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #37)
> I assume for the uplift things just get landed directly, by the way...

Right, there are no stylo changes needed for v54.

Comment 42

[Gerry Chang [:gchang]]

Comment on attachment 8869249 [details] [diff] [review]
Beta v54 - part 1

Backout for web compat issues. Beta54+. Should be in 54 beta 11.

Comment 43

[Mats Palmgren (inactive)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4183fa9f04182521e485da0402cc841cec008f43
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/40bcde7aff0da5df22c2f7388cead96656a9a7ed
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7166b44658df0723b16d43b7f884deb73c99f0ba

Comment 44

[Sebastian Zartner [:sebo]]

Docs already got updated by Chris Mills reflecting the removal of 'appearance'.
Changed https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance.
Deleted https://developer.mozilla.org/en-US/docs/Web/CSS/appearance.
Removed release note from https://developer.mozilla.org/en-US/Firefox/Releases/54.

Mats, can you have a look if the changes are correct?

Sebastian

Comment 45

[Mats Palmgren (inactive)]

The v54 release notes says:
"<input> elements of types checkbox and radio with -moz-appearance: none; set on them are now non-replaced elements, and can now be styled with significantly more CSS properties including border and background-color (bug 418833, bug 605985, bug 1320732)."

But bug 418833 and bug 1320732 was also backed out (on all branches).
(That's unrelated to my changes in this bug though.)

So I think the release notes should say something like:

"<input> elements of types checkbox and radio with -moz-appearance: none; set on them are now non-replaced elements (bug 605985) for compatibility with other browsers."

Looks good otherwise. Thanks.

Comment 46

[Chris Mills [:cmills]]

(In reply to Mats Palmgren (:mats) from comment #45)
> The v54 release notes says:
> "<input> elements of types checkbox and radio with -moz-appearance: none;
> set on them are now non-replaced elements, and can now be styled with
> significantly more CSS properties including border and background-color (bug
> 418833, bug 605985, bug 1320732)."
> 
> But bug 418833 and bug 1320732 was also backed out (on all branches).
> (That's unrelated to my changes in this bug though.)
> 
> So I think the release notes should say something like:
> 
> "<input> elements of types checkbox and radio with -moz-appearance: none;
> set on them are now non-replaced elements (bug 605985) for compatibility
> with other browsers."
> 
> Looks good otherwise. Thanks.

Fixed.

Comment 47

[Sebastian Zartner [:sebo]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #46)
> (In reply to Mats Palmgren (:mats) from comment #45)
> > The v54 release notes says:
> > "<input> elements of types checkbox and radio with -moz-appearance: none;
> > set on them are now non-replaced elements, and can now be styled with
> > significantly more CSS properties including border and background-color (bug
> > 418833, bug 605985, bug 1320732)."
> > 
> > But bug 418833 and bug 1320732 was also backed out (on all branches).
> > (That's unrelated to my changes in this bug though.)
> > 
> > So I think the release notes should say something like:
> > 
> > "<input> elements of types checkbox and radio with -moz-appearance: none;
> > set on them are now non-replaced elements (bug 605985) for compatibility
> > with other browsers."
> > 
> > Looks good otherwise. Thanks.
> 
> Fixed.

So let's mark this as dev-doc-complete. Thanks for the quick responses!

Sebastian