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

Other (inconsistent) getter for `checked`: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/widgets/button.xml#40 https://searchfox.org/mozilla-central/rev/6bfadf95b4a6/toolkit/content/widgets/listbox.xml#1089-1090

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.

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.

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.

: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.

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

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

Verified with Firefox 61.0a1 build 20180419224145 on Win10 64 bit.

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

(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.

Thanks for fixing this. Activity stream added the checkboxes in bug 1404890 for 61 with no intention to uplift that to 60.