Closed Bug 1453227 Opened 2 years ago Closed 2 years ago

Occasionally, checked property of checkbox in preferences is different from its visual state

Categories

(Firefox :: Preferences, defect, P5)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: hectorz, Assigned: hectorz)

References

Details

(Keywords: regression)

Attachments

(1 file)

I don't have a good STR, but I think the problem is `checked` property is testing whether the `checked` attributed is `true`[1], but the attribute may be set to `checked` as part of bug 1430391[2]

[1]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/widgets/checkbox.xml#48
[2]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/preferencesBindings.js#368-369
Yeah, usually the presence of the "checked" attribute - whatever the value - is enough for the checked property to be true. 
The getter in checkbox.xml could be fixed (to use hasAttribute("checkec")), but as this is *very* old code, its likely we have other code working around this peculiarity. 

Also, some of the comments there could be improved 

The checkbox binding is not long for this world (bug 1397874), but in order to remove it should address this inconsistency - so although we've lived with this for > 15 years, it may still be worthwhile to fix.
Priority: -- → P5
I pushed to try to see what the impact might be with those changes to the checkbox and listbox bindings: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aadfc96e3fe61e566cbea969bbb8ea4b482aaeb8
(In reply to Sam Foster [:sfoster] from comment #2)
> The getter in checkbox.xml could be fixed (to use hasAttribute("checkec")),
> but as this is *very* old code, its likely we have other code working around
> this peculiarity. 

Do you think we should change the code in [1] to set `checked` with `true`, before the inconsistencies are properly fixed?

[1]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/preferencesBindings.js#368-369
(In reply to Hector Zhao [:hectorz] from comment #4)
> 
> Do you think we should change the code in [1] to set `checked` with `true`,
> before the inconsistencies are properly fixed?
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/
> preferencesBindings.js#368-369

We could do that, yes. The try run shows a pretty large number of failures from "fixing" the bindings to check the attribute presence rather than its value. It might be better to file a bug dedicated to making that consistent, leaving this bug to fix the observed problem in the interim.
Assignee: nobody → bzhao
Status: NEW → ASSIGNED
STR:

1. Open Firefox Nightly;
2. Open about:preferences (either by typing url or through the hamburger menu);
3. Click the "Home" category;
4. Click any of the "Web Search", "Top Sites", "Highlights", "Snippets" checkboxes, and it will stay checked on first click.

This happens to the checkboxes added to about:preferences by an extension (in this case, activity stream).
Comment on attachment 8968753 [details]
Bug 1453227 - set 'checked' attribute to 'true' to appease checkbox and listitem-checkbox bindings.

https://reviewboard.mozilla.org/r/237450/#review243496

Thanks for the clear STR. As discussed, this seems a reasonable interim solution.
Attachment #8968753 - Flags: review?(sfoster) → review+
:jaws, as I'm not a peer, can you also bless this before I hit the merge button? See Comment #5 for why this rather than a more thorough (and risky) fix.
Flags: needinfo?(jaws)
Comment on attachment 8968753 [details]
Bug 1453227 - set 'checked' attribute to 'true' to appease checkbox and listitem-checkbox bindings.

https://reviewboard.mozilla.org/r/237450/#review243502
Attachment #8968753 - Flags: review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afa738308f06
set 'checked' attribute to 'true' to appease checkbox and listitem-checkbox bindings. r=jaws,sfoster
Flags: needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/afa738308f06
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Verified with Firefox 61.0a1 build 20180419224145 on Win10 64 bit.
Status: RESOLVED → VERIFIED
Comment on attachment 8968753 [details]
Bug 1453227 - set 'checked' attribute to 'true' to appease checkbox and listitem-checkbox bindings.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1430391
[User impact if declined]: Extension-added checkboxes may not work properly when opening about:preferences for the first time.
[Is this code covered by automated tests?]: Not for this patch
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Desktop QA in Beijing will verify this if approved, also see comment 7 above for STR
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very minimal change
[String changes made/needed]: None
Attachment #8968753 - Flags: approval-mozilla-beta?
(In reply to Hector Zhao [:hectorz] from comment #14)
> [Needs manual test from QE? If yes, steps to reproduce]: Desktop QA in
> Beijing will verify this if approved, also see comment 7 above for STR

Hmmm, checkboxes mentioned in comment 7 doesn't exist in Fx 60 Beta yet, but this could be verified with the shield studies one in en-* build.
I think it is better to let this change ride the train with 61. We are two weeks away from release so it's very late in the beta cycle for uplift. It also sounds like this feature isn't fully implemented in 60. When I tried the STR in Nightly, I could still get the box to be checked by clicking a 2nd time on the checkbox when the first click didn't work.
Attachment #8968753 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Blocks: 1447893
Thanks for fixing this. Activity stream added the checkboxes in bug 1404890 for 61 with no intention to uplift that to 60.
You need to log in before you can comment on or make changes to this bug.