Closed Bug 418833 Opened 16 years ago Closed 7 years ago

can't define the style of input when the type is set to "checkbox" or "radio" with CSS

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- disabled
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: skasski, Assigned: mconley)

References

()

Details

(Keywords: css1, dev-doc-needed)

Attachments

(11 files, 2 obsolete files)

1.24 KB, text/html
Details
63.86 KB, image/png
Details
58 bytes, text/x-review-board-request
tnikkel
: review+
Details
58 bytes, text/x-review-board-request
tnikkel
: review+
Details
965 bytes, text/html
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details
1.27 MB, image/png
Details
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Firefox does not show the background-color, borders, padding and much more things when I use CSS with <input>s of type "checkbox" and "radio".

Reproducible: Always

Actual Results:  
The checkbox or radiobutton does not change the style.
The only things I found, that I can define are margin and the height/width.

Expected Results:  
I would like to be able to define a background-color/image.
The borders should be able to be defined as outset or double, green or blue, 1px or 5em, etc.
The padding-function and other things sould be fixed too.
Keywords: css1
same on linux.
Even Internet Explorer 8 Beta 2 supports the border property on checkboxes. Opera looks the best while WebKit lacks this (it's implemented as a blue crystal style in Safari). This has my vote, Gecko is falling behind the times.
Confirmed in Firefox 3.1b4 - Windows XP SP3
Confimed on:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090430 Minefield/3.6a1pre
Confirmed on:

Firefox 3.0.10 - Windows 7 RC1 Build 7100
Confirmed on Firefox 3.6.8.
It's time this bug is fixed, along with issues like Bug 52500. As said above, Gecko is lacking behind with these extreme old issues.
Could this issue be linked with the fact that the default forms.css has alot of !important statements in it, and you are thus unable to alter these properties?
Still present on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Confirmed on 4.0 (Linux)
Still present on Firefox 8 (Linux).  A few properties can be set, but there is no way to get rid of the 2px border.  Even an inline style="border:1px solid red !important;" has no effect (it does work in WebKit and Opera).
Still present on Firefox 12.0 on Mac, Linux and Windows.

It appears to be these that are causing the issues:

<system> forms.css:473
input[type=radio], input[type=checkbox] {
background-color: -moz-field !important;
border: 2px inset threedface !important;
}

The only way to style a checkbox with a different background/border is to add a background image now. However, when it is CHECKED, the black tick still appears in the middle of the image.

I'm pretty sure this bug has to be confirmed by now after 3 years?
I confirm this bug too at Firefox 19.0.2 with the provided url.

see also bug 605985 which seems to be relative.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?
Severity: enhancement → minor
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 19 Branch
Version: 19 Branch → unspecified
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?
Presumably this is just a -moz-appearance issue? See https://developer.mozilla.org/en-US/docs/CSS/-moz-appearance
Component: General → Style System (CSS)
Flags: needinfo?
Product: Firefox → Core
This bug is still present in Firefox 37 & 38 on Linux and Windows 7 (And Mac presumably).
This bug is... really unfortunate at the moment. Any plans on fixing this really old issue?
I made an example jsfiddle so that you can compare how it looks in chrome/safari vs firefox: http://jsfiddle.net/go392m5s/.
I can confirm this is still present in the latest nightly 45.0a1. Thanks for the jsfiddle, beckie.
@beckiechoi don't forget to include indeterminate checkbox styling: http://jsfiddle.net/Mottie/go392m5s/12/
What's the required skill to fix this bug? I assume this is involves advanced XUL/XBL/Widget wizardry, but I might be wrong.


Neil, is this a question you could answer? :dcamp told me you should. Thanks!
Flags: needinfo?(enndeakin)
You would need to add something to the end of nsNativeTheme::IsWidgetStyled so that the call to aPresContext->HasAuthorSpecifiedRules can override the default theme.

That said, I think but am not sure that you get all or nothing when drawing native checkboxes, meaning you either get a button with or without a check in it and and if you disable the default rendering, nothing is drawn, so you get no checkmark either.

mstange is a better person to ask about native theming though.
Flags: needinfo?(enndeakin)
Markus, can you give us a little context on what would be needed to fix this bug?  Is overriding the native theme for checkboxes and radio buttons an all-or-nothing kind of thing?
Flags: needinfo?(mstange)
(In reply to Blake Winton (:bwinton) (:☕️) from comment #22)
> Markus, can you give us a little context on what would be needed to fix this
> bug?  Is overriding the native theme for checkboxes and radio buttons an
> all-or-nothing kind of thing?

Maybe you are talking about what should happen from a technical standpoint, but from a designer/developer expectation perspective then the expectation should be that once you start setting appearance: none on things you would lose any native styling.

This is the way it works in other browsers and in Firefox on other input elements. There are ways we could degrade more gracefully, but I don't think any of those should involve retaining system styling.
OK, so I was referring to using:

<input type="checkbox" style="border: 1px solid black;">

Some elements imply that the native theme is overridden when a custom border or background is applied to an element without having to specify the appearance.

moz-appearance/appearance is different and disables theming entirely. From testing, this appears to happen already with html checkboxes, but some other default appearance is being applied, probably from forms.css. It looks like the checkmark drawing is hard-coded in nsGfxCheckboxControlFrame.cpp when moz-appearance: none is used.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #22)
> Markus, can you give us a little context on what would be needed to fix this
> bug?  Is overriding the native theme for checkboxes and radio buttons an
> all-or-nothing kind of thing?

I think the first step would be to concretely define the intended behavior. What properties should turn off native theming? What should be the default look when native theming is off? Should we stop painting a checkmark as soon as somebody overrides a CSS property that turns off native theming?

As for the implementation, the main problem here is that nsGfxCheckboxControlFrame::BuildDisplayList calls nsFormControlFrame::BuildDisplayList and not nsIFrame::BuildDisplayList. The latter supports all CSS properties, the former only border and background.

Does this help?
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #25)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #22)
> > Markus, can you give us a little context on what would be needed to fix this
> > bug?  Is overriding the native theme for checkboxes and radio buttons an
> > all-or-nothing kind of thing?
> 
> I think the first step would be to concretely define the intended behavior.
> What properties should turn off native theming? What should be the default
> look when native theming is off? Should we stop painting a checkmark as soon
> as somebody overrides a CSS property that turns off native theming?

There are few ways we could handle it it:

1) Require *appearance: none to disable native styles

This is what Webkit/Blink and Edge do, and then they hide the radio or checkbox completely.

Pros: 
- Makes it harder to accidentally disrupt these elements by setting a style on input
- Forces full styling customization so people don't have to worry about where the checkmark or radio dot comes from
- Works like that in other browsers so web developer won't have to special case Gecko

Cons:
- No fallback styling makes this different than how styling other form elements works


2) Make it work like other input elements and have styling of any sort that affects the appearance revert to fallback(non-native) appearance: e.g. border or background

Pros:
- More graceful than just not showing anything
- Inline with our current approach (only styled with CSS instead of whatever static thing we are doing now)
- Matches how other input elements work with styling

Cons:
- Not the same as other rendering engines
- Adds the complexity of how to handle checkmarks and radio dots


A third option might be requiring appearance: none but still having a fallback appearance.
Attached file wip patch (obsolete) —
I have no idea how reviewboard works. Sorry if this doesn't work.
https://reviewboard.mozilla.org/r/60534

Tweets got me curious, but not sure I'll have much time to play with this. This removes the hardcoded checkmarks/circles and replaces them with svg versions. I also removed a bunch of !important rules that prevented overriding the styles. The net effect (I hope! Need to test harder) is to retain our old behavior (i.e. turning off -moz-appearance gives you the "old" style), but also allow you to override with your own borders/backgrounds/images if you want.

Webkit/Blink show nothing at all when you set -webkit-appearance: none, so this still doesn't match them. Haven't tested Edge/IE.

Curious what you think of this approach dbaron? Why were all these !important rules in here to begin with?
Attachment #8764974 - Flags: feedback?(dbaron)
The reviewboard review that you linked to is private. I'm not sure why (given that I can't see any details about it), so can you just attach a patch to the bug instead?
Flags: needinfo?(wjohnston2000)
Comment on attachment 8778718 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. f?dbaron

I believe this is the patch that wesj wants dbaron to give feedback on.
Attachment #8778718 - Flags: feedback?(dbaron)
Comment on attachment 8778718 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. f?dbaron

At least some of the !importants were added because there were sites that broke without them (i.e., you couldn't distinguish checked from unchecked or selected from unselected) because they styled both color and background-color on the controls the same.  That may not be an issue anymore.  That said, I'm not sure if it's these -- if you want to figure out why they were added, "git blame" on https://github.com/mozilla/gecko-dev/ is your friend.

What other feedback did you want?  What were you trying to accomplish here?
Attachment #8778718 - Flags: feedback?(dbaron)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #31)
> What were you trying to accomplish here?

I believe the goal is to have
<input style="-moz-appearance: none; -webkit-appearance:none; border: 4px solid blue;border-radius: 4px !important;width: 20px;height: 20px;color: green;" type="checkbox">
render with a 4px wide blue border (and ideally a border radius of 4px, although the patch doesn't seem to do that).  Currently it renders with a grey 3d border.
Flags: needinfo?(mconley)
Attached file Manual test case
Left is Blink / WebKit, middle is Gecko with attachment 8778718 [details] applied, right is Gecko as it stands today.
Hey dbaron,

So I think what's hoped for here is WebKit / Blink parity on styling these elements. See the attachments I just tossed up. The patch in this bug gets us quite a bit closer, but we're not exactly there yet (looks like at least padding isn't working correctly).

Do you know what would be required to get us at parity here? Would it be a simple-ish modification to the wesj's patch?
Flags: needinfo?(mconley) → needinfo?(dbaron)
Gentle poke.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #32)
> I believe the goal is to have
> <input style="-moz-appearance: none; -webkit-appearance:none; border: 4px
> solid blue;border-radius: 4px !important;width: 20px;height: 20px;color:
> green;" type="checkbox">
> render with a 4px wide blue border (and ideally a border radius of 4px,
> although the patch doesn't seem to do that).  Currently it renders with a
> grey 3d border.

I presume the reason the border-radius doesn't work is the !important in forms.css just above the modifications made by the patch:
https://hg.mozilla.org/mozilla-central/file/40fb8eae281b/layout/style/res/forms.css#l534
(see lines 541 and 551).

I guess (given the screenshots) that you're saying the patch does fix the gray 3d border, which makes sense.

(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #35)
> So I think what's hoped for here is WebKit / Blink parity on styling these
> elements. See the attachments I just tossed up. The patch in this bug gets
> us quite a bit closer, but we're not exactly there yet (looks like at least
> padding isn't working correctly).

I presume that would be the "padding: 0 ! important" on line 565 at the above link, also not modified by the patch.

Removing this *might* require some adjustments to nsFormControlFrame::Reflow.  Or it might not...  (Presumably the box-sizing: border-box is still correct?)

> Do you know what would be required to get us at parity here? Would it be a
> simple-ish modification to the wesj's patch?

So a few other things that are strange about checkbox and radio that are not modified by the patch:

nsFormControlFrame.h has an override of nsIFrame::BuildDisplayList that means that we *only* paint border, background, and outline for these controls, and nothing else.  That's probably still the right thing to do, but it's possible it might not be.  (We don't, for example, want to paint children of these elements!)

Much of the code in nsFormControlFrame.cpp is also specific to checkbox and radio.  Probably GetIntrinsicISize, GetIntrinsicBSize, and ComputeAutoSize are fine, but Reflow might need some adjustments if you change the way padding works (as I mentioned above).

It's also worth looking, with this patch, to see if there's any remaining justification for nsGfxCheckboxControlFrame and nsGfxRadioControlFrame to be separate frame classes (distinct from nsFormControlFrame, their common base class -- which also has a few static methods that should really live on a separate namespace class since they're used by other controls -- and which should really have a better name since it's a base class for just those 2 and not all form controls).
Flags: needinfo?(dbaron)
> We don't, for example, want to paint children of these elements!

Actually, for the "appearance: none" case we may want to paint their ::before/::after, but not normal kids...  In any case, this is handled in frame construction: we don't construct frames for kids of checkbox/radio.
I have a new try build for folks to try:

https://archive.mozilla.org/pub/firefox/try-builds/mconley@mozilla.com-0cc9ecf638b89d5aee22dc037b25940efcde7d61/try-macosx64/firefox-52.0a1.en-US.mac.dmg

This has a slight modification to our forms.css rules which allows for web content to override some things, like padding and border-radius.

Hey Blake - would you mind testing out the build, and letting me know if it satisfies your needs?
Flags: needinfo?(bwinton)
Seems good to me.  I'ld like to discuss it more with shorlander, in relationship to what we're doing with the style guide and browserStyle WebExtensions, but from what I can tell, it should be fine.
Flags: needinfo?(bwinton)
Flags: needinfo?(mconley)
Did some more testing, and it looks great to me!  (Based on https://www.youtube.com/watch?v=Lu5VYqEFu7s I'm excited to see this land, and let us switch our about:addons page to HTML, and let WebExtensions use browser-style to get much more harmonious settings pages!  :)
Attachment #8806145 - Flags: review?(dao+bmo)
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

https://reviewboard.mozilla.org/r/89638/#review89092
Attachment #8806145 - Flags: review?(tnikkel) → review+
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

checkmark.svg and indeterminate-checkmark.svg have CSS referring to "circle" but no such element. I also suspect black is not the right color here. I think we use -moz-Field in the background, so you should use -moz-FieldText here.

nit: remove stray space in "background-color: ThreeDFace ;"
Attachment #8806145 - Flags: review?(dao+bmo) → review-
I've discussed it more with shorlander, and we're both happy with the functionality.  :)
Attachment #8778718 - Attachment is obsolete: true
Attachment #8764974 - Attachment is obsolete: true
Comment on attachment 8806146 [details]
Bug 418833 - Remove !important padding and border-radius rules for checkbox and radio form controls.

In comment 38 dbaron said:

> I presume that would be the "padding: 0 ! important" on line 565 at the
> above link, also not modified by the patch.
> 
> Removing this *might* require some adjustments to
> nsFormControlFrame::Reflow.  Or it might not...  (Presumably the box-sizing:
> border-box is still correct?)

I don't know the historical reasons for why !important was used in the first place and I haven't tried digging into nsFormControlFrame::Reflow. A layout peer should review this.
Attachment #8806146 - Flags: review?(dao+bmo)
Attached file Test for bug 101320
Hey tn, for this second patch I've requested review on - I traced the !important's I'm removing back to bug 101320. It seems to involve the calculations on how <input>'s calculate their sizes with padding when -moz-appearance is none... but I really can't say with a high degree of certainty whether or not we care at this point. With -moz-appearance set to the default, the sizing doesn't appear to be distorted.

Are we okay to remove these !important's? This will let our folks style checkboxes properly.
Flags: needinfo?(mconley)
Comment on attachment 8806146 [details]
Bug 418833 - Remove !important padding and border-radius rules for checkbox and radio form controls.

https://reviewboard.mozilla.org/r/89640/#review89544

Bug 101320 doesn't go into any detail as to what exactly the problem was, and your patch doesn't regress the original testcase. So it seems likely that the changes we've made to how these are drawn now mean that it is no longer required.
Attachment #8806146 - Flags: review?(tnikkel) → review+
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

https://reviewboard.mozilla.org/r/89638/#review89554

::: layout/style/res/forms.css
(Diff revision 2)
>    padding: 0 !important;
>    -moz-binding: none;
> -  /* same colors as |input| rule, but |!important| this time. */
> -  background-color: -moz-Field ! important;
> +  background-repeat: no-repeat;
> +  background-position: center;
> -  color: -moz-FieldText ! important;
> -  border: 2px inset ThreeDLightShadow ! important;

So you're not actually removing the 2px border because the same declaration is on all inputs at the top of this file. But by removing it, it makes things harder to understand, since the (border box) size minus the border is the value we need to keep in sync with nsFormControlFrame::GetIntrinsic(I/B)Size. I think it's better to leave the border declaration here. But if you'd prefer you could make a comment in the border declaration for all inputs, and also mention that location here, and in nsFormControlFrame::GetIntrinsic(I/B)Size,
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

https://reviewboard.mozilla.org/r/89638/#review89554

> So you're not actually removing the 2px border because the same declaration is on all inputs at the top of this file. But by removing it, it makes things harder to understand, since the (border box) size minus the border is the value we need to keep in sync with nsFormControlFrame::GetIntrinsic(I/B)Size. I think it's better to leave the border declaration here. But if you'd prefer you could make a comment in the border declaration for all inputs, and also mention that location here, and in nsFormControlFrame::GetIntrinsic(I/B)Size,

Ah - I hadn't realized all inputs had that border. How about I put it back here, but just remove the !important to allow the overrides?
Flags: needinfo?(tnikkel)
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #56)
> Comment on attachment 8806145 [details]
> Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding
> checkbox/radio styling.
> 
> https://reviewboard.mozilla.org/r/89638/#review89554
> 
> > So you're not actually removing the 2px border because the same declaration is on all inputs at the top of this file. But by removing it, it makes things harder to understand, since the (border box) size minus the border is the value we need to keep in sync with nsFormControlFrame::GetIntrinsic(I/B)Size. I think it's better to leave the border declaration here. But if you'd prefer you could make a comment in the border declaration for all inputs, and also mention that location here, and in nsFormControlFrame::GetIntrinsic(I/B)Size,
> 
> Ah - I hadn't realized all inputs had that border. How about I put it back
> here, but just remove the !important to allow the overrides?

Sounds good to me.

nit: the indentation of the second row in layout/style/jar.mn is messed up
Attachment #8806145 - Flags: review?(dao+bmo) → review+
(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #56)
> Ah - I hadn't realized all inputs had that border. How about I put it back
> here, but just remove the !important to allow the overrides?

Yes, of course. That is what I intended to convey.
Flags: needinfo?(tnikkel)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c875e87e5301
Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/872ebe45f2f6
Remove !important padding and border-radius rules for checkbox and radio form controls. r=tnikkel
Thanks again for your work on this wesj, and sorry that it took so long to get this landed!
Flags: needinfo?(wjohnston2000)
Meh, simple little Android reftest failures aren't nearly enough fun, try this on for size: you also made devtools/client/framework/test/browser_toolbox_options.js permanently time out when a synthesizeMouseAtCenter click failed to click, https://treeherder.mozilla.org/logviewer.html#?job_id=6075455&repo=autoland#L3157
Attachment #8806145 - Flags: review?(dao+bmo) → review+
I've figured out the Android reftest failure. Looking at the devtools browser_toolbox_options.js failure now.
Flags: needinfo?(mconley)
Comment on attachment 8809205 [details]
Bug 418833 - Make browser_toolbox_options.js more resilient to things being added to the document loading queue.

https://reviewboard.mozilla.org/r/91810/#review91778

Thanks, makes sense to me!

::: devtools/client/framework/test/browser_toolbox_options.js:87
(Diff revision 1)
> +  // It's possible that the iframe for options hasn't fully loaded yet,
> +  // and might be paint-suppressed, which means that clicking things
> +  // might not work just yet. The "load" event is a good indication that
> +  // we're ready to proceed.
> +  if (tool.panelDoc.readyState != "complete") {
> +    yield BrowserTestUtils.waitForEvent(tool.panelWin, "load");

I think the `once` helper[1] is more idiomatic for this in DevTools tests:

```
yield once(tool.panelWin, "load");
```

It should already be accessible as a global in the test.

[1]: http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/devtools/client/framework/test/shared-head.js#269
Attachment #8809205 - Flags: review?(jryans) → review+
Comment on attachment 8809204 [details]
Bug 418833 - Make non-native checkbox and radio input styles look right in Fennec.

https://reviewboard.mozilla.org/r/91808/#review91798

I only familiar with the style about accessible caret, not this part. Redirect the review to snorp.
Attachment #8806145 - Flags: review?(dao+bmo) → review+
Comment on attachment 8809204 [details]
Bug 418833 - Make non-native checkbox and radio input styles look right in Fennec.

https://reviewboard.mozilla.org/r/91808/#review92338
Attachment #8809204 - Flags: review?(snorp) → review+
Comment on attachment 8809205 [details]
Bug 418833 - Make browser_toolbox_options.js more resilient to things being added to the document loading queue.

https://reviewboard.mozilla.org/r/91810/#review91778

> I think the `once` helper[1] is more idiomatic for this in DevTools tests:
> 
> ```
> yield once(tool.panelWin, "load");
> ```
> 
> It should already be accessible as a global in the test.
> 
> [1]: http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/devtools/client/framework/test/shared-head.js#269

Thanks!
Bah, still have some reftest failures on Android with my latest stuff: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c466ea551f14a2b9c207a823bd30f2a63ad97fd6

Going to need to update my patch snorp, and re-request review. Sorry. :/
Blocks: 1317795
Keywords: dev-doc-needed
Attachment #8806145 - Flags: review?(dao+bmo)
Comment on attachment 8812277 [details]
Bug 418833 - Make browser_toolbox_computed_view.js have a longer timout to avoid a permaorange on debug builds.

https://reviewboard.mozilla.org/r/94078/#review94360
Attachment #8812277 - Flags: review?(gl) → review+
Comment on attachment 8812279 [details]
Bug 418833 - Bump fuzzyness on ua-style-sheet-checkbox-radio-1 reftest for Android.

https://reviewboard.mozilla.org/r/94082/#review94364
Attachment #8812279 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8812278 [details]
Bug 418833 - Get rid of some padding rules for checkbox and radio on Fennec that were never being applied.

https://reviewboard.mozilla.org/r/94080/#review94564
Attachment #8812278 - Flags: review?(snorp) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eecb0af8a88f
Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a9da196a7884
Remove !important padding and border-radius rules for checkbox and radio form controls. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/ec772ba1b1d9
Make non-native checkbox and radio input styles look right in Fennec. r=snorp
https://hg.mozilla.org/integration/autoland/rev/9f7debc99bf8
Make browser_toolbox_options.js more resilient to things being added to the document loading queue. r=jryans
https://hg.mozilla.org/integration/autoland/rev/0691e88c6ff2
Make browser_toolbox_computed_view.js have a longer timout to avoid a permaorange on debug builds. r=gl
https://hg.mozilla.org/integration/autoland/rev/61f417a156ac
Get rid of some padding rules for checkbox and radio on Fennec that were never being applied. r=snorp
https://hg.mozilla.org/integration/autoland/rev/5df1453ded4c
Bump fuzzyness on ua-style-sheet-checkbox-radio-1 reftest for Android. r=jrmuizel
Oh hey thanks mconley. I forgot I'd turned off bugmail so I sorta missed the feedback here (and everything else). You rock.
Thanks for fixing this! Once this gets to the stable release, I'm sure there will be less div workarounds that hurt accessibility.
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1309316, in a way.

[User impact if declined]:

The label after in-content checkboxes won't wrap correctly. It'll wrap like:

[ ] This is a long string and this is not
wrapping correctly

vs what UX wants, which is:

[ ] This is a long string and this is totally
    wrapping correctly.

The hackery that the in-content checkboxes were using (a ::before pseudoelement on the <label> that mimic'd a checkbox) was causing this. By allowing us to properly style checkboxes, we don't have to use the hackery, and we get the correct wrapping.

If it turns out that this series of patches isn't acceptable, we could (in theory, and with UX's approval) change the in-content checkboxes for about:tabcrashed to be the native checkboxes, and get the correct wrapping. Or we could go for the bad wrapping. But ideally, we take this series of patches instead and everybody wins.

[Describe test coverage new/current, TreeHerder]:

Lots and lots of automated tests use checkboxes. Also, this only affects _non natively styled_ checkboxes, which isn't commonly used in the tree (yet). Fennec is the only one that uses them by default, and the tests (both mochitest and reftests) are passing.

[Risks and why]: 

I'd say this is low to medium risk. It's definitely low risk to Desktop, because native checkboxes are the default. I think it's slightly higher risk for Fennec, since non-native checkboxes are the norm there.

[String/UUID change made/needed]:

None.
Attachment #8806145 - Flags: approval-mozilla-aurora?
Comment on attachment 8806146 [details]
Bug 418833 - Remove !important padding and border-radius rules for checkbox and radio form controls.

See above.
Attachment #8806146 - Flags: approval-mozilla-aurora?
Comment on attachment 8809204 [details]
Bug 418833 - Make non-native checkbox and radio input styles look right in Fennec.

See above.
Attachment #8809204 - Flags: approval-mozilla-aurora?
Comment on attachment 8809205 [details]
Bug 418833 - Make browser_toolbox_options.js more resilient to things being added to the document loading queue.

See above.
Attachment #8809205 - Flags: approval-mozilla-aurora?
Comment on attachment 8812277 [details]
Bug 418833 - Make browser_toolbox_computed_view.js have a longer timout to avoid a permaorange on debug builds.

See above.
Attachment #8812277 - Flags: approval-mozilla-aurora?
Comment on attachment 8812278 [details]
Bug 418833 - Get rid of some padding rules for checkbox and radio on Fennec that were never being applied.

See above.
Attachment #8812278 - Flags: approval-mozilla-aurora?
Comment on attachment 8812279 [details]
Bug 418833 - Bump fuzzyness on ua-style-sheet-checkbox-radio-1 reftest for Android.

See above.
Attachment #8812279 - Flags: approval-mozilla-aurora?
Just a heads-up, this might end up backfiring to a degree for sites that are already using -moz-appearance:none to normalize/stripe down the styles on radios/checkboxes.

For instance, take this issue on webcompat.com with Facebook: https://github.com/webcompat/web-bugs/issues/3777 (note it only happens for Firefox mobile UAs).

I'm not sure what can be done about this, short of just getting sites to not use it for that purpose anymore, but I figured it was worth mentioning it here just in case.
(In reply to Thomas Wisniewski from comment #109)
> 
> I'm not sure what can be done about this, short of just getting sites to not
> use it for that purpose anymore, but I figured it was worth mentioning it
> here just in case.

Hm, that's a good call twisniewski. At the very least, it should go into the developer documentation for 53 (and 52 when it uplifts).

Web Compat-wise, is there more to be done here?
Flags: needinfo?(wisniewskit)
Other than potentially finding a work-around, we'll just have to tough it out and deal with the fallout, I suppose.

I'd imagine that the "best" solution here would be to keep the -moz prefix version as-is, and provide an unprefixed version that does what it says on the tin, but that would likely delay things while the feature was discussed on a standards track.

I can't judge whether that's a better overall solution, though, so maybe just ripping off the band-aid and dealing with the fallout is good enough?
Flags: needinfo?(wisniewskit)
(In reply to Thomas Wisniewski from comment #111)
> I can't judge whether that's a better overall solution, though, so maybe
> just ripping off the band-aid and dealing with the fallout is good enough?

I guess somebody is going to have to make a call here. Do you feel confident to make a final judgement on the best way to proceed here, or should we bubble this up the chain?
Flags: needinfo?(wisniewskit)
Given the prevalence of CSS reset techniques, I wouldn't be surprised if there's a fair bit of fallout, but I'm hardly an expert on the topic. I'll check with my team to see if they have any thoughts.

It might also be a good time to ping Blink/Edge/WebKit to see if they'd like to finally standardize this. If so, there's a chance that a standard would be easy to agree-upon, given the nature of the feature, so we might not have to delay things very long.

I'm not sure if we should delay its tide on the trains for this, so I'll try to get some insight soon.
Flags: needinfo?(wisniewskit)
I think the android default radio style should just include a dot as well, and that would fix the issue.
Actually they do have default styles, ntim. In Facebook's case this is actually a non-issue, as they intend to hide the radio button all along (they also apply border:none). However, given how long -moz-appearance:none has been around for people to mis-use or abuse, I worry this will cause actual unintended problems. Hopefully it won't (or they will be too minor to worry about), but I'd rather be proactive and at least raise the issue in advance.
I think there is too much caution here. Worried about sites breaking? Get the word out via Mozilla's publishing mediums (their blog, Twitter, etc).

One of the worst problems with Firefox and bugs is: *IF* they get fixed it takes literally years (eight since this bug was started). Do we REALLY want to bug up what should be normal behavior in the future by trying to fix broken sites today and then have another bug report sit here for nearly a decade with even more sites misusing code?

As a web developer my primary browser is Firefox ESR and I still have to deal with some sites telling me my browser is out of date. Something somewhere is going to break, better to break a small number of sites explicitly who went out of their way than to tip-toe and let the number of compatibility issues rise over time. Not that trying to make the transition smooth is a bad thing but the process of building a web browser and rendering engine isn't one of perfection so the best thing you can do is make sure it's done right when you do a large GUI review of an element and ensure that future related bugs are (as far as can be known) eliminated or greatly minimized so people who are paying attention now will see it and fix their sites.
Oh, I'm certainly not suggesting to delay this for another decade (if anything, I'm arguing to standardize this feature right now if possible). I just want to make sure we've properly considered whether there's a significant risk here, and if so, if there's a way to accomplish this without that risk, without needless delays.
I'm very much unconvinced on backporting this given we have _known_ web compat fallout.  This feels like the sort of change that we want maximal bake time for, maximal time to contact sites, etc.

Is the new -moz-appearance:none behavior basically matching what -webkit-appearance:none does?  If not, why not?

If the new behavior _does_ match -webkit-appearance:none and Facebook is still broken, have you already contacted Facebook to get this fixed on their end?  Presumably if that's the situation they have different Gecko and WebKit styles to some extent...
Flags: needinfo?(wisniewskit)
Flags: needinfo?(mconley)
As I mentioned in comment 155, this isn't an issue with the Facebook page, bz. There's no breakage there, Firefox is now just doing what Chrome does (as Facebook intended) by hiding the radio buttons. The user who reported it as an issue just didn't expect that kind of change.

As such we don't yet have any reports (to my knowledge) of any real breakage or compatibility issues.

I only raised this point to make sure we don't know of anything that's likely to break, given how web devs tend to abuse such subtle features. I was going to confer with the webcompat team about it at our team meeting tomorrow, but I figured it would also be good to check with others as well, as early as possible.
Flags: needinfo?(wisniewskit)
Flags: needinfo?(mconley)
> As I mentioned in comment 155

Ah, comment 115.  OK.

If we're basically matching how -webkit-appearance behaves, that's excellent.  There may still be compat fallout, as you not, but it makes it a lot easier to sell the new behavior to people and minimizes the chances of such fallout.

Still not entirely convinced on backporting to Aurora, but not going to object to it a priori.
(In reply to Thomas Wisniewski from comment #119)
> I only raised this point to make sure we don't know of anything that's
> likely to break, given how web devs tend to abuse such subtle features. I
> was going to confer with the webcompat team about it at our team meeting
> tomorrow, but I figured it would also be good to check with others as well,
> as early as possible.

We can't know about what might break until it does, unfortunately. Riding the trains and keeping an eye out for bustage is the best we can do. I agree with bz letting this ride all the trains is safer.
Comment on attachment 8806145 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling.

(In reply to Mike Taylor [:miketaylr] from comment #121)
> (In reply to Thomas Wisniewski from comment #119)
> > I only raised this point to make sure we don't know of anything that's
> > likely to break, given how web devs tend to abuse such subtle features. I
> > was going to confer with the webcompat team about it at our team meeting
> > tomorrow, but I figured it would also be good to check with others as well,
> > as early as possible.
> 
> We can't know about what might break until it does, unfortunately. Riding
> the trains and keeping an eye out for bustage is the best we can do. I agree
> with bz letting this ride all the trains is safer.

Okay. I will revoke the aurora approval requests and try to come up with something for 52.

One of two things will likely happen:

1) about:tabcrashed will, temporarily, use the unstyled checkbox
2) about:tabcrashed will, temporarily, use the native checkbox
3) about:tabcrashed will, temporarily, have bad wrapping for one of its checkboxes.

I'll work it out.
Attachment #8806145 - Flags: approval-mozilla-aurora?
Attachment #8806146 - Flags: approval-mozilla-aurora?
Attachment #8809204 - Flags: approval-mozilla-aurora?
Attachment #8809205 - Flags: approval-mozilla-aurora?
Attachment #8812277 - Flags: approval-mozilla-aurora?
Attachment #8812278 - Flags: approval-mozilla-aurora?
Attachment #8812279 - Flags: approval-mozilla-aurora?
(In reply to Thomas Wisniewski from comment #119)
> As I mentioned in comment 155, this isn't an issue with the Facebook page,
> bz. There's no breakage there, Firefox is now just doing what Chrome does
> (as Facebook intended) by hiding the radio buttons. The user who reported it
> as an issue just didn't expect that kind of change.
> 
> As such we don't yet have any reports (to my knowledge) of any real breakage
> or compatibility issues.

This isn't quite accurate. Per the screenshot, there's a user visible difference. 

I would guess that FB assumed the following rule served to Fennec behaved the same as Chrome, that is -moz-apperance:none more or less made the radio disappear. But this changeset makes it so we have a visible radio circle, but not inner dot on selection.

.touchableOptionArea input[type="checkbox"], .touchableOptionArea input[type="radio"] {
    -moz-appearance: none;
    background: transparent;
    border: none;
    height: 20px;
    margin: -10px 0 0;
    outline: none;
    position: absolute;
    right: 8px;
    top: 50%;
    width: 20px;
}

Chrome mobile gets the following rule (only difference is prefix):

.touchableOptionArea input[type=checkbox], .touchableOptionArea input[type=radio] {
    -webkit-appearance: none;
    background: transparent;
    border: none;
    height: 20px;
    margin: -10px 0 0;
    outline: none;
    position: absolute;
    right: 8px;
    top: 50%;
    width: 20px;
}

Mike, is there any way to make the radio circle hidden in this brave new world? If so, we can ask FB to use that and write blog posts, etc.
Flags: needinfo?(mconley)
I would think that with "-moz-appearance: none" doing "border:none" would make the circle go away....
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #124)
> I would think that with "-moz-appearance: none" doing "border:none" would
> make the circle go away....

Agreed, and on Desktop, this appears to be the case:

https://jsfiddle.net/ypbug7aL/1/

Looking at Fennec's content.css, I suspect we're hitting this:

http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/mobile/android/themes/core/content.css#188

I'm guessing that probably shouldn't be !important - but it'd be good if somebody who knows about Fennec form field styling weighs in. Vivien Nicolas added that !important in bug 516641, reviewed by mfinkle.

Hey snorp - who'd be good to weigh in on whether or not the border is supposed to be !important here? (I suspect it shouldn't be...)
Flags: needinfo?(mconley) → needinfo?(snorp)
I expect it was !important because the corresponding border style in forms.css was !important, so this was the only way to override that style, right?  The patch in this bug removed the !important in forms.css, and I expect it should have removed the one in content.css too.
Yeah, I really don't know, and all of the people who worked with that are gone now. :bz's comment above makes sense, though.
Flags: needinfo?(snorp)
Okay, thanks. Filed bug 1320732.
Depends on: 1320732
Depends on: 1320751
Depends on: 1328474
I've updated some of the text and added a footnote to the browser compat info on the appearance ref page:

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

I've also added a note to the Fx53 release notes page:

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

Does this seem OK for this bug?
Depends on: 1344257
Depends on: 1345956
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #129)
> I've updated some of the text and added a footnote to the browser compat
> info on the appearance ref page:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance
> 
> I've also added a note to the Fx53 release notes page:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/53#CSS
> 
> Does this seem OK for this bug?

I think those notes are much too broad.  What was done here was specific to radio and checkbox inputs and doesn't cover all of -moz-appearance.  I think a better description of what changed here is probably:
  radio and checkbox inputs with -moz-appearance: none now support styling with significantly more CSS properties
(More detail could certainly be added about which properties are involved, but I don't know the details right now.)

I also don't think it's worth calling out a single bugfix within a property with very broad effects in the overall support chart for -moz-appearance.  And it's also not true that -moz-appearance wasn't supported before; it did disable the native appearance for the controls; they just fell back to a different appearance that wasn't very styleable.

Since the latter change (to the -moz-appearance) looks like a decent amount of work to clean up, I'm going to make a needinfo? rather than fix it myself.

I'd also note that the change in this release should probably cite both this bug and bug 1320732, and then Firefox 54 has bug 605985 along the same lines (which I just asked about backporting to 53).
Flags: needinfo?(cmills)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #130)

> I think those notes are much too broad.  What was done here was specific to
> radio and checkbox inputs and doesn't cover all of -moz-appearance.  I think
> a better description of what changed here is probably:
>   radio and checkbox inputs with -moz-appearance: none now support styling
> with significantly more CSS properties
> (More detail could certainly be added about which properties are involved,
> but I don't know the details right now.)
> 
> I also don't think it's worth calling out a single bugfix within a property
> with very broad effects in the overall support chart for -moz-appearance. 
> And it's also not true that -moz-appearance wasn't supported before; it did
> disable the native appearance for the controls; they just fell back to a
> different appearance that wasn't very styleable.
> 
> Since the latter change (to the -moz-appearance) looks like a decent amount
> of work to clean up, I'm going to make a needinfo? rather than fix it myself.
> 
> I'd also note that the change in this release should probably cite both this
> bug and bug 1320732, and then Firefox 54 has bug 605985 along the same lines
> (which I just asked about backporting to 53).

Thanks David. I've updated the notes in light of your comments. I have not referred to the last bug you mention, but I'd watched it so I can keep an eye on it and added it to the Fx53 notes if it is backported.

In terms of completing this work, I think it is still worth calling it out in the support chart, but we need to create a reference saying what properties are now able to style radios/checkboxes. I'm not sure of the best way to find out this information other than manual testing, but I'd love to create it at some point.
Flags: needinfo?(cmills)
This was backed out from Firefox 53 over in bug 1352406.
Hey cmills,

Because of the backout, is it possible for you to move the work you did in comment 129 to the 54 release instead?
Flags: needinfo?(cmills)
(In reply to Mike Conley (:mconley) from comment #133)
> Hey cmills,
> 
> Because of the backout, is it possible for you to move the work you did in
> comment 129 to the 54 release instead?

Yup, sure thing. I've moved the release note to

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

And updated the support chart on 

https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance
Flags: needinfo?(cmills)
Awesome, thank you so much!
Run mozregress, seems https://webcompat.com/issues/5571 is affected by this issue.
Note that comment 137 is tracked in bug 1328474.
Blocks: 1371951
You need to log in before you can comment on or make changes to this bug.