Closed
Bug 1268848
Opened 8 years ago
Closed 8 years ago
Safe Browsing preferences graying out inconsistency
Categories
(Firefox :: Settings UI, defect, P1)
Firefox
Settings UI
Tracking
()
Tracking | Status | |
---|---|---|
firefox46 | --- | unaffected |
firefox47 | --- | unaffected |
firefox48 | --- | verified |
firefox49 | --- | verified |
People
(Reporter: pauly, Assigned: rhankins, Mentored)
References
Details
(Keywords: regression, Whiteboard: [fxprivacy] [good first bug])
Attachments
(2 files, 2 obsolete files)
14.45 KB,
image/png
|
Details | |
2.76 KB,
patch
|
gcp
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[Note]: - follow-up of bug 874408 [Affected versions]: - 48.0a2 (2016-04-29), 49.0a1 (2016-04-28) [Affected platforms]: - all [Steps to reproduce]: 1. Start Firefox 2. Open about:preferences#security 3. Check all the 3 options from General 4. Uncheck option #2 => option #3 grayed out. 5. Uncheck/recheck option #1 [Expected result]: - Option #3 grayed out [Actual result]: - Option #3 enabled, even if option #2 is disabled [Regression range]: - introduced by bug 874408
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy] [good first bug]
Comment 2•8 years ago
|
||
This one should be easy to fix: In preferences/in-content/security.js#168[0] we enable the blockUncommonUnwanted element. However, this should only be done if blockDownloads is also checked. Then, in the assert tests/tests/browser_security.js#47[1] we should make sure the assert also checks against blockDownloads.checked. [0]https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/security.js#168 [1]https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_security.js#47
Mentor: jhofmann
Assignee | ||
Comment 3•8 years ago
|
||
Proposed patch. I believe the security.js javascript is ok, and I was able to test it but I'm unsure how to test or verify the browser_security.js assert.
Comment 4•8 years ago
|
||
Hi Ryan! Thanks for contributing! You can find more information about these tests here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest tl;dr just run "./mach test browser/components/preferences/in-content/tests/browser_security.js" If I'm not mistaken your test should fail, because I believe it should test against !blockDownloads.checked instead. Can you verify and update your patch? Thanks!!
Assignee: nobody → rhankins
Assignee | ||
Comment 5•8 years ago
|
||
I was able to run the tests; thanks. It seems like box 3 should be disabled if either box 1 is unchecked or box 2 is unchecked. This check, however, fails: is (blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly"); What am I missing?
Updated•8 years ago
|
Keywords: regression
Comment 6•8 years ago
|
||
The expression you're looking for is "checked || !blockDownloads.checked" if I'm not mistaken. Sorry for that, I can understand that usage of the "checked" variable here is pretty confusing. If you feel like cleaning up you could add "checked = checkbox.checked;" in line 44 and then switch the "checked" variable in the two following assertions (like in your previous message). Let me know how that works out or if you have any other questions :)
Assignee | ||
Comment 7•8 years ago
|
||
Ok, that makes sense to me (finally) -- the test code toggles the checkbox, so then it uses the inverted checked. Thanks for the help; I'm sure you could have done this a lot faster yourself, but I understand it now. Tests pass with this patch.
Attachment #8747395 -
Attachment is obsolete: true
Comment 8•8 years ago
|
||
(In reply to Ryan Hankins from comment #7) > Created attachment 8748169 [details] [diff] [review] > 1268848-rev2.patch > > Ok, that makes sense to me (finally) -- the test code toggles the checkbox, > so then it uses the inverted checked. Thanks for the help; I'm sure you > could have done this a lot faster yourself, but I understand it now. > > Tests pass with this patch. Great, glad I could help you, that's what mentored bugs are for!
Comment 9•8 years ago
|
||
Comment on attachment 8748169 [details] [diff] [review] 1268848-rev2.patch gcp, would you have time to quickly review this?
Attachment #8748169 -
Flags: review?(gpascutto)
Comment 10•8 years ago
|
||
Comment on attachment 8748169 [details] [diff] [review] 1268848-rev2.patch Review of attachment 8748169 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/tests/browser_security.js @@ +43,5 @@ > "safebrowsing.malware.enabled is set correctly"); > > // check if the other checkboxes have updated > is(blockDownloads.hasAttribute("disabled"), checked, "block downloads checkbox is set correctly"); > + is(blockUncommon.hasAttribute("disabled"), checked || !blockDownloads.checked, "block uncommon checkbox is set correctly"); As pointed out this is hella confusing but I understand from the discussion in the bug that's correct.
Attachment #8748169 -
Flags: review?(gpascutto) → review+
Comment 11•8 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10) > > As pointed out this is hella confusing but I understand from the discussion > in the bug that's correct. Thanks! I totally agree. Ryan, we could push this now, but I'd love if you could follow my suggestion in comment #6 and make the code easier to read by reading the "checked" variable again in line 44. What do you think? :)
Flags: needinfo?(rhankins)
Assignee | ||
Comment 12•8 years ago
|
||
It's much easier to follow this way.
Attachment #8748169 -
Attachment is obsolete: true
Comment 13•8 years ago
|
||
Comment on attachment 8748230 [details] [diff] [review] 1268848-rev3.patch Review of attachment 8748230 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/tests/browser_security.js @@ +44,5 @@ > > // check if the other checkboxes have updated > + checked = checkbox.checked; > + is(blockDownloads.hasAttribute("disabled"), !checked, "block downloads checkbox is set correctly"); > + is(blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly"); Shouldn't these read "unset" or "unchecked" rather than "set"?
Attachment #8748230 -
Flags: review+
Comment 14•8 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13) > Comment on attachment 8748230 [details] [diff] [review] > 1268848-rev3.patch > > Review of attachment 8748230 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/preferences/in-content/tests/browser_security.js > @@ +44,5 @@ > > > > // check if the other checkboxes have updated > > + checked = checkbox.checked; > > + is(blockDownloads.hasAttribute("disabled"), !checked, "block downloads checkbox is set correctly"); > > + is(blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly"); > > Shouldn't these read "unset" or "unchecked" rather than "set"? Well they're either set or unset, depending on if the checkbox.
Flags: needinfo?(rhankins)
Comment 15•8 years ago
|
||
Ok, now we will need a Try push to see if everything's still working as it should. You can find more details here: https://wiki.mozilla.org/ReleaseEngineering/TryServer I'll do this for you for now. If you want to work on more bugs in the future you should think about getting your own Try access! Another thing: Please format your commit message according to our commit message conventions. Details here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #15) > Ok, now we will need a Try push to see if everything's still working as it > should. You can find more details here: > https://wiki.mozilla.org/ReleaseEngineering/TryServer > > I'll do this for you for now. If you want to work on more bugs in the future > you should think about getting your own Try access! > > Another thing: Please format your commit message according to our commit > message conventions. Details here: > https://developer.mozilla.org/en-US/docs/Mercurial/ > Using_Mercurial#Commit_Message_Conventions Ok, thanks -- that was fun.
Comment 18•8 years ago
|
||
(In reply to Ryan Hankins from comment #17) > > Ok, thanks -- that was fun. Great that you liked it! :) I'll add the "checkin-needed" flag so that this will get pushed into the tree if the try looks ok. We would obviously be happy if you'd keep contributing even more! The only bug I'm mentoring right now is Bug 1194874. That one might be a little too easy. You could still do it to get into the contributing workflow or leave it for someone else, it's up to you :) Another great way of finding good bugs to work on is Bugs Ahoy (http://www.joshmatthews.net/bugsahoy). I know for certain that e.g. the DevTools people have a great number of ESLint errors that they need help fixing (http://www.joshmatthews.net/bugsahoy/?devtools=1&unowned=1).
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6b0b3807493f
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6b0b3807493f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•8 years ago
|
Iteration: --- → 49.1 - May 9
Priority: P3 → P1
Reporter | ||
Comment 21•8 years ago
|
||
Verified fixed FX 49.0a1 (2016-05-08) Win 7
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
Comment 22•8 years ago
|
||
Want to request uplift to aurora for this? The regression is from 48 so it would be nice to fix before the regression ships to release.
Flags: needinfo?(rhankins)
Comment 23•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22) > Want to request uplift to aurora for this? The regression is from 48 so it > would be nice to fix before the regression ships to release. Sure, thanks! How do we do that? :)
Flags: needinfo?(rhankins) → needinfo?(lhenry)
Comment 24•8 years ago
|
||
From the view of the patch in bugzilla, set the flag for mozilla-aurora-approval to "?" and fill out the questions that will pop up in the latest bugzilla comment. It's some questions about risk, the cause of the regression, test coverage, and so on. Uplift and approvals are described here, https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments
Flags: needinfo?(lhenry)
Comment 25•8 years ago
|
||
Comment on attachment 8748230 [details] [diff] [review] 1268848-rev3.patch Approval Request Comment [Feature/regressing bug #]: Bug 874408, which changes the safe browsing settings [User impact if declined]: Through a bit of clicking users might get to a situation where the UI is not reflecting the actual safe browsing settings. Confusing but not harmful. [Describe test coverage new/current, TreeHerder]: Automated tests and QA verified [Risks and why]: This is a really simple change that only affects the settings UI. If anything were wrong, the safe browsing settings UI would malfunction. Given the testing around this it's highly unlikely that will happen, though. [String/UUID change made/needed]:none
Attachment #8748230 -
Flags: approval-mozilla-aurora?
Comment 26•8 years ago
|
||
Comment on attachment 8748230 [details] [diff] [review] 1268848-rev3.patch Fix a new regression, taking it
Attachment #8748230 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d52c5d23e08d
Reporter | ||
Comment 28•8 years ago
|
||
Verified fixed FX 48.0a2 (2016-06-05) Win 7
You need to log in
before you can comment on or make changes to this bug.
Description
•