Last Comment Bug 418833 - can't define the style of input when the type is set to "checkbox" or "radio" with CSS
: can't define the style of input when the type is set to "checkbox" or "radio"...
Status: RESOLVED FIXED
: css1, dev-doc-needed
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- minor with 22 votes (vote)
: mozilla53
Assigned To: Mike Conley (:mconley) - Getting through review / needinfo backlog
:
: Jet Villegas (:jet)
Mentors:
http://skrupel.freehostia.com/eigenes...
: 1311488 (view as bug list)
Depends on: 1320751 1328474 1320732
Blocks: 1275287 1317795 1320049
  Show dependency treegraph
 
Reported: 2008-02-21 07:28 PST by Skasi
Modified: 2017-01-03 20:17 PST (History)
47 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
wip patch (39 bytes, text/x-review-board-request)
2016-06-24 10:06 PDT, Wesley Johnston (:wesj)
no flags Details | Review
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. f?dbaron (58 bytes, text/x-review-board-request)
2016-08-07 17:22 PDT, Wesley Johnston (:wesj)
no flags Details | Review
Manual test case (1.24 KB, text/html)
2016-10-06 06:05 PDT, Mike Conley (:mconley) - Getting through review / needinfo backlog
no flags Details
Browser engine comparison (63.86 KB, image/png)
2016-10-06 06:09 PDT, Mike Conley (:mconley) - Getting through review / needinfo backlog
no flags Details
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. (58 bytes, text/x-review-board-request)
2016-10-31 15:00 PDT, Mike Conley (:mconley) - Getting through review / needinfo backlog
tnikkel: review+
Details | Review
Bug 418833 - Remove !important padding and border-radius rules for checkbox and radio form controls. (58 bytes, text/x-review-board-request)
2016-10-31 15:00 PDT, Mike Conley (:mconley) - Getting through review / needinfo backlog
tnikkel: review+
Details | Review
Test for bug 101320 (965 bytes, text/html)
2016-11-01 10:36 PDT, Mike Conley (:mconley) - Getting through review / needinfo backlog
no flags Details
Bug 418833 - Make non-native checkbox and radio input styles look right in Fennec. (58 bytes, text/x-review-board-request)
2016-11-09 15:14 PST, Mike Conley (:mconley) - Getting through review / needinfo backlog
snorp: review+
Details | Review
Bug 418833 - Make browser_toolbox_options.js more resilient to things being added to the document loading queue. (58 bytes, text/x-review-board-request)
2016-11-09 15:14 PST, Mike Conley (:mconley) - Getting through review / needinfo backlog
jryans: review+
Details | Review
Bug 418833 - Make browser_toolbox_computed_view.js have a longer timout to avoid a permaorange on debug builds. (58 bytes, text/x-review-board-request)
2016-11-18 10:42 PST, Mike Conley (:mconley) - Getting through review / needinfo backlog
gl: review+
Details | Review
Bug 418833 - Get rid of some padding rules for checkbox and radio on Fennec that were never being applied. (58 bytes, text/x-review-board-request)
2016-11-18 10:42 PST, Mike Conley (:mconley) - Getting through review / needinfo backlog
snorp: review+
Details | Review
Bug 418833 - Bump fuzzyness on ua-style-sheet-checkbox-radio-1 reftest for Android. (58 bytes, text/x-review-board-request)
2016-11-18 10:42 PST, Mike Conley (:mconley) - Getting through review / needinfo backlog
jmuizelaar: review+
Details | Review
Releave vs Fennec Nightly vs Chrome (Facebook form thingy) (1.27 MB, image/png)
2016-11-28 09:57 PST, Mike Taylor [:miketaylr]
no flags Details

Description User image Skasi 2008-02-21 07:28:32 PST
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.
Comment 1 User image Caleb Cushing 2008-08-03 20:57:05 PDT
same on linux.
Comment 2 User image John A. Bilicki III 2008-09-09 04:06:04 PDT
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.
Comment 3 User image reelix 2009-04-30 06:39:43 PDT
Confirmed in Firefox 3.1b4 - Windows XP SP3
Comment 4 User image Christian Sturm 2009-04-30 10:03:17 PDT
Confimed on:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090430 Minefield/3.6a1pre
Comment 5 User image reelix 2009-04-30 10:24:24 PDT
Confirmed on:

Firefox 3.0.10 - Windows 7 RC1 Build 7100
Comment 6 User image Rick 2010-08-10 02:02:05 PDT
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.
Comment 7 User image Rick 2010-08-10 02:23:18 PDT
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?
Comment 8 User image Christian Sturm 2010-09-03 06:22:36 PDT
Still present on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Comment 9 User image flying sheep 2011-04-27 09:12:36 PDT
Confirmed on 4.0 (Linux)
Comment 10 User image Zachary Yaro 2012-01-03 16:47:38 PST
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).
Comment 11 User image Ben 2012-05-22 18:45:56 PDT
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?
Comment 12 User image Alfredos-Panagiotis Damkalis [:fredy] 2013-03-29 19:26:43 PDT
I confirm this bug too at Firefox 19.0.2 with the provided url.

see also bug 605985 which seems to be relative.
Comment 13 User image Alfredos-Panagiotis Damkalis [:fredy] 2013-03-29 19:40:21 PDT
I added the [NEEDINFO] tag in order to get information about any relative css specification which describes the right behavior on this.
Comment 14 User image Justin Dolske [:Dolske] 2013-03-30 17:04:51 PDT
Presumably this is just a -moz-appearance issue? See https://developer.mozilla.org/en-US/docs/CSS/-moz-appearance
Comment 15 User image account 2015-05-12 11:46:44 PDT
This bug is still present in Firefox 37 & 38 on Linux and Windows 7 (And Mac presumably).
Comment 16 User image beckiechoi 2015-08-13 12:07:16 PDT
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/.
Comment 17 User image Michael Heyns 2015-11-17 08:48:05 PST
I can confirm this is still present in the latest nightly 45.0a1. Thanks for the jsfiddle, beckie.
Comment 18 User image Rob G 2015-12-27 13:12:56 PST
@beckiechoi don't forget to include indeterminate checkbox styling: http://jsfiddle.net/Mottie/go392m5s/12/
Comment 19 User image Blake Winton (:bwinton) (:☕️) 2016-03-16 12:36:05 PDT
This is also affecting WebExtensions panel styles, as shown in https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/RadioButtonBug.png
Comment 20 User image Tim Guan-tin Chien [:timdream] (please needinfo) 2016-03-23 21:52:29 PDT
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!
Comment 21 User image Neil Deakin 2016-03-24 05:47:58 PDT
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.
Comment 22 User image Blake Winton (:bwinton) (:☕️) 2016-03-24 06:09:23 PDT
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?
Comment 23 User image Stephen Horlander [:shorlander] 2016-03-24 06:13:25 PDT
(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.
Comment 24 User image Neil Deakin 2016-03-24 06:27:15 PDT
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.
Comment 25 User image Markus Stange [:mstange] 2016-03-24 12:00:44 PDT
(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?
Comment 26 User image Stephen Horlander [:shorlander] 2016-03-24 13:02:26 PDT
(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.
Comment 27 User image Wesley Johnston (:wesj) 2016-06-24 10:06:31 PDT
Created attachment 8764974 [details]
wip patch

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?
Comment 28 User image Kris Maglione [:kmag] 2016-07-21 09:28:04 PDT
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?
Comment 29 User image Wesley Johnston (:wesj) 2016-08-07 17:22:41 PDT
Created attachment 8778718 [details]
Bug 418833 - Move default checkbox/radio drawing to images. Allow overriding checkbox/radio styling. f?dbaron

Review commit: https://reviewboard.mozilla.org/r/60536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60536/
Comment 30 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-04 12:28:36 PDT
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.
Comment 31 User image David Baron :dbaron: ⌚️UTC-8 2016-10-04 13:03:45 PDT
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?
Comment 32 User image Blake Winton (:bwinton) (:☕️) 2016-10-04 13:49:24 PDT
(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.
Comment 33 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-06 06:05:26 PDT
Created attachment 8798409 [details]
Manual test case
Comment 34 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-06 06:09:07 PDT
Created attachment 8798410 [details]
Browser engine comparison

Left is Blink / WebKit, middle is Gecko with attachment 8778718 [details] applied, right is Gecko as it stands today.
Comment 35 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-06 06:15:26 PDT
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?
Comment 36 User image John A. Bilicki III 2016-10-06 06:19:46 PDT Comment hidden (off-topic)
Comment 37 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-12 09:01:33 PDT
Gentle poke.
Comment 38 User image David Baron :dbaron: ⌚️UTC-8 2016-10-19 12:12:34 PDT
(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).
Comment 39 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-19 12:24:08 PDT
> 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.
Comment 40 User image Kris Maglione [:kmag] 2016-10-19 17:32:44 PDT
*** Bug 1311488 has been marked as a duplicate of this bug. ***
Comment 41 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-21 22:34:24 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0995e07843
Comment 42 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-28 14:10:54 PDT
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?
Comment 43 User image Blake Winton (:bwinton) (:☕️) 2016-10-29 13:00:53 PDT
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.
Comment 44 User image Blake Winton (:bwinton) (:☕️) 2016-10-31 08:11:09 PDT
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!  :)
Comment 45 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-31 15:00:06 PDT Comment hidden (mozreview-request)
Comment 46 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-10-31 15:00:06 PDT Comment hidden (mozreview-request)
Comment 47 User image Timothy Nikkel (:tnikkel) 2016-10-31 19:33:49 PDT
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
Comment 48 User image Dão Gottwald [:dao] 2016-11-01 04:38:08 PDT
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 ;"
Comment 49 User image Blake Winton (:bwinton) (:☕️) 2016-11-01 06:55:35 PDT
I've discussed it more with shorlander, and we're both happy with the functionality.  :)
Comment 50 User image Dão Gottwald [:dao] 2016-11-01 07:50:16 PDT
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.
Comment 51 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-01 10:20:57 PDT Comment hidden (mozreview-request)
Comment 52 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-01 10:20:57 PDT Comment hidden (mozreview-request)
Comment 53 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-01 10:36:44 PDT
Created attachment 8806431 [details]
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.
Comment 54 User image Timothy Nikkel (:tnikkel) 2016-11-02 00:49:07 PDT
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.
Comment 55 User image Timothy Nikkel (:tnikkel) 2016-11-02 01:07:46 PDT
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 56 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-02 08:51:27 PDT
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?
Comment 57 User image Dão Gottwald [:dao] 2016-11-02 08:59:24 PDT
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
Comment 58 User image Timothy Nikkel (:tnikkel) 2016-11-02 13:44:28 PDT
(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.
Comment 59 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-02 14:10:33 PDT Comment hidden (mozreview-request)
Comment 60 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-02 14:10:33 PDT Comment hidden (mozreview-request)
Comment 61 User image Pulsebot 2016-11-02 14:24:10 PDT
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
Comment 62 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-02 14:26:25 PDT
Thanks again for your work on this wesj, and sorry that it took so long to get this landed!
Comment 63 User image Wes Kocher (:KWierso) 2016-11-02 17:14:15 PDT
I had to back these out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6068750&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7a2381a43ae5
Comment 64 User image Phil Ringnalda (:philor) 2016-11-02 20:00:56 PDT
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
Comment 65 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-04 12:49:32 PDT
I've figured out the Android reftest failure. Looking at the devtools browser_toolbox_options.js failure now.
Comment 66 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-09 15:14:13 PST Comment hidden (mozreview-request)
Comment 67 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-09 15:14:13 PST Comment hidden (mozreview-request)
Comment 68 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-09 15:14:13 PST Comment hidden (mozreview-request)
Comment 69 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-09 15:14:13 PST Comment hidden (mozreview-request)
Comment 70 User image J. Ryan Stinnett [:jryans] (use ni?) 2016-11-09 15:23:21 PST
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
Comment 71 User image Morris Tseng [:mtseng] [:Morris] 2016-11-09 17:59:05 PST
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.
Comment 72 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-10 22:06:48 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a88d5851aa
Comment 73 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2016-11-11 06:38:11 PST
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
Comment 74 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-11 21:40:02 PST
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!
Comment 75 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-11 21:40:43 PST
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. :/
Comment 76 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-16 11:28:20 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7d12cede02
Comment 77 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-16 12:25:13 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87309ad1d7b
Comment 78 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-16 12:55:01 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf97d52af9f
Comment 79 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-17 07:57:53 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cad9462eb522
Comment 80 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-17 15:15:22 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e06f2d584f0
Comment 81 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 09:00:00 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9869d8dcd30c
Comment 82 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:39:59 PST
Hallelujah - I have a green reftest run on Android. https://treeherder.mozilla.org/#/jobs?repo=try&revision=923b44227844cdb7c83f0e1feca24dec3292f715
Comment 83 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 84 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 85 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 86 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 87 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 88 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 89 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 10:42:42 PST Comment hidden (mozreview-request)
Comment 90 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 11:19:35 PST Comment hidden (mozreview-request)
Comment 91 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 11:19:35 PST Comment hidden (mozreview-request)
Comment 92 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 11:19:35 PST Comment hidden (mozreview-request)
Comment 93 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 11:19:35 PST Comment hidden (mozreview-request)
Comment 94 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-18 11:19:35 PST Comment hidden (mozreview-request)
Comment 95 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-11-18 19:43:28 PST
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
Comment 96 User image Jeff Muizelaar [:jrmuizel] 2016-11-18 20:25:14 PST
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
Comment 97 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2016-11-21 07:02:31 PST
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
Comment 98 User image Pulsebot 2016-11-21 08:23:12 PST
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
Comment 100 User image Wesley Johnston (:wesj) 2016-11-23 10:32:06 PST
Oh hey thanks mconley. I forgot I'd turned off bugmail so I sorta missed the feedback here (and everything else). You rock.
Comment 101 User image beckiechoi 2016-11-23 16:28:09 PST
Thanks for fixing this! Once this gets to the stable release, I'm sure there will be less div workarounds that hurt accessibility.
Comment 102 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:38:22 PST
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.
Comment 103 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:38:46 PST
Comment on attachment 8806146 [details]
Bug 418833 - Remove !important padding and border-radius rules for checkbox and radio form controls.

See above.
Comment 104 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:38:51 PST
Comment on attachment 8809204 [details]
Bug 418833 - Make non-native checkbox and radio input styles look right in Fennec.

See above.
Comment 105 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:39:00 PST
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.
Comment 106 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:39:08 PST
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.
Comment 107 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:39:13 PST
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.
Comment 108 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-24 07:39:19 PST
Comment on attachment 8812279 [details]
Bug 418833 - Bump fuzzyness on ua-style-sheet-checkbox-radio-1 reftest for Android.

See above.
Comment 109 User image Thomas Wisniewski 2016-11-24 13:19:49 PST
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.
Comment 110 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-25 06:56:35 PST
(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?
Comment 111 User image Thomas Wisniewski 2016-11-25 07:00:37 PST
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?
Comment 112 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-25 07:10:48 PST
(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?
Comment 113 User image Thomas Wisniewski 2016-11-25 07:20:13 PST
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.
Comment 114 User image Tim Nguyen :ntim 2016-11-26 03:18:32 PST
I think the android default radio style should just include a dot as well, and that would fix the issue.
Comment 115 User image Thomas Wisniewski 2016-11-26 08:20:19 PST
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.
Comment 116 User image John A. Bilicki III 2016-11-26 09:41:34 PST
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.
Comment 117 User image Thomas Wisniewski 2016-11-26 09:49:42 PST
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.
Comment 118 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-28 07:11:44 PST
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...
Comment 119 User image Thomas Wisniewski 2016-11-28 07:22:46 PST
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.
Comment 120 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-28 07:28:52 PST
> 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.
Comment 121 User image Mike Taylor [:miketaylr] 2016-11-28 09:13:52 PST
(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 122 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-28 09:32:45 PST
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.
Comment 123 User image Mike Taylor [:miketaylr] 2016-11-28 09:57:21 PST
Created attachment 8814979 [details]
Releave vs Fennec Nightly vs Chrome (Facebook form thingy)

(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.
Comment 124 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-28 10:01:33 PST
I would think that with "-moz-appearance: none" doing "border:none" would make the circle go away....
Comment 125 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-28 10:09:11 PST
(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...)
Comment 126 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-28 10:13:09 PST
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.
Comment 127 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2016-11-28 10:14:51 PST
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.
Comment 128 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2016-11-28 10:19:47 PST
Okay, thanks. Filed bug 1320732.

Note You need to log in before you can comment on or make changes to this bug.