Closed Bug 605985 Opened 14 years ago Closed 7 years ago

-moz-appearance:none should make checkboxes and radios be non-replaced elements (except on Android)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
p11 + ---
firefox54 --- fixed

People

(Reporter: jykng, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [webcompatibility] [webcompat])

Attachments

(5 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

So while applying -moz-appearance:none (which I'm guessing is appearance:normal css3 UI module) to checkbox and radio input form elements does allow for some more customization, one thing that doesn't change are the borders.  The 2px grooved borders remain regardless of what the border is declared as, and also adjusting the border-radius doesn't change the shape either.  Other things like box-shadow and size can be applied to the elements though.

Reproducible: Always

Actual Results:  
The border remains 2px groove.  Also background color can't be changed, but background image can be changed.  And the checkmark/dot remains too.

Expected Results:  
The border and background-color becomes whatever it's declared as.  The element becomes something like a plain block element rather than just a checkbox/radio that can be changed slightly.

This could be circumvented by hiding the checkbox or radio itself and applying custom styling to the :after or :before of the label of the element.

The most common use of this would be to use custom images as a background for the elements.

Webkit allows for changes to the background color and borders if -webkit-appearance:none.
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
This isn't really what -moz-appearance means; it just triggers native theme drawing.

In theory, form controls probably should have had their own values of 'display', and then you'd have been able to set display to block or inline, but it's probably too late for that.
There are !important UA rules setting the border and background, because too many sites styled them assuming it would work as in IE and made them completely invisible in Gecko...
http://www.w3.org/TR/css3-ui/#active is something that's suggested, but in practice doesn't work.  Using that method in the example is also something that doesn't resemble the mistakes done by those styling something for IE.

There are alternative ways to style such elements, but they generally involve using javascript with images (for all browsers), or complex uses of generated content on labels.  Both cases involves hiding the original element.
Any update on this? Incredibly annoying not to be able to completely style a radio button without having to replace with Javascript. -webkit-appearance has it working fine.
Blocks: 756437
No longer blocks: 756437
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
I found an solution:
http://cssdeck.com/item/321/css-checkbox-styles

Just kick the checkbox out, with visibility:hidden,

And then create a new <label/> for it, and link it by id with "for" attribute.
The label can be styled in any way you want.
While I was working on making our product accessible for keyboard usage I noticed that the method christopherzuendorf@gmx.de suggested and we are already using totally breaks the usability for disabled users who depend on the keyboard. <input> elements with display: none or visibility: hidden wont be reachable by the TAB key. If anyone knows a workaround I would be really happy about it.

I really want to style our checkboxes according to our visual guidelines and hate the OS shining through our interface. But in this case it conflicts with basic usabilty guidelines and causes a dilemma. On Webkit it is no problem because -webkit-appereance: none basically lets the checkbox disappear or keeps it up to you to style it and keeps it selectable via TAB. 

Leaving this bug unfixed encourages the bad practice of misusing the <label> as a visual checkbox and making the actual checkbox inaccessible; so please higher the priority of this bug for the sake of a more accessible web :)

If I just missed a good workaround please note me, I would be pretty happy about that too!
I confirm this bug too at Firefox 19.0.2 with http://jsfiddle.net/j8WvL/

See also bug 418833 which seems to be relative.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?
I added the [NEEDINFO] tag in order to get information about any relative css specification which describes the right behavior on this.
Flags: needinfo?
Flags: needinfo?
Flags: needinfo?
* http://habrahabr.ru/post/113391/
* http://jsfiddle.net/iegik/j8WvL/6/
* http://wiki.csswg.org/spec/css4-ui#dropped-css3-features
* https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance

Non-standard
That`s it, can close it? Or try to make this browser better?

And finnaly - http://en.wiktionary.org/wiki/none
"none" - means "no one", nothing, not this.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0
Yup this bug is still giving problems 

Browser : Firefox 23.0/ windows

I guess I will be working on this bug. Don't hate me. I have no clue how i am going to do it. Might take some time.
Just looking at Firefox Nightly today (29.0a1 (2014-01-29)) and although I have -moz-appearance: none; set on boxes I still get the white box and border.

Is there any preferable way to remove the default UI from input and checkboxes (as I realise that appearance: none is not specced)?
Regarding Nightly: I've noticed that with a pretty recent update, that the border and background were still applied with -moz-appearance:none.

I think there must've been a recent change or bug that got introduced, I'm still trying to track it down.
If you have label surrounding your radio buttons they won't work in firefox. Why I don't know but take them off and you'll be fine.
In case anyone is interested in a standard solution to hide radios/checkboxes and customize labels, here it is: 
http://jsfiddle.net/mariusandreiana/4PmYG/
Issue still exists in 27.0.1

UA: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0
If you visit <http://s.tabelog.com/smartphone/restaurant_list/list/?pal=&LstPrf=&LstAre=&lunch=&feature_cond=&PG=&utf8=%E2%9C%93&lid=top_search_form&sa=%E6%BA%9C%E6%B1%A0%E5%B1%B1%E7%8E%8B&sk=> in Fennec, you'll get something horrid like the screenshot on the right at https://cloudup.com/cJUdQwYQkwN. This is because they're relying on -webkit-appearance: none. Since there is no functional equivalent in Gecko, our users really are out of luck--even with site outreach. :(
tracking-p11: --- → +
Whiteboard: [webcompatibility]
Blocks: 1170774
@marius Unfortunately that removes keyboard accessibility. My current approach uses opacity, then compensates for non-removable border with a negative margin.

 input[type=checkbox].replaced-with-label {
    width: 0;
    opacity: 0;
    margin-left: -3px; /* takes up space even though width is 0 */
 }
 @-moz-document url-prefix() {
     input[type=checkbox].replaced-with-label {
         -moz-appearance: none;
         /*un-removable 2px border on checkboxes! https://bugzilla.mozilla.org/show_bug.cgi?id=605985*/
         margin-right: -4px;
     }
 }
Flags: needinfo?(dholbert)
Is there documentation anywhere on what "-webkit-appearance:none" is actually expected to do? (or describing what it does in webkit-based browsers at least)

From brief testing, it looks like it approximately means the element simply generates a frame (box) based purely on its "display" type, without any of it's tag's form-widget rendering behavior at all.  But it's not quite this simple, because Chrome & Edge [which has some -webkit-appearance:none support] each have some quirks which suggest that they've got some residual widget rendering behavior.

(In particular: Edge can still be made to render a radio button inside of -webkit-appearance:none, using ::before:
  jsfiddle: https://jsfiddle.net/dvy8b0gf/5/
  tweet w/ screenshot: https://twitter.com/CodingExon/status/626876377321439232

And in Chrome, checkboxes and radio buttons with "-webkit-appearance:none" can have different baselines, despite having the same styling & otherwise rendering the same:
  jsfiddle: https://jsfiddle.net/dnrwztdx/

I suspect those are both just bugs, but I'm not sure because I don't quit know how this is expected to work.)
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #28)
> From brief testing, it looks like it approximately means the element simply
> generates a frame (box) based purely on its "display" type, without any of
> it's tag's form-widget rendering behavior at all.

(I'm only talking about radios and checkboxes here, BTW.  For button- and textfield-flavored widgets, "-webkit-appearance:none" seems to behave more like "-moz-appearance: none" -- it just removes native theming, but keeps the functional widget-flavored box. And you can style borders/backgrounds as-desired, regardless of '-[vendor]-appearance'.)
(In reply to Daniel Holbert [:dholbert] from comment #28)
> Is there documentation anywhere on what "-webkit-appearance:none" is
> actually expected to do? (or describing what it does in webkit-based
> browsers at least) [ on radio buttons and checkboxes]

(We might want to include this^^ documentation in our "standardize/document the behavior of -webkit prefixed CSS that seems to be required for web compatibility" thrust.)

On the implementation side -- if it's just as simple as generating a custom frame/box based only on "display" (maybe with 'inline' promoted to 'inline-block'), then we could probably hack something up without too much trouble in nsCSSFrameConstructor.cpp, I expect.
(treating -webkit-appearance as its own property, not as a -moz-appearance alias)
(In reply to Daniel Holbert [:dholbert] from comment #30)
> (In reply to Daniel Holbert [:dholbert] from comment #28)
> > Is there documentation anywhere on what "-webkit-appearance:none" is
> > actually expected to do? (or describing what it does in webkit-based
> > browsers at least) [ on radio buttons and checkboxes]
> 
> (We might want to include this^^ documentation in our "standardize/document
> the behavior of -webkit prefixed CSS that seems to be required for web
> compatibility" thrust.)

The plan is to write the documentation, likely in https://compat.spec.whatwg.org/. I've filed https://github.com/whatwg/compat/issues/6 pointing back here.
It looks like the w3c draft on "appearance" & its "none" value [1] may approximately match the behavior of -webkit-appearance:none.

Notable quotes:
 "All decorative visual aspects of a form control which are not caused by a CSS style rule must be suppressed when the appearance is changed using appearance"
 "However, the UA must preserve aspects of the form control which are necessary to operate the control with its original semantics." (An example given lower down: the slider on <input type="range">

 "This does not include aspects of a control which are merely needed to observe the state" (An example given lower down: the check on a checkbox)

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


Depending on how OK we are with the current spec text there "appearance" (not sure; would need to check with dbaron), it's possible we want to migrate "-moz-appearance" to more closely match the spec, or perhaps introduce unprefixed "appearance" as new & different property which more closely matches the spec. (and have -webkit-appearance as an alias for that)
What about appearance:inherit;?

W3C says: 'normal' resets 'appearance' to 'normal' and the others to 'inherit'.
Value:	normal | <appearance> | inherit
http://www.w3.org/TR/2004/CR-css3-ui-20040511/#appearance0

So, if provided value (none) not supported - appearance should be inherited from parent element. Am I right?

http://jsfiddle.net/iegik/b7z5b6dy/
> So, if provided value (none) not supported - appearance should be inherited from parent element.
> Am I right?

The computed value of the appearance property should be inherited, yes.

In Firefox right now, the initial value of "-moz-appearance" is "none" -- so that's what's getting inherited (from the <p>) in your example. (Checkboxes get their native theming via a "-moz-appearance: checkbox" line in an internal browser stylesheet:
http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=662737a36e01&mark=504-504#502
... and your "-moz-appearance: inherit" line is overriding that with the "none" value inherited from the <p>.)

But in the current CSS-UI-4 draft (linked in comment 33), the "appearance" initial value is actually "auto" (which just means whatever-the-default-behavior-is), not "none".  So if that part of the spec sticks around (& we implement it), then that's the value we'd be inheriting from the <p>, and your jsfiddle would render with a natively-themed checkbox, just as if you hadn't provided any CSS. And that probably makes sense.
(Sorry, I mis-parsed the question, and answered a slightly different question than the one you asked, I think.  What I thought you were asking was: "Does '-moz-apperaance: inherit' in my jsfiddle mean the -moz-appearance value should be inherited, and why is that making my checkbox unthemed")

I'm not sure what you mean about the "none" value not being supported. The "none" value *is* supported for -moz-appearance; it just behaves differently in different browsers right now.
About http://jsfiddle.net/iegik/b7z5b6dy/1/ - I`m asking: What is the difference between "none" and "inherit" value on -moz-appearance style property?

W3schools answered:
> The inherit keyword specifies that a property should inherit its value from its parent element.
http://www.w3schools.com/cssref/css_inherit.asp

MDN answered:
> The inherit CSS-value causes the element for which it is specified to take the computed value of the property from its parent element. It is allowed on every CSS property.
https://developer.mozilla.org/en-US/docs/Web/CSS/inherit

If in my case `input[type="checkbox"]` should inherit properties from `p` element - why I see border on checkbox, when there is no border on `p` element?

I think trouble is here:

/* forms.css:519; */
input[type="radio"],      /* here should be something like ::-moz-radio-wrapper */
input[type="checkbox"] {  /* here should be something like ::-moz-checkbox-wrapper */
  ...
  padding: 0 !important;
  ...
  background-color: -moz-Field ! important;
  color: -moz-FieldText ! important;
  border: 2px inset ThreeDFace ! important;
}

- why "!important" is here?!!
(In reply to Artūrs J. from comment #37)
> About http://jsfiddle.net/iegik/b7z5b6dy/1/ - I`m asking: What is the
> difference between "none" and "inherit" value on -moz-appearance style
> property?

Arturs, "inherit" simply inherits the CSS value, which in this case is "none"; so they're equivalent, in your jsfiddle. If the <p> had a different "-moz-appearance" value, then that value would be what's inherited (though the checkbox may or may not actually render differently with other "-moz-appearance" values).

In the interests of keeping this bug focused & not having too many side-discussions (which make bugzilla pages unreadable): if you have further questions on this behavior, please ask on IRC in irc.mozilla.org #developers or on the https://lists.mozilla.org/listinfo/dev-tech-layout mailing list.
Would anyone be opposed to a patch that removes a few of these !important statements mentioned in comment #37? As far as I understand it, they are only there for outdated IE compatibility reasons mentioned in comment #2.
If you're willing and able to defend (as part of the code review process) the argument that they're no longer needed for compatibility, sure.  (That probably means showing the changes leave Gecko within the existing range of cross-browser behavior.)
See Also: → 1246836
Since webkit-prefixed properties were activated on Aurora (bug 1213126), the checkboxes on www.captaintrain.com/search started to break.

This is because we use feature-detection to style the checkbox properly:

- With "webkit-appearance:none + custom background and border" on Webkit,
- With a custom label on other browsers.

Now that Firefox reports it support WebkitAppearance, our webapp uses the webkit-code-path – but fails to style the checkbox properly, because the borders and background are not themable on Firefox.

If it is possible to either:

- Not claiming support for WebkitAppearance yet,
- Or fix this bug,

that would be great :)
Thanks Pierre, that's good feedback. WebKit prefix support won't leave Aurora until we're confident we're doing it right--so your users on Beta and Release will be OK. 

We should probably consider removing the -webkit-appearance alias until we understand all the issues (and ideally match on behavior).

Would you mind filing a new bug and cc'ing me? I looked at https://www.captaintrain.com/ but don't see any checkboxes, so some steps to reproduce would be good.
Flags: needinfo?(kemenaran)
Blocks: 1248975
(In reply to Mike Taylor [:miketaylr] from comment #42)
> Thanks Pierre, that's good feedback. WebKit prefix support won't leave

> Would you mind filing a new bug and cc'ing me? I looked at
> https://www.captaintrain.com/ but don't see any checkboxes, so some steps to
> reproduce would be good.

See bug 1248975
Flags: needinfo?(kemenaran)
I've tested this on the latest Firefox, Firefox Beta and Firefox Developer Edition. The issue is reproduce-able on all of them. Setting '-moz-appearance: none' for a `<select type="checkbox">' does not have any effect.
(In reply to raees.bhatti from comment #44)
> I've tested this on the latest Firefox, Firefox Beta and Firefox Developer
> Edition. The issue is reproduce-able on all of them. Setting
> '-moz-appearance: none' for a `<select type="checkbox">' does not have any
> effect.

Correction: Element is <input type="checkbox">
In Chromium, it appears that -webkit-appearance:none makes checkboxes and radios be non-replaced elements.  They're then display:inline-block by default, but can be changed to display:inline.

We should probably just do this.
Summary: -moz-appearance:none doesn't completely work on checkboxes and radios → -moz-appearance:none should make checkboxes and radios be non-replaced elements
(The CSSWG just resolved that "appearance: none" should turn checkbox & radio <input> elements into a normal non-replaced element: http://logs.csswg.org/irc.w3.org/css/2016-05-10/#e683564 )
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
You can see some tests on http://lanmarket.ua/
Whiteboard: [webcompatibility] → [webcompatibility] [webcompat]
This is needed for compatibility in the admin interface for Magento which uses the :after pseudo element to display checkmarks.
David, are you working on this or have WIP patches that someone could take over?
Flags: needinfo?(dbaron)
Blocks: 1333482
My work in progress is the following 2 patch queue:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b8732eb7d576/backstop-theme
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b8732eb7d576/appearance-checkbox-radio-non-replaced

I think the interesting part is writing the first patch (currently empty), so that we can remove most (?) of the checkbox and radio styles from the UA sheet.  But I haven't investigated what other engines do.
Flags: needinfo?(dbaron)
(and if somebody else wants to take it, that would be great)
Thanks, I'll take a look...
Assignee: dbaron → mats
As you can see, I'm leaving Android platforms as is.  The reason is that this platform
doesn't have any native theming support so the styling is the same regardless of
'-moz-appearance' value.  This is unfortunate because I doubt it's web-compatible
(although I haven't tested what other browsers do on Android).  It's an already
existing issue though and these patches shouldn't make it worse.
(In reply to Mats Palmgren (:mats) from comment #60)
> This is unfortunate because I doubt it's
> web-compatible (although I haven't tested what other 
> browsers do on Android).  It's an already
> existing issue though and these patches shouldn't make it worse.

Thanks mats
I need to put on my todo list the appearance stuff. I have tested 
a couple of things already, but I need a better set of tests.
Karl, FYI, I've tested Chrome, Edge, and Safari on desktop pretty thoroughly
already using the test: https://people-mozilla.org/~mpalmgren/tests/appearance.html
The intention is to unship all values except 'none', in bug 1333482.
https://github.com/w3c/csswg-drafts/issues/802

Always happy to have more testing though, especially on mobile...
yes I was talking about mobile, sorry for not being clear.
Most of the webcompat issues are on mobile and some of them will be difficult to tackle, because designers have specifically targeted firefox.

<3 for https://people-mozilla.org/~mpalmgren/tests/appearance.html
Thanks.
(In reply to Mats Palmgren (:mats) from comment #62)
> Karl, FYI, I've tested Chrome, Edge, and Safari on desktop pretty thoroughly
> already using the test:
> https://people-mozilla.org/~mpalmgren/tests/appearance.html

FWIW: in a mozilla-central debug build on my machine (Ubuntu 16.10), this ^ page triggers an abort:
 "Assertion failure: rect->width >= indicator_size (GetMinimumWidgetSize was ignored), at widget/gtk/gtk3drawing.cpp:337"

Does that happen for you? (maybe you're not on Ubuntu?)
Flags: needinfo?(mats)
Comment on attachment 8834700 [details] [diff] [review]
part 1 - Remove most default styling for checkbox and radio buttons.

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

Commit-message nit:
> Bug 605985 part 1 - Remove most default styling for checkbox and radio buttons. r=dholbert
This description makes it sound like the patch is deleting code, but that's not what the patch does (it only removes a few lines & mostly adds a bunch).  I suspect that you intend the word "remove" to refer to the %ifdeffing that you're adding -- right? If so, perhaps "disable" (or "ifdef away") would be clearer here...?

Also, this patch includes C++ changes to reduce the intrinsic size to 0, for unthemed checkbox/radios.  That seems worth calling out in the commit message (though maybe not in the first line, if you don't feel like it's the main focus).

::: layout/forms/nsFormControlFrame.h
@@ +112,5 @@
> +  {
> +    // XXXmats We have traditionally always returned 9px for GetMin/PrefISize
> +    // but we might want to factor in what the theme says, something like:
> +    // GetMinimumWidgetSize - GetWidgetPadding - GetWidgetBorder.
> +    return nsPresContext::CSSPixelsToAppUnits(9);

I'm not sure it's worth exposing this in the .h file (and cluttering up the class definition) -- we could just as easily make it a file-scoped static function in the .cpp file.  But, not a big deal.

::: layout/style/res/forms.css
@@ +568,5 @@
> +input[type="checkbox"] {
> +  box-sizing: border-box;
> +  cursor: default;
> +  padding: initial;
> +  -moz-binding: none;

Two things:
 (1) For "padding" and the other properties that follow, consider using "unset" instead of "initial". That way, you can reset inherited & non-inherited properties using a consistent keyword.

 (2) For -moz-binding, probably best to use "initial"/"unset" (whichever you choose) for consistency with the properties around it.  (to make it clearer that you're resetting it, rather than giving it a special value).
(NOTE: Maybe that adjustment should happen in a followup patch here, since I'm now noticing that these resetting styles are mostly moved-without-modification from anther spot in this file.)

 (3) Please include a comment before all of these "initial" (or "unset") styles to explain *why you need to reset them*.  (You're just setting the default values, so it's non-obvious why you even need to bother.)  After poking around a bit, I *think* they're all aiming to override declarations on a more general "input {" rule that exists elsewhere in this file....? If that's the idea, please mention that here.

@@ +581,5 @@
> +input[type="checkbox"]:disabled,
> +input[type="checkbox"]:disabled:active,
> +input[type="checkbox"]:disabled:hover,
> +input[type="checkbox"]:disabled:hover:active {
> +  cursor: inherit;

(possible-followup-fodder):  Per my comment above, "unset" would probably be clearer here. ("cursor" is inherited, so "unset" & "inherit" are equivalent) That makes it clearer that your intent is to *undo* the effects of some rule from elsewhere in this file, rather than e.g. trying to make some normally-non-inherited property inherit through to this element.  And it makes it clearer that this intent is the same between this "cursor" style & the "border"/"padding"/etc styles above, if they're all using the same keyword to do their resetting.

Though per my NOTE above, this might be better to punt to a followup patch so that this patch remains largely code-moves.

@@ +584,5 @@
> +input[type="checkbox"]:disabled:hover:active {
> +  cursor: inherit;
> +}
> +
> +%if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

Probably don't bother including MOZ_WIDGET_GONK here -- I don't think that configuration is tested or supported at this point, so we shouldn't be adding new checks for it.

@@ +586,5 @@
> +}
> +
> +%if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
> +/*
> + * These platforms doesn't have any theming support and thus appearance:none

(And then if you remove the Gonk check above, "These platforms" should probably collapse to "Android" here.)

@@ +662,5 @@
>  input[type="checkbox"]:indeterminate:disabled {
>    background-image: url(indeterminate-checkmark.svg#disabled);
>  }
>  
> +%endif

This is pretty far from its %ifdef, so you should probably include a comment of some sort (on the same line or the next line) to hint at what it's endiffing.

e.g. I think this'll work:
%endif /* defined(MOZ_WIDGET_ANDROID) */

(Pretty sure you can use /**/ comments on %endif lines, based on e.g. usage here:
https://dxr.mozilla.org/mozilla-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/browser/themes/shared/customizableui/customizeMode.inc.css#178
)
Attachment #8834700 - Flags: review?(dholbert) → review+
Comment on attachment 8834701 [details] [diff] [review]
part 2 - Make -moz-appearance:none on radio and checkbox inputs make them non-replaced elements.

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +3686,5 @@
>    NS_ASSERTION(control, "input doesn't implement nsIFormControl?");
>  
> +  auto controlType = control->GetType();
> +
> +  // Note that Android/Gonk widgets don't have theming support and thus

As noted for the other patch, probably better not to add new references to MOZ_WIDGET_GONK in this comment & in the #if below this.

(after looking at the contextual code, I suspect you're striving for consistency with some preexisting ANDROID||GONK checks earlier in this function.  If that's the goal, it's perhaps better to just drop gonk from the existing check, rather than adding new untested gonk checks.  But this is fine too, I guess.)
Attachment #8834701 - Flags: review?(dholbert) → review+
Comment on attachment 8834702 [details] [diff] [review]
part 3 - The expected result for all checkboxes and radios with -moz-appearance:none is about:blank now (except on Android).

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

::: layout/reftests/forms/input/checkbox/reftest.list
@@ +3,3 @@
>  != checked-native.html checked-native-notref.html
> +fails-if(Android) == checked.html about:blank
> +fails-if(Android) == checked-notref.html about:blank

Drive-by nit here: the updated manifest-expectations here are really confusing/surprising.

In particular:
 (1) We have these two files which we now expect to render as blank, but their names ("checked") sound very much non-blank.
 (2) The "notref" naming scheme implies that checked.html and checked-notref.html should *not* render the same. However, we'll now be asserting that they *DO* render the same. (since we're asserting that they both match the same thing)

It'd probably be worth renaming these files to make the new expectations clearer.  E.g. rather than "checked.html" and "checked-notref.html", maybe they should be renamed to "appearance-none-checked-1.html" and "appearance-none-unchecked-1.html"?

Also, before landing, you should add some new reftests that more directly test the new rendering behavior here -- e.g. styling <input type="checkbox" style="-moz-appearance:none"> as a green square, or something like that (and perhaps with different styling depending on whether the "checked" attr is set or not, like an actual site would do).
(I spun off bug 1337960 for comment 64; never mind about that for the purposes of this bug here, since it doesn't seem to rely on <input> elements.)
(In reply to Daniel Holbert [:dholbert] from comment #65)
> > +%if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
> 
> Probably don't bother including MOZ_WIDGET_GONK here -- I don't think that
> configuration is tested or supported at this point, so we shouldn't be
> adding new checks for it.

Fwiw, I asked about the status for widget/gonk in bug 1306555 comment 1,
but I got no reply.  I've asked Jet to sort that out.
(In reply to Daniel Holbert [:dholbert] from comment #65)
> > +input[type="checkbox"] {
> > +  box-sizing: border-box;
> > +  cursor: default;
> > +  padding: initial;
> > +  -moz-binding: none;
> 
> Two things:
>  (1) For "padding" and the other properties that follow, consider using
> "unset" instead of "initial". That way, you can reset inherited &
> non-inherited properties using a consistent keyword.

Can't use 'unset' for 'cursor' since that would be a functional change.
We don't want to display a 'wait' cursor over the checkbox here:
data:text/html,<body style="cursor:wait"><input type=checkbox>

But it's a good point that 'unset' makes the intent clearer in general.
I filed bug 1338158 to follow-up on that.

> > +input[type="checkbox"]:disabled,
> > +input[type="checkbox"]:disabled:active,
> > +input[type="checkbox"]:disabled:hover,
> > +input[type="checkbox"]:disabled:hover:active {
> > +  cursor: inherit;

I'm not sure which of unset/inherit is clearer in this case though,
so I left it as is.  Let's sort that out in bug 1338158.
(it's not so much that we want to reset the value in this case, but
more that we want to actively inherit the value)

> Probably don't bother including MOZ_WIDGET_GONK here

I kept it for now until we know for sure if GONK should stay or not.
Flags: needinfo?(mats)
Blocks: 1338293
(In reply to Daniel Holbert [:dholbert] from comment #67)
> It'd probably be worth renaming these files to make the new expectations
> clearer.

OK, I renamed them.

> Also, before landing, you should add some new reftests that more directly
> test the new rendering behavior here -- e.g. styling <input type="checkbox"
> style="-moz-appearance:none"> as a green square, or something like that

I suspect we have reftests for this, but I added a few more variations to
layout/reftests/forms/input/checkbox/checkbox-baseline.html just in case.

Also, while doing that I discovered that we have a lingering problem with
'color' that affects border/outline colors.  That's an existing bug though,
and requires functional changes, so I filed it separately as bug 1338293.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad4365c7898
part 1 - Remove most default styling for checkbox and radio buttons and make the instrinsic size be zero for -moz-appearance:none checkbox/radios.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c62dffd917
part 2 - Make -moz-appearance:none on radio and checkbox inputs make them non-replaced elements.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/db62e3ac853f
part 3 - The expected result for all checkboxes and radios with -moz-appearance:none is about:blank now (except on Android).
Doesn't part 1 of this series without doing comment 53 cause a checkbox or radio that has border or background styles specified:
 * no longer have a border (if background specified)
 * no longer show a checkbox/dot when checked
given that IsWidgetStyled will return true and we'll thus disable the native-theme, but that we have no drawing of the border or checkmark/dot other than in the native theme now?

Is that actually what other implementations do?
Flags: needinfo?(mats)
Attached file testcase for comment 73 (obsolete) —
Does this test the cases you're concerned about?  If so:

(In reply to David Baron :dbaron: ⌚️UTC-5 (busy February 1-10) from comment #73)
> Doesn't part 1 of this series without doing comment 53 cause a checkbox or
> radio that has border or background styles specified:
>  * no longer have a border (if background specified)

Correct.  This is also what Edge and Chrome does.

>  * no longer show a checkbox/dot when checked

Correct.  This is also what Edge and Chrome does.
Additionally, this is also required by the spec now:
"... the check mark in an <input type=checkbox checked> must be suppressed ..."
Flags: needinfo?(mats)
No, that doesn't reflect what I was asking since it uses 'appearance: none'.
Flags: needinfo?(mats)
Ah, sorry about that.  Is this the case you're thinking about?
It doesn't seem to have any effect at all, with or without these patches.

Same in Edge and Chrome.  (Edge renders a green background outside the circle
of the radio control though, but that seems more like a bug to me.)
Attachment #8835690 - Attachment is obsolete: true
Flags: needinfo?(mats)
OK, I guess there's some code that I can't find right now that prevents border and background styles from disabling native theming for checkboxes and radios specifically.  (I remembered something like that, but didn't see code for it.)
FTR, I'm tested Firefox on Linux above.  I'll check other platforms when I get
a chance, just in case it's platform dependent...
It seems to be a white-list of widgets that we check:
http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/widget/nsNativeTheme.cpp#344-349
(plus a few more special cases before that), but checkbox/radio isn't included.
(this is on file as bug 716550, which I'll close as invalid, because changing
it would be incompatible with Edge/Chrome - for both background & border)

FTR, I checked a build with the patches on Win10 just in case and it worked as expected.
Blocks: css-ui-3
Flags: in-testsuite+
See Also: → 1340661
Depends on: 1344257
Blocks: 1344395
No longer blocks: 1344395
Depends on: 1344395
So I'm a little disturbed that we're making significant changes to radio and checkbox stylability in two successive releases (bug 418833 in Firefox 53, this in Firefox 54).  I think that reduces the amount of testing that we get on the intermediate state.

Is it worth either pushing this ahead into Firefox 53 (beta) so they both ship in 53, or backing bug 418833 out of beta so they both ship in 54, to reduce the amount of churn we cause developers in a single area?
Flags: needinfo?(mconley)
Flags: needinfo?(mats)
Bug 1320732 was backed out in bug 1345956 and it looks like it will be backed out on v53 too.

Given that bug 418833 has a few unresolved regressions, I'd recommend backing it out
on v53 and addressing those in v54.  I don't think uplifting this bug to v53 will help
with those.

Fwiw, I made a proposal on how to make Android checkbox/radio controls web-compatible
in bug 1340661 comment 33.
Flags: needinfo?(mats)
(BTW, the changes I've made here should be idempotent for Android so it's not really
related to the other bugs IMO.)
Summary: -moz-appearance:none should make checkboxes and radios be non-replaced elements → -moz-appearance:none should make checkboxes and radios be non-replaced elements (except on Android)
(In reply to Mats Palmgren (:mats) from comment #85)
> Given that bug 418833 has a few unresolved regressions, I'd recommend
> backing it out
> on v53 and addressing those in v54.  I don't think uplifting this bug to v53
> will help
> with those.
> 

Backing this out on v53 will require other shifts, as some of the front-end checkbox styling in about:crashes, about:networking, about:debugging and about:neterror rely on it. I would really rather we keep it in v53 if at all possible.

What are these unresolved regressions you're referring to? Have I missed some? The only one I know about is bug 1345956, which I'll be landing on aurora and beta once I get approval, which re-introduce the !important on the radio / checkbox borders on Android.

Beyond that, are there regressions against bug 418833 that I need to know about, and if so, why aren't they marked blocking that bug?
Flags: needinfo?(mconley) → needinfo?(mats)
I'm referring to the unresolved "depends on" bugs on bug 418833.  I assumed there were
regressions that affected web content.  Given the discussion in bug 1340661 I figured it
might be better to address checkbox/radio on Android in a proper way for v54 instead.
Otherwise, I see no problem with uplifting this bug to v53 per se.
Flags: needinfo?(mats)
Here's the situation as I understand it:

Bug 418833 causes web content regressions which are layout bugs, but only on Android.
Our checkbox/radio implementation on Android has never been web compatible though.

This bug may cause web content regressions, but the layout is considered correct and
makes us compatible with other UAs on all platforms except Android which is still
incompatible with other UAs.

Uplifting this bug to v53 doesn't help Android, but as dbaron suggested, it may allow
web authors to address any fallout once instead of twice.  Backing out bug 418833 from
v53 will do the same but is trickier since we now have UI that depends on those changes.

We know that we will have to make checkbox/radio changes on Android to make them web
compatible at some point (implementing a simple native theme for just those two
controls as I suggested in bug 1340661 comment 33), and this change may also cause
some web site regressions that web developers may need to adjust to.

There's also bug 1333482 which implements -webkit-appearance/appearance:none which
I'm still planning to uplift to v54.  I guess we could uplift it to v53 too,
but that's a lot riskier and there's a lot of moving parts here.  (The plan there
is to enable all three -moz-appearance/-webkit-appearance/appearance initially and
then later drop -moz-appearance.)

My suggestion was based on causing minimal harm to web developers: backout 418833 on
v53 and implement the theme for Android in v54.  Then we'll only cause changes once
(in v54 this time), but the layout is web compatible also on Android this time.

Option A:
Ideally, we would have all these changes in the same release.  Then there's only
one big change which makes us compatible with other UAs on all platforms so there
shouldn't be a need for web devs to adjust styling again after that.

Option B:
Alternatively, keeping 418833 in v53 and *not* uplifting this bug to v53 only affects
web content on Android, which is already "broken" anyway.  (Desktop Firefox isn't
affected by those changes as far as I know, please correct me if I'm wrong.)

Option C:
Alternatively, keeping 418833 in v53 + uplifting this bug to v53 affects web content
on all platforms (Firefox + Fennec).

I vote for A, in v54.  I think B is OK and better than C, because for desktop
Firefox there will only be one big change in v54 with B (if we ignore Android,
it's the same as A).

(BTW, Mike, are you planning to take on implementing the native theme for Android?
and if so in what time frame?)

Thoughts?
Flags: needinfo?(mconley)
Flags: needinfo?(dbaron)
It's not clear to me how option (C) differs from the A-in-53 variant.  (Perhaps A is just A-in-54 and C is A-in-53?  Maybe it doesn't matter.)

That said, if the effects of what we're shipping in 53 are Android-only, then I'm much less concerned.  If that were the case, I wouldn't expect many Web developers to notice.

I think the people who might notice are our own Web compat team, and I think if we shipped Android-only changes in 53, and then all-platform changes (including Android) in 54, we'd probably want the Web compat team not to bug people about any problems in 53.  (Certainly not to bug people about problems in 53 that go away in 54, and maybe even to wait to bug developers about problems present in 53 that change in some way in 54.)  So I'd probably want some buy-in from the Web compat team if we were going to do that.

But I'm not sure I have the state of all the patches correct.  Do the changes in 53 affect desktop, or are they purely Android-only?
Flags: needinfo?(dbaron)
I explained the web compat concerns in these two emails and are about appearance: none on select elements.
Most of the issues we have are related to Firefox on Android.

https://groups.google.com/forum/#!msg/mozilla.dev.platform/oZ9cPF8Y1pE/LiSUtpeHBwAJ
https://groups.google.com/d/msg/mozilla.dev.platform/oZ9cPF8Y1pE/5Sxjz2a8CAAJ

(I understand this issue is about -moz-appearance:none should make checkboxes and radios)

On Desktop about -moz-appearance: none and checkbox
https://webcompat.com/issues/4177

On Android about -moz-appearance: none and checkbox
https://webcompat.com/issues/3777


I don't think, there will be issues on our side. 
Adding ni for Mike Taylor
Flags: needinfo?(miket)
> It's not clear to me how option (C) differs from the A-in-53 variant.

A-in-53 would support -webkit-appearance (bug 1333482), C would not.
Otherwise the same.

> Do the changes in 53 affect desktop, or are they purely Android-only?

The only change I have in v53 so far is bug 1322698, which fixes baselines
for appearance:none checkbox/radio, but as usual we didn't fix it for Android:
https://hg.mozilla.org/mozilla-central/rev/c718681647c7
(fixing these things for Android requires a native theme for checkbox/radio
and stop using appearance:none by default)

That's a fairly minor change though, so I'm not worried about letting that
one ride separately from everything else.

mconley's changes are Android-only, AFAIK.
Sorry for my delay in responding here.

IMO, (A) is most desirable. The issue with bug 418833 and Android has been fixed by backing out bug 1320732 (which happened in bug 1345956). Because bug 1320732 is backed out, this means that the border around checkboxes and radio inputs on Android cannot be overridden. This is always the way it's been though, so I don't think that's a problem.

I agree that implementing a native nsITheme for Android is what we truly should be doing here, but making sure these inputs were visible was of far greater importance (and I wasn't confident in my ability to ship a native theme implementation in time to uplift for 53).

So the stuff I was dealing with had only to do with Android, and it's been "fixed" where necessary by way of keeping our current shipping behaviour.

I'll unfortunately add another wrinkle - I have a high degree of suspicion that this bug caused bug 1339775, which makes checkbox/radio's with (-moz-)appearance: none to not get the right accessibles created for them, essentially making them invisible to users of the accessible APIs. I've hooked up the dependencies.

The patch currently in bug 1339775 addresses the reported issue for our in-content UIs, but doesn't solve the more general issue on the web for those appearance: none checkboxes. Bug 1349835 has been filed for that work.

Sorry if I've muddied the waters even more here. :/
Blocks: 1339775
Flags: needinfo?(mconley)
(In reply to Mats Palmgren (:mats) from comment #89)
> 
> (BTW, Mike, are you planning to take on implementing the native theme for
> Android?
> and if so in what time frame?)
> 

Whoops, missed this part.

I'd _like_ to do this, and I have the beginnings of a patch, but because bug 1345956 landed, the urgency is somewhat lessened (and tipped more towards Photon and Quantum work).

I'll happily give what I've got up if someone else wants to take it on. I already had a preliminary conversation with snorp, btw, and he's of the impression that it should be straight forward to use the actual native checkboxes and radios from Android instead of drawing our own, which would be a platform integration improvement for Android in general.
Also agreed A sounds like the best.

(And bonus points for the native theme).
Flags: needinfo?(miket)
So, to execute option A, we should backout bug 418833 and dependent bugs from v53.
Mike, will you take care of that?
Flags: needinfo?(mconley)
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #90)
> That said, if the effects of what we're shipping in 53 are Android-only,
> then I'm much less concerned.  If that were the case, I wouldn't expect many
> Web developers to notice.

Unfortunately that is not quite so.  Bug 418833 actually affects all platforms,
but since all platforms except Android have a native theme they are not affected
by those changes *unless* the author specifies -moz-appearance:none.  In that
case they are affected, because we now use background image styles:
https://dxr.mozilla.org/mozilla-beta/rev/d0f0b8b69e9a826ed266314cf1cfbde8e53438c4/layout/style/res/forms.css#616-646
rather than hard-coded drawing of the check marks in C++ code.
So if the author specifies a background then there will be a regression v53
(also on desktop), which is bug 1328474.  On Android though, since it has
-moz-appearance:none by default, the regression occurs by just setting
a background, IIUC.  This seems like a rather bad regression on Android.

:mconley, or anyone, please correct me if I've misunderstood the situation.
We need to get relman involved. A backout of this stuff in 53, despite going back to the original behaviour, is still a pretty big change involving multiple landings straight on to beta, and we're only a few weeks away from release.

For lizzard, here's what's going on[1]:

Bug 418833 has ridden the trains up to 53. It changes some behaviour on how checkboxes and radio inputs look and feel. A few things landed in the front-end that depend on that change (specifically, bug 1317795).

In this bug, mats landed a patch that changes checkboxes and radio inputs even more.

In comment 83, dbaron suggested that having the first change (bug 418833) in 53, and this change (bug 605985) in 54, is a bit risky because of how little testing we're getting in the intermediate state.

In bug 1328474 comment 15, mats has identified an additional web compat regression that will affect web authors and how checkboxes and radios display. He notes that ideally, we will land all patches together at once.

So we're in a state where we either want to land all of those patches to 53, or back that changes that are already in 53 _out_, and aim for 54 instead.

If we _did_ do the backout on 53, we'd need to do what we did for 52 , which bug 418833 did not land in time for. See bug 1317795 comment 30 for that history, but essentially:

1) Backout bug 1317795 from 53
2) Backout bug 418833 from 53
3) Land bug 1321302 on to 53 (since we need to wallpaper over the lack of bug 418833)
4) Land bug 1321306 on to 53 (also since we need to wallpaper over the lack of bug 418833)

I'm of the opinion that the backout on 53 is the safest option, since it's (I believe) putting us back into the state we've been shipping in 52 and under.

Do we do the backouts / wallpaper landings?

mats, feel free to chime in if I've represented the situation incorrectly, but this is how I see it.

[1]: And sorry this is so late breaking. :(
Flags: needinfo?(mconley) → needinfo?(lhenry)
(In reply to Mike Conley (:mconley) from comment #98)
> So we're in a state where we either want to land all of those patches to 53,
> or back that changes that are already in 53 _out_, and aim for 54 instead.

Just want to note that even if we uplift all patches to v53 it would only fix
Firefox -- we would still have regressions in Fennec (as noted in bug 1328474).
We would need to write a native theme for Android (new C++ code) to avoid that,
which seems unrealistic for 53.

I agree that the suggested backout on 53 is the safest option, and I don't think
we have much choice because we don't know how widespread the regressions in
bug 1317795 are.  We also need more time for testing this bug and bug 1333482
since these just recently landed.
Backout in 53 sounds good. Can you request beta uplift on bug 1321302 and 1321306?  
And we might want to open new bugs for the backout (and note that they happened in comments on the originals) to make things easier to track. 

Thanks for the helpful summary!
Flags: needinfo?(lhenry)
Filed bug 1352406 for the backouts. Some slight variations were required see bug 1352406 comment 0. I'm currently doing a local build to test, and then will push to try.
Hi there!

I've had a go at documenting this. First of all, I've made some changes to the general info on the non-prefixed ref doc, and the support info footnotes and some of the general info on the prefixed ref doc:

https://developer.mozilla.org/en-US/docs/Web/CSS/appearance
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance

These could do with a general tech review.

I've also updated the Fx54 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/54#CSS

Let me know if this reads OK. Thanks!
Depends on: 1373417
Hi there, I have been using -moz-appearance: none; in order to use my own custom styled check boxes, which Firefox was basically ignoring unless I used this.

With this bug "fix", I now have nothing displaying, instead of my custom styled checkboxes.
If I remove the "-moz-appearance: none;" then instead I get the default small checkboxes, instead of my styled ones.

So it seems like this is a step backwards... Or I am missing something.

I am simply changing the width and height to 30px. How is one supposed to achieve this now?
Sorry to hear that! It's hard to say what's going wrong, without a concrete testcase or URL that shows the brokenness / behavior difference.

Rather than trying to diagnose it here in comment 100+, could you file a new bug at https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout and include a testcase or a link to a broken page? (and put :dholbert and :mats in the CC field, and/or mention the bug number here.)

Thanks!
This is huge, congrats to everyone who contributed. Checkbox hacks no more. Can someone please post a link to the bug ticket for android that covers this same problem?
I believe bug 1352238 covers this problem on Android (and some other related work).
Flags: needinfo?(zemuslug)
Flags: needinfo?(zemuslug)
Depends on: 1431541
Depends on: 1435658
You need to log in before you can comment on or make changes to this bug.