Closed
Bug 1430391
Opened 7 years ago
Closed 7 years ago
Shield studies checkbox is checked even if checked attribute is "false"
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 59
People
(Reporter: magicp.jp, Assigned: myk)
References
Details
Attachments
(3 files)
Steps to reproduce:
1. Launch Nightly
2. Go to about:preferences#privacy > Nightly Data Collection and Use
3. Check off "Allow Firefox to install and run studies"
4. Restart Nightly
5. Confirm "Allow Firefox to install and run studies"
Actual results:
Shield studies checkbox is checked even if checked attribute is "false" (app.shield.optoutstudies.enabled = false).
Expected results:
Display checkbox correctly.
Assignee | ||
Comment 2•7 years ago
|
||
I see this on Windows and Linux but not Mac. Investigating…
Assignee: nobody → myk
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to magicp from comment #0)
> 3. Check off "Allow Firefox to install and run studies"
> 4. Restart Nightly
> 5. Confirm "Allow Firefox to install and run studies"
On Linux, anyway, I don't even need to restart Nightly. I just need to switch to the General tab, reload the preferences page, and switch back to the Privacy & Security tab.
I think I know what's going on here. My next step is to confirm a fix and write an automated test for it.
Assignee | ||
Comment 5•7 years ago
|
||
The setValue function, which I copied without changes from preferences.xml to preferencesBindings.js, sets either an element's property or the equivalent attribute, depending on whether or not the element has a property with the given name.
The function's comment explains the reason for this:
>Initialize a UI element property with a value. Handles the case
>where an element has not yet had a XBL binding attached for it and
>the property setter does not yet exist by setting the same attribute
>on the XUL element using DOM apis and assuming the element's
>constructor or property getters appropriately handle this state.
In other words, setValue can race XBL binding attachment, so it accounts for that possibility by setting the attribute rather than the property if the property isn't yet defined.
In the case of the checkbox and listitem elements, however, when the element doesn't yet have a XBL binding attached, then setValue sets the "checked" attribute incorrectly. Those elements expect the attribute to be present (with any value) if the element is checked and absent if the element is unchecked. But setValue sets the attribute to "true" if the element is checked and "false" if the element is unchecked.
In other words, setValue sets checked="false" when the element shouldn't be checked, which gets interpreted the same as checked="true": the element appears checked.
I'm unsure why this bug didn't manifest previously, but I suspect it's because setValue used to be called later in the page load process, since it used to be triggered by <preference> binding constructors, and now it's triggered by DOMContentLoaded. Thus it presumably won the race less frequently, making this bug less obvious.
In any case, the fix is obvious: make setValue set/remove the "checked" attribute appropriately. Here's the fix.
Unfortunately, I'm unsure how to write an automated test for this, since by definition the behavior is indeterminate. The "checked" attribute won't always be set, and asserting that a checked element's internal state is consistent doesn't prove that it's because the "checked" attribute is now being correctly set/removed.
Once we replace the checkbox and menuitem XBL implementations with a non-XBL version, we should be able to make the behavior here determinate: either setValue always runs before the checkbox/menuitem implementations are instantiated, or it always runs afterward. After which we will be able to remove one of the two paths here and test that the correct thing always happens.
Flags: needinfo?(jaws)
Attachment #8943406 -
Flags: review?(jaws)
Comment 6•7 years ago
|
||
Comment on attachment 8943406 [details] [diff] [review]
make setValue set/remove "checked" attribute appropriately
Review of attachment 8943406 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the explanation. Yes, in XUL we treat presence of an attribute as true and absence as false.
Attachment #8943406 -
Flags: review?(jaws) → review+
Updated•7 years ago
|
Flags: needinfo?(jaws)
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac1424f154c
make setValue set/remove 'checked' attribute appropriately; r=jaws
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment 9•7 years ago
|
||
I have reproduced this bug with Nightly 59.0a1 (2018-01-13)on Windows 10 , 64 Bit !
This bug's fix is Verified with latest Beta !
Build ID 20180215111455
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
QA Whiteboard: [testday-20180216]
Comment 10•7 years ago
|
||
I verified the fix on macOS 10.13 and Ubuntu 16.04 using latest Nightly 60.0a1 and beta 59.0b10, too. The bug is not reproducing anymore.
You need to log in
before you can comment on or make changes to this bug.
Description
•