Closed Bug 1767367 Opened 2 months ago Closed 2 months ago

Fix inverted useDownloadDir pref in about:preferences

Categories

(Firefox :: Preferences, defect, P1)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox100 - unaffected
firefox101 + verified
firefox102 + verified

People

(Reporter: aminomancer, Assigned: aminomancer)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

I forgot to replace value with inverted in bug 1762775. This is just to fix that.

I forgot to add a property in bug bug 1762775.
This patch just resolves that.

How does this manifest in practice? Is the pref actually backwards from the user's perspective? I suppose we will need to uplift this?

Flags: needinfo?(shmediaproductions)

(In reply to Dão Gottwald [::dao] from comment #2)

How does this manifest in practice? Is the pref actually backwards from the user's perspective?

Yes

I suppose we will need to uplift this?

Yes

Type: task → defect
Flags: needinfo?(shmediaproductions)

Comment on attachment 9274749 [details]
Bug 1767367 - Fix inverted useDownloadDir pref. r=dao

Beta/Release Uplift Approval Request

  • User impact if declined: Wrong preference state in user-visible preferences
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Check that the "Always ask you where to save files" checkbox behaves as expected in the Firefox settings
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Trivial equivalent of a missing ! operator addition in markup + JS
  • String changes made/needed: No
  • Is Android affected?: No
Attachment #9274749 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Severity: -- → S1
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → Desktop
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f0946689c7ef
Fix inverted useDownloadDir pref. r=dao,preferences-reviewers,Gijs
Has Regression Range: --- → yes

Yeah it's just reversed. Thanks and sorry I overlooked this 😳

Is this something we can add a test for?

Flags: needinfo?(shmediaproductions)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Is this something we can add a test for?

I mean, as I noted on phabricator, we don't normally test that a checkbox pref "does what it says on the tin" - the implementation is shared amongst all checkbox, and really the test can't check that the prose English label of the checkbox maps to the "mental model" of the boolean pref "for real". We could add a test specific to this checkbox but it's unlikely to be updated in future, and it'd be just as likely that the entire UI changes and/or the wording gets changed (in which case, the test might pass even if we want to flip the meaning of the checkbox, if we forget to do the implementation part of it). So tbh I don't think this is useful.

Flags: needinfo?(shmediaproductions)
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Would this be a good candidate for a 100.1 release should one come out?

If the regressing bug was in fact bug 1762775, this wouldn't affect 100. If you're saying that 100 is actually affected, then it sounds like we need to revisit the regressing bug.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

If the regressing bug was in fact bug 1762775, this wouldn't affect 100.

Yeah it is definitely bug 1762775 — prior to that, the value attribute did basically the same thing since it was a radiogroup, so it was semantically correct. I know for sure it was my mistake, I was worried about random testing stuff instead of actually reading the text lol.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

If the regressing bug was in fact bug 1762775, this wouldn't affect 100. If you're saying that 100 is actually affected, then it sounds like we need to revisit the regressing bug.

Ugh, my mistake. I didn't see from comment 9 that 100 is not affected. Sorry for the noise.

Set release status flags based on info from the regressing bug 1762775

Comment on attachment 9274749 [details]
Bug 1767367 - Fix inverted useDownloadDir pref. r=dao

Approved for 101.0b2.

Attachment #9274749 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

I have reproduced this issue using Firefox 101.0b1 on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 101.0b2 and Firefox 102.0a1 on Win 10 x64, Ubuntu 20.04 x64 and macOS 10.15.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.