Cannot override appearance: none with appearance: checkbox

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mborn319, Assigned: mats)

Tracking

({dev-doc-complete, regression})

54 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54+ fixed, firefox55+ fixed)

Details

Attachments

(12 attachments, 2 obsolete attachments)

9.70 KB, image/jpeg
Details
6.19 KB, text/html
Details
1.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
17.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
348.96 KB, patch
Details | Diff | Splinter Review
1.51 KB, patch
mats
: review+
Details | Diff | Splinter Review
17.18 KB, patch
mats
: review+
Details | Diff | Splinter Review
333.63 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
345.45 KB, patch
Details | Diff | Splinter Review
32.57 KB, patch
Details | Diff | Splinter Review
334.30 KB, patch
mats
: review+
Details | Diff | Splinter Review
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.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
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?
Assignee: nobody → mats
Flags: needinfo?(bzbarsky)
Posted file various -webkit-appearance tests (obsolete) —
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.
[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...
Flags: needinfo?(bzbarsky)
Track 54+ as web compat regression.
Attachment #8868811 - Attachment is obsolete: true
Blocks: 1333482
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
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.)
Attachment #8869109 - Flags: review?(bzbarsky)
Attachment #8869110 - Flags: review?(bzbarsky)
Attachment #8869113 - Flags: review?(bzbarsky)
Attachment #8869113 - Attachment is patch: true
Comment on attachment 8869109 [details] [diff] [review]
part 1 - Backout bug 1358840

r=me
Attachment #8869109 - Flags: review?(bzbarsky) → review+
Comment on attachment 8869110 [details] [diff] [review]
part 2 - Backout bug 1357655

r=me
Attachment #8869110 - Flags: review?(bzbarsky) → review+
Comment on attachment 8869114 [details] [diff] [review]
part 4 - Backout bug 1333482 part 10 (Stylo)

r=me
Attachment #8869114 - Flags: review?(bzbarsky) → review+
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?
Attachment #8869113 - Flags: review?(bzbarsky) → review+
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.
Attachment #8869113 - Flags: review+ → review?(dholbert)
I'll carry over the r+ on this part since the interdiff to Trunk is empty.
Attachment #8869249 - Flags: review+
Ditto.
Attachment #8869251 - Flags: review+
Attachment #8869252 - Flags: review?(dholbert)
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.
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 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
Attachment #8869113 - Flags: review?(dholbert) → review+
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 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).
Attachment #8869252 - Flags: review?(dholbert) → review+
Thanks for the quick review Daniel!

Here's a rebased part 3 against m-i rev 05c1b54b0319.
Attachment #8869113 - Attachment is obsolete: true
Attachment #8869874 - Flags: review+
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!
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
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.
Attachment #8869249 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
Backout by cbook@mozilla.com:
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
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
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
I assume for the uplift things just get landed directly, by the way...
Pushed by kwierso@gmail.com:
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
The backout fixes this bug.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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 on attachment 8869249 [details] [diff] [review]
Beta v54 - part 1

Backout for web compat issues. Beta54+. Should be in 54 beta 11.
Attachment #8869249 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1365627
Keywords: dev-doc-needed
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
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
(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.
(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
You need to log in before you can comment on or make changes to this bug.