Closed Bug 1598717 Opened 11 months ago Closed 11 months ago

Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login

Categories

(Toolkit :: Password Manager, defect, P1)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 + verified
firefox73 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(5 files)

Bug 1566536 is adding "{brandShorterName} will save this password for this website." to the password generation autocomplete menu but that text shouldn't show if we won't actually auto-save due to an existing saved login with an empty username:

(Quoting katieC from bug 1566536 comment #9)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from bug 1566536 comment #8)

Ugh… I forgot about an edge-case… if we already have a login with no username saved for this site then we won't auto-save but the string will still claim that we will… should we show/hide the "Firefox will save this password for this website" string depending on whether we will automatically save?

Yes, if we autosave the generated password - show string. If we don't autosave it - hide string.

Flags: qe-verify+

I will also use this bug to tweak the styling with Katie's help to better match the spec.

We can also discuss Sam's comment:

<!ENTITY useGeneratedPassword.label "Use a Securely Generated Password…">

Nit? This seems a little redundant as the next menu (the AC popup) isn't just the generated password UI, it also has matching saved logins. I guess that's why Chrome has "Suggest password..."

Hey Matt,

Given that this does affect the interaction with Password Generation in Private Mode, will it be fixed and uplifted soon enough for FX72 Beta testing?
Beta testing would be in the time-frame of next week and it would be good to know if these changes are also targeted to go along with Bug 1566536 or not.

Flags: needinfo?(MattN+bmo)

Yes, it should be fixed in the next day or two and uplifted ASAP.

Flags: needinfo?(MattN+bmo)

Also increase the min-width for password generation popups so at least en-US doesn't wrap.

Depends on D55742

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/c1aa563394ec
Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/daf59b9083fb
Wrap the "Firefox will save this password for this website" text. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/58bfe894a732
Make the line-height of the 3 password generation lines consistent. r=sfoster
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Hey Matt,

The message is indeed hidden when there is already a saved entry with "No username".

However, this seemed to cause a regression for the autocomplete dropdown, see the attached recording.
I can reproduce that every time with STR:

  • Have username autofilled with a saved entry
  • Toggle the autocomplete dropdown containing the "Use a Securely generated password" option (via context menu) for the password field
  • Click over to the username field and check out the autocomplete dropdown

This can't be reproduced in the current Nightly (that doesn't have this patch).
Reproducible on the build I took from: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedJob=279717536

Build ID: 20191205041108
Nighty 73.0a1 Windows 10 x64.
Let me know if you need a new bug submitted or it will be handled here.

Flags: needinfo?(MattN+bmo)

[Tracking Requested - why for this release]: Thanks I will try fix this. This will likely also need to be uplifted but I've been busy with other projects.

The fix was trivial and can land ASAP. Since it should have been fairly clear that comment 10 was reporting a bug, it would have been good if a bug was filed but we don't need one now.

Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/495ec19bdb58
Only set display:block on password generation richlistitems that aren't collapsed.

Timea can you re-verify?

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Keywords: regression

Yes, I will verify it once it lands. Leaving NI? for me to remember.

Flags: needinfo?(tbabos)

Comment on attachment 9113336 [details]
Bug 1598717 - Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login. r=sfoster

Beta/Release Uplift Approval Request

  • User impact if declined: A user will be told "Firefox will save this password for this website" even when that's not the case. The message would also not wrap and therefore would truncated with an ellipsis in some languages. The spacing of the three lines of text would also be incorrect.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Steps are clear for QA
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk with verification from QA.
  • String changes made/needed: None
Attachment #9113336 - Flags: approval-mozilla-beta?
Attachment #9113337 - Flags: approval-mozilla-beta?
Attachment #9113338 - Flags: approval-mozilla-beta?
Attachment #9115363 - Flags: approval-mozilla-beta?

Reproduced the issue with an old Nightly build 73.0a1 (Build id: 20191212095326).
Verified - Fixed in latest Nightly build 73.0a1 (Build id: 20191212215040).

Flags: needinfo?(tbabos)
QA Whiteboard: [qa-triaged]

Comment on attachment 9113336 [details]
Bug 1598717 - Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login. r=sfoster

password manager fixes, approved for 72.0b7

Attachment #9113336 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9113337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9113338 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9115363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

During verification it might be worth looking at the dropdown in other locales (for a possible followup bug)?

(In reply to Julien Cristau [:jcristau] from comment #24)

During verification it might be worth looking at the dropdown in other locales (for a possible followup bug)?

Sure, we will also verify the fix in other locales.

Verified - Fixed in latest Beta build 72.0b7 (Build id: 20191213132525). Verified also with some different locales: de, fr, it, tr, ar (RTL).

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.