Addon options checkboxes inactive

VERIFIED FIXED in Firefox 55

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: mozilla, Assigned: mattw, NeedInfo)

Tracking

(Blocks 1 bug, {dev-doc-complete, regression, testcase})

55 Branch
mozilla56
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55blocking verified, firefox56+ verified)

Details

(Whiteboard: triaged)

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
Posted file 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.

Updated

2 years ago
Blocks: 1275287
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Extension Compatibility → WebExtensions: Frontend
Ever confirmed: true
Keywords: testcase
Product: Firefox → Toolkit

Comment 1

2 years ago
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 418833
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?
Flags: needinfo?(kmaglione+bmo)
Or did you mean to mark as dup of bug 1352238, which will be backing out bug 418833?
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.
Flags: needinfo?(kmaglione+bmo)
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?
Flags: needinfo?(mats)
[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.
Depends on: 1352238
Flags: needinfo?(mats)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Comment 9

2 years ago
kris looking at how this relates to patch being backing out... thinks mconley is engaged.
Flags: needinfo?(mconley)
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P1
Whiteboard: triaged
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.
Flags: needinfo?(mconley)

Updated

2 years ago
Duplicate of this bug: 1361084
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
Flags: needinfo?(mwein)
OS: Unspecified → All
Hardware: Unspecified → All
Duplicate of this bug: 1365112
Duplicate of this bug: 1355077
(Assignee)

Updated

2 years ago
Assignee: nobody → mwein
Flags: needinfo?(mwein)

Comment 15

2 years ago
Posted file 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

2 years ago
(Comment 15 continued)

I discovered this while writing the test extension for Bug 1371951.
(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.

Updated

2 years ago
Duplicate of this bug: 1374193
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!
Flags: qe-verify+
Flags: needinfo?(mwein)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(mwein)
Attachment #8880065 - Flags: review?(mdeboer)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
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 hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
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`?
Attachment #8880065 - Flags: review?(mdeboer)
(Assignee)

Comment 25

2 years ago
> 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.
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 28

2 years ago
mozreview-review
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.
Attachment #8880065 - Flags: review?(mdeboer) → review+
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 29

2 years ago
Yeah, I agree! Adding dev-doc-needed.
Keywords: dev-doc-needed
Comment hidden (mozreview-request)

Comment 31

2 years ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5fa98ffa258
Require all browser_style elements to have the browser-style class r=mikedeboer
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8880065 - Flags: review+ → review?(mdeboer)

Comment 33

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/53d36fd7c194
Backed out changeset d5fa98ffa258 as requested by mattw. r=backout

Comment 34

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23c4b7136fd6
Require all browser_style elements to have the browser-style class r=mikedeboer
(Assignee)

Comment 35

2 years ago
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
Attachment #8880065 - Flags: approval-mozilla-beta?

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23c4b7136fd6
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 37

2 years ago
mozreview-review
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!
Attachment #8880065 - Flags: review?(mdeboer) → review+

Comment 38

2 years ago
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

2 years ago
> 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 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
Attachment #8880065 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 42

2 years ago
Posted image 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.

Updated

2 years ago
Status: RESOLVED → VERIFIED

Comment 43

2 years ago
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

2 years ago
I can’t use the reviewboard for some reason, so I’m posting this here instead.
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.
Flags: needinfo?(matthewjwein)

Comment 46

2 years ago
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).
(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.
There is repeated .select.browser-style declaration

Updated

2 years ago
Depends on: 1391464

Updated

11 months ago
Blocks: 1465256

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.