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

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[Matthew N. [:MattN]]

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

Comment 2

[Matthew N. [:MattN]]

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

Comment 3

[Timea Cernea [:tbabos][inactive]]

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.

Comment 4

[Matthew N. [:MattN]]

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

Comment 5

[Matthew N. [:MattN]]

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

Comment 6

[Matthew N. [:MattN]]

Attachment: Bug 1598717 - Wrap the "Firefox will save this password for this website" text. r=sfoster

Depends on D55741

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1598717 - Make the line-height of the 3 password generation lines consistent. r=sfoster

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

Depends on D55742

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c1aa563394ec
https://hg.mozilla.org/mozilla-central/rev/daf59b9083fb
https://hg.mozilla.org/mozilla-central/rev/58bfe894a732

Comment 10

[Timea Cernea [:tbabos][inactive]]

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.

Comment 11

[Timea Cernea [:tbabos][inactive]]

Attachment: Autocomplete dropdown regression

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Following up in email.

Comment 13

[Matthew N. [:MattN]]

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

Comment 14

[Matthew N. [:MattN]]

Attachment: Bug 1598717 - Only set display:block on password generation richlistitems that aren't collapsed.

Comment 15

[Matthew N. [:MattN]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/495ec19bdb58
Only set display:block on password generation richlistitems that aren't collapsed.

Comment 17

[Julien Cristau [:jcristau]]

Timea can you re-verify?

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 19

[Timea Cernea [:tbabos][inactive]]

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

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/495ec19bdb58

Comment 21

[Matthew N. [:MattN]]

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

Comment 22

[Alin Ilea, Desktop Test Engineering]

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

Comment 23

[Julien Cristau [:jcristau]]

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

Comment 24

[Julien Cristau [:jcristau]]

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

Comment 25

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/19ea31d1f07f
https://hg.mozilla.org/releases/mozilla-beta/rev/ff6f7529cea4
https://hg.mozilla.org/releases/mozilla-beta/rev/fcf76df603c5
https://hg.mozilla.org/releases/mozilla-beta/rev/1ac4dd30c332

Comment 26

[Alin Ilea, Desktop Test Engineering]

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

Comment 27

[Alin Ilea, Desktop Test Engineering]

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