1354336 - Addon options checkboxes inactive

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Wesley Branton]

Attachment: Test Addon

HTML input elements that are checkbox or radio types within the addon options are not clickable in in the 55.0 Nightly build of Firefox. The input element displays fine, but you cannot click it.

This issue is not present in the release or beta builds.

You can use the sample ZIP addon attached to this report to reproduce the issue. You can also change the input type to radio in the options.html file of this ZIP folder to reproduce the radio button issue.

All other input types appear to function properly.

Comment 1

[Bleddle]

I'd like to add that I can't even access the options/preferences page for certain addons, which completely breaks them e.g AdNauseum, Imagus, TrackMeNot and many more.

On Firefox 55.0a1 20170407100252

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not sure how this can be a duplicate of that old bug, given that it's a regression.  Was this _caused_ by the fix for that bug, perhaps?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or did you mean to mark as dup of bug 1352238, which will be backing out bug 418833?

Comment 5

[Kris Maglione [:kmag]]

Bug 1275287 turned on support for optional useragent stylesheets, and those stylesheets cause issues for some pages that use radio and checkbox elements thanks to bug 418833.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That's a bit of a problem, because we're backing out bug 418833 from everywhere and wontfixing it, no?  Mats, are you aware of this interaction?  Do the patches in bug 1352238 address this issue?

Comment 7

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]: severe regression

I expect that the backout part of bug 1352238 will fix this, but let's
reopen and track it for now.

Comment 8

[Marcia Knous [:marcia]]

Tracking 55+ based on Comment 7.

Comment 9

[:shell escalante]

kris looking at how this relates to patch being backing out... thinks mconley is engaged.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

The backout of bug 418833 (currently occurring in bug 1357169) does not seem to fix the issue.

I'll note, however, that the backouts of bug 418833 actually (as far as I understand), do not actually impact the ability of our front-end to style the checkbox on desktop. For 54+, I believe it should be possible to not use the checkbox ::before hack we're using here, and just style the checkbox directly after using appearance: none on it.

Comment 12

[Mats Palmgren (inactive)]

The problem seems to be this rule:
http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/browser/components/extensions/extension.css#217-219
It overrides 'display: inline-block' from forms.css here:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/layout/style/res/forms.css#575

In a beta tree (v54) these two rules are the same; the difference seems to be that in v55
we apply extension.css to the content at hand, whereas in v54 it appears that we don't
(I don't see any rules from that file for this content at all in v54).

So, it appears to be a regression from this chunk from bug 1275287:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/toolkit/mozapps/extensions/content/extensions.js#103-109

Comment 15

[ExE Boss]

Attachment: Checkbox test v1.1

For some odd reason, it works if one adds an `id` attribute and a `for` attribute with the same value to the `<input>` and `<label>` elements respectively.

Comment 16

[ExE Boss]

(Comment 15 continued)

I discovered this while writing the test extension for Bug 1371951.

Comment 17

[Geoffrey De Belie (Smile4ever)]

(In reply to ExE Boss from comment #15)
> Created attachment 8876421 [details]
> Checkbox test v1.1
> 
> For some odd reason, it works if one adds an `id` attribute and a `for`
> attribute with the same value to the `<input>` and `<label>` elements
> respectively.

The commit that implements this workaround for the addon Translate Now can be found here:
https://github.com/Smile4ever/firefoxaddons/commit/2d44155d115eaaf31dec4b9800046f53c3587ee2

Adding a label before the element with "for" didn't help, the label really needs to be after the input element.

Comment 19

[Liz Henry (:lizzard)]

Matthew, any luck here? This sounds bad enough that I am marking it as a blocker for the 55 release. I don't want to break all option setting for add-ons, and this sounds very noticeable in nightly. 
Once we have a patch in nightly it would be good to verify the fix and please request uplift to beta. Thanks!

Comment 20

[Matthew Wein [:mattw]]

Attachment: Bug 1354336 - Require all browser_style elements to have the browser-style class

This change is meant to prevent existing options from breaking. The following elements
are now required to have the following classes before any styling will be applied:

checkbox: "moz-checkbox"
radio: "moz-radio"
button: "moz-button"
select: "moz-select"
option: "moz-option"
label: "moz-label"

Review commit: https://reviewboard.mozilla.org/r/151408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151408/

Comment 21

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151408/diff/1-2/

Comment 22

[Matthew Wein [:mattw]]

The patch I've uploaded sets "options_ui.browser_style" to false by default, which is consistent with https://developer.chrome.com/extensions/optionsV2#chrome_style.

I've also made "browser_style" more strict by requiring elements to have the following special class names before styling will be applied:

checkbox: "browser-style-checkbox"
radio: "browser-style-radio"
button: "browser-style-button"
select: "browser-style-select"
option: "browser-style-option"
label: "browser-style-label"

Below are a few examples of the format required for an element to be styled with "browser_style":

Checkboxes:
<div class="browser-style-checkbox">
  <input id="checkbox-1" type="checkbox"><label for="checkbox-1">Checkbox 1</label>
</div>
<div class="browser-style-checkbox">
  <input id="checkbox-2" type="checkbox"><label for="checkbox-2">Checkbox 2</label>
</div>

Radio Group:
<div class="browser-style-radio">
  <input name="radio-group" id="radio-1" type="radio"><label for="radio-1">Radio</label>
</div>
<div class="browser-style-radio">
  <input name="radio-group" id="radio-2" type="radio"><label for="radio-2">Radio</label>
</div>

Buttons:
<button class="browser-style-button default">Default</button>
<button class="browser-style-button default focused">Focused</button>

Comment 23

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151408/diff/2-3/

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

https://reviewboard.mozilla.org/r/151408/#review157126

I think you'll want to introduce one class-name that dictates the use of browser styling; '.browser-style', without the specific control type as suffix.
Then you can do `input[type="radio"].browser-style:hover + label::before`, etc.

You'll need a extensions peer to review the XPIInstall.jsm change.

::: browser/components/extensions/extension.css
(Diff revision 3)
>  /* stylelint-disable property-no-vendor-prefix */
>  /* Base */
> -button,
> -select,
> -option,
> +button.browser-style-button,
> +select.browser-style-select,
> +option.browser-style-option {
> -input {

Why remove `input`?

Comment 25

[Matthew Wein [:mattw]]

> I think you'll want to introduce one class-name that dictates the use of
> browser styling; '.browser-style', without the specific control type as
> suffix.
> Then you can do `input[type="radio"].browser-style:hover + label::before`,
> etc.

Sounds good to me!

> 
> You'll need a extensions peer to review the XPIInstall.jsm change.

I made this change before making any modifications to extension.css, and now that the style is scoped to a specific class-name I'm thinking we should continue to have browser_style enabled by default.

Comment 26

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

https://reviewboard.mozilla.org/r/151408/#review157126

> Why remove `input`?

Sorry, I forgot to finish this part. I moved -moz-appearance:none to the global '.browser-style' class because I think it makes sense to have this scoped only to those elements. I also added -webkit-appearance:none for Chrome compatibility. What do you think?

Comment 27

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151408/diff/3-4/

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

https://reviewboard.mozilla.org/r/151408/#review158004

Alright, LGTM! It does seem like you need to document this quite well for developers to get our styling? Please make sure to do that ;-) You can use the keyword 'dev-doc-needed' to loop in MDN folks eventually.

Comment 29

[Matthew Wein [:mattw]]

Yeah, I agree! Adding dev-doc-needed.

Comment 30

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151408/diff/4-5/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5fa98ffa258
Require all browser_style elements to have the browser-style class r=mikedeboer

Comment 32

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151408/diff/5-6/

Comment 33

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53d36fd7c194
Backed out changeset d5fa98ffa258 as requested by mattw. r=backout

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23c4b7136fd6
Require all browser_style elements to have the browser-style class r=mikedeboer

Comment 35

[Matthew Wein [:mattw]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1275287
[User impact if declined]: Non-browser-style checkboxes and radio groups in extension options won't work.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is relatively small, and is contained to extensions which have options pages that have browser_style enabled.
[String changes made/needed]: None

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23c4b7136fd6

Comment 37

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

https://reviewboard.mozilla.org/r/151408/#review159104

r=me for removing the webkit prefix thing. I could've rs'd this if you showed me the interdiff last week!

Comment 38

[CosminB]

Attachment: CheckboxRadioButtons.gif

I can reproduce this issue on Firefox 55.0b6 (20170629005143) and Firefox 56.0a1 (2017-07-03) under Wind 7 64-bit and Ubuntu 12.04 64-bit. 

This issue is verified as fixed on Firefox 56.0a1 (2017-07-03) under Wind 7 64-bit and Ubuntu 12.04 64-bit.
The elements that are checkbox or radio types within the addon options are clickable.

Please see the attached video.

Comment 39

[CosminB]

> I can reproduce this issue on Firefox 55.0b6 (20170629005143) and Firefox
> 56.0a1 (2017-07-03) under Wind 7 64-bit and Ubuntu 12.04 64-bit. 

I meant to say Firefox 56.0a1 (2017-06-28) not Firefox 56.0a1 (2017-07-03)

Comment 40

[Julien Cristau [:jcristau]]

Comment on attachment 8880065 [details]
Bug 1354336 - Require all browser_style elements to have the browser-style class

fix a regression for add-on options in beta55, verified in nightly

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d666bbb2097a

Comment 42

[CosminB]

Attachment: BetaFix.gif

This issue is verified as fixed on Firefox 55.0b7 (20170704233521) under Wind 7 64-bit and Ubuntu 12.04 64-bit. 
The elements that are checkbox or radio types within the addon options are clickable.
 
Please see the attached video.

Comment 43

[ExE Boss]

There are at least still two more bugs in the file `extension.css` at lines `559 => 548` and `564 => 553`.

The class should be `.panel-section-tabs-button.selected.browser-style` and not `.panel-section-tabs-button.select.browser-styleed`.

Also, do we really want `.panel-section-tabs-button.selected` to also require `.browser-style`?

Comment 44

[ExE Boss]

I can’t use the reviewboard for some reason, so I’m posting this here instead.

Comment 45

[Will Bamberg [:wbamberg]]

I've written a little page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/user_interface/Browser_styles

Let me know if it makes sense and I'll update other places to link to it.

Comment 46

[ExE Boss]

I think that you should add previews and the components from: http://design.firefox.com/StyleGuide/#/navigation (which exist in `chrome://browser/content/extension.css`)

I tried to add the following after the table at the end of the document:

> <p>Additionally, there are certain pre-made components that are made to look like <a href="http://design.firefox.com/StyleGuide/#/navigation">native Firefox panel popups</a>.</p>

But I can’t contribute it to MDN for some reason that isn’t explained anywhere (I can’t even preview my changes).

Comment 47

[Will Bamberg [:wbamberg]]

(In reply to ExE Boss from comment #46)
> I think that you should add previews and the components from:
> http://design.firefox.com/StyleGuide/#/navigation (which exist in
> `chrome://browser/content/extension.css`)

I don't think it's appropriate to include this. These docs are supposed to document a more or less standardised technology for developing cross-browser extensions. I've discussed this with the Firefox add-ons team and my understanding was that they don't want to make extension.css and the classes it contains part of the "Browser Extensions" standard, and it's not obvious they want to commit to supporting it as a public API for Firefox extensions - you'll see that the link you posted already claims to be out of date.

So I think the most helpful thing we can say is: include browser_style, and the "browser-style" class on certain elements, and you will get something that looks more like the browser's native styling, whatever that is in any particular version of a particular browser.

Comment 48

[Stoyan Dimitrov [:stoyan]]

There is repeated .select.browser-style declaration