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

Status: NEW

(Toolkit :: Password Manager, enhancement, P3)

Description

[u658943]

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

Comment 1

[u658943]

Attachment: Screenshot which depicts the issue

Comment 2

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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

Comment 5

[Tim Giles [:tgiles]]

Attachment: WIP: Bug 1697092 - Update password autocomplete popup menu to Proton spec. r=harry

Comment 7

[Tim Giles [:tgiles]]

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!

Comment 8

[Sam Foster [:sfoster] (he/him)]

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.

Comment 9

[Sam Foster [:sfoster] (he/him)]

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.