Open Bug 1697092 Opened 7 months ago Updated 21 days ago

Password autocomplete menu not using the same design/color scheme as context menus with Proton

Categories

(Toolkit :: Password Manager, enhancement, P3)

Firefox 88
enhancement

Tracking

()

People

(Reporter: daphnei.mee, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-door-hangers])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:86.0) Gecko/20100101 Firefox/86.0

Steps to reproduce:

Enable proton in nightly and try to autofill a password with lockwise

Actual results:

The password window appears, but the color scheme and design language does not match other context menus that browser.proton.contextmenus.enabled enables

Expected results:

The autofill box should use the same design language and color scheme as any other context menu under Proton

Version: Firefox 86 → Firefox 88

The Bugbug bot thinks this bug should belong to the 'Toolkit::Password Manager' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Component: Untriaged → Password Manager
Product: Firefox → Toolkit

:mconley, can you see this gets triaged? I'm not aware of a plan to touch the autocomplete menus

Flags: needinfo?(mconley)
Summary: Password autofilll not using the same design/color scheme as other context menus with Proton → Password autocomplete menu not using the same design/color scheme as context menus with Proton

I'm also unaware of such plans. I'll raise it.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mconley)
Keywords: helpwanted
Priority: -- → P3
Whiteboard: [proton-door-hangers]
Severity: -- → N/A
Assignee: nobody → tgiles
Status: NEW → ASSIGNED
Keywords: helpwanted
Attachment #9211890 - Attachment description: WIP: Bug 1697092 - Update password autocomplete popup menu to Proton spec. → Bug 1697092 - Update password autocomplete popup menu to Proton spec.
Duplicate of this bug: 1699993
Attachment #9211890 - Attachment description: Bug 1697092 - Update password autocomplete popup menu to Proton spec. → WIP: Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry,desktop-theme-reviewers
Attachment #9211890 - Attachment description: WIP: Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry,desktop-theme-reviewers → Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry
Attachment #9211890 - Attachment description: Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry → WIP: Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry
See Also: → 1705507

Hey Sam, I'd like to get this into 90 or so and not sure who to consult in order to get this to land. Thanks!

Flags: needinfo?(sfoster)

Im not sure I'll be finishing this, but next steps to move this forward are on me, so I'll assign myself for now.

Assignee: tgiles → sfoster
Flags: needinfo?(sfoster)

There seems to be a couple of ways to get menuseparators into the autocomplete popup. The first is to create actual result items for the separators, right alongside the creation of the footer and generated-password items:

diff --git a/toolkit/components/passwordmgr/LoginAutoComplete.jsm b/toolkit/components/passwordmgr/LoginAutoComplete.jsm
--- a/toolkit/components/passwordmgr/LoginAutoComplete.jsm
+++ b/toolkit/components/passwordmgr/LoginAutoComplete.jsm
@@ -63,6 +63,11 @@ XPCOMUtils.defineLazyPreferenceGetter(
   "SHOULD_SHOW_ORIGIN",
   "signon.showAutoCompleteOrigins"
 );
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "gProtonEnabled",
+  "browser.proton.enabled"
+);
 
 XPCOMUtils.defineLazyGetter(this, "log", () => {
   return LoginHelper.createLogger("LoginAutoComplete");
@@ -216,6 +221,12 @@ class LoginAutocompleteItem extends Auto
   }
 }
 
+class SeparatorAutocompleteItem extends AutocompleteItem {
+  constructor() {
+    super("separator");
+  }
+}
+
 class GeneratedPasswordAutocompleteItem extends AutocompleteItem {
   constructor(generatedPassword, willAutoSaveGeneratedPassword) {
     super("generatedPassword");
@@ -363,6 +374,10 @@ function LoginAutoCompleteResult(
   // The footer comes last if it's enabled
   if (isFooterEnabled()) {
     if (generatedPassword) {
+      if (this._rows.length && gProtonEnabled) {
+        // separate the generated password item from any content above it
+        this._rows.push(new SeparatorAutocompleteItem());
+      }
       this._rows.push(
         new GeneratedPasswordAutocompleteItem(
           generatedPassword,
@@ -382,6 +397,10 @@ function LoginAutoCompleteResult(
       this._rows.push(new ImportableLearnMoreAutocompleteItem());
     }
 
+    if (this._rows.length && gProtonEnabled) {
+      // separate the footer from any content above it
+      this._rows.push(new SeparatorAutocompleteItem());
+    }
     this._rows.push(
       new LoginsFooterAutocompleteItem(hostname, telemetryEventData)
     );

The SeparatorAutocompleteItems can then be turned into <menuseparator> elements in _appendCurrentResult That mostly works, though it creates some off-by-one errors when interacting with the menu that would need to be addressed.

But, the separators are really presentation details and this seems weird. Plus, it would potentially require GeckoView and any other consumers to know about and handle these separator result items.

The other option seems to be to insert them directly in MozElements.MozAutocompleteRichlistboxPopup's _appendCurrentResult. We could have similar conditional logic there to insert or not depending on whether there are items before the footer and generated-password item. But at that point, it might make more sense to insert them unconditionally and show/hide them in CSS. There might still be off-by-one errors here to address, there's kind of a baked-in assumption that the menu list children are 1:1 with the satchel/AC results.

Modifying _appendCurrentResult would be a bit fiddly. It tries really hard to reuse elements from the previous result and care needs to be taken to ensure we don't try to display a login item in a menuseparator, or vice-versa. Also, the height calculations might need adjustment to accommodate the separators

Either way, the <menuseparator>s do meet all the requirements outlined in :harry's review of the current patch, and something like this is the way to go.

Assignee: sfoster → nobody
Status: ASSIGNED → NEW
Blocks: 1626913
See Also: → 1696132
You need to log in before you can comment on or make changes to this bug.