Currently autocomplete UI is only shown on username fields but it would be useful to also show it on <input type=password> for cases where there is no username field present. I think it should always be shown regardless of whether there is a username field (Safari doesn't open it automatically, only when the icon in the field is clicked) since if the user autofills from the username field then they likely won't focus the password field at all and it simplifies the implementation. Having this appear on password fields will likely be useful for password generation and insecure password UI.

What is the autocomplete UI on password fields supposed to look like? We can't show the actual passwords, so what do we show?

(In reply to Tanvi Vyas [:tanvi] from comment #1) > What is the autocomplete UI on password fields supposed to look like? We > can't show the actual passwords, so what do we show? Use the same logic (share the code) as the Fill Login context menu (so we don't have to make extra work) which shows the username if there is one, otherwise another descriptor to disambiguate. There is room for improvement with that context menu logic but we should share the logic for now and improve the common helper later as desired.

Our context menu logic also aligns with Safari IIRC since it also uses a date as a distinguishing factor when there is no username (see the first item): http://grab.by/T6qs

(In reply to Matthew N. [:MattN] (away until Oct. 4) from comment #2) > (In reply to Tanvi Vyas [:tanvi] from comment #1) > > What is the autocomplete UI on password fields supposed to look like? We > > can't show the actual passwords, so what do we show? > > Use the same logic (share the code) as the Fill Login context menu (so we > don't have to make extra work) which shows the username if there is one, > otherwise another descriptor to disambiguate. There is room for improvement > with that context menu logic but we should share the logic for now and > improve the common helper later as desired. This sounds good to me. And I also like the Safari UI. Ryan also mentioned using a date as an indicator. So: * Use the Fill Login context menu * If there is a username, show that * If there is no username, show "Password saved" followed by the date. Ryan, does that sound good to you? What format should the date be in? 01/02/2016 01/2016 01/16 What about countries that put the date before the month? Do we use "Today" and "Yesterday" when appropriate Should we use "This month", "Last month", "Last year", or be more specific with dates?

(In reply to Tanvi Vyas [:tanvi] from comment #4) > (In reply to Matthew N. [:MattN] (away until Oct. 4) from comment #2) > > (In reply to Tanvi Vyas [:tanvi] from comment #1) > > > What is the autocomplete UI on password fields supposed to look like? We > > > can't show the actual passwords, so what do we show? > > > > Use the same logic (share the code) as the Fill Login context menu (so we > > don't have to make extra work) which shows the username if there is one, > > otherwise another descriptor to disambiguate. There is room for improvement > > with that context menu logic but we should share the logic for now and > > improve the common helper later as desired. > > This sounds good to me. And I also like the Safari UI. Ryan also mentioned > using a date as an indicator. > > So: > * Use the Fill Login context menu > * If there is a username, show that > * If there is no username, show "Password saved" followed by the date. > > Ryan, does that sound good to you? > > > What format should the date be in? > 01/02/2016 > 01/2016 > 01/16 We should use the exact same code as we use for the context menu. It already uses locale aware dates. > What about countries that put the date before the month? That's already handled with the context menu code. > Do we use "Today" and "Yesterday" when appropriate > Should we use "This month", "Last month", "Last year", or be more specific > with dates? We don't have support for this yet in the platform. See bug 1186262.

I'll discuss with Philipp. Assuming that the user can only save one username-less password per site, I am also assuming there are two cases: 1. Where there is only one credential saved (password, no username) 2. There are multiple credentials saved (one password with no username, and one or more other logins) For #1 I imagine that we don't need to show the date, just get permission to fill the password. For #2, it makes sense copy to copy the copy from the content menu (see attached). Make sense?

(In reply to Ryan Feeley [:rfeeley] from comment #6) > Created attachment 8800160 [details] > no-username.png > > I'll discuss with Philipp. Assuming that the user can only save one > username-less password per site, I am also assuming there are two cases: > > 1. Where there is only one credential saved (password, no username) > 2. There are multiple credentials saved (one password with no username, and > one or more other logins) > > > For #1 I imagine that we don't need to show the date, just get permission to > fill the password. > For #1, we just have a dropdown menu that says: Fill Password > For #2, it makes sense copy to copy the copy from the content menu (see > attached). > > Make sense? For #2, do we copy the context menu exactly? So if I have 3 passwords saved (for example) with no username, the dropdwon menu on the password box would say: No username (1/1/2016) No username (1/1/2015) No username (1/1/2014) If that's the case, it doesn't sound like we would need any string changes. Ryan and Matt, can you both confirm? As for the "View Saved Logins" option, I think that is being added in another bug https://bugzilla.mozilla.org/show_bug.cgi?id=1189618 so won't happen here in this bug.

> For #2, do we copy the context menu exactly? So if I have 3 passwords saved > (for example) with no username, the dropdwon menu on the password box would > say: > No username (1/1/2016) > No username (1/1/2015) > No username (1/1/2014) I don't think this is possible. There can only be one saved password per domain, although there can be multiple logins (username + password). So the list would probably look like: > No username (1/1/2016) > username@domain1.com > username@domain2.com

Hi MattN, After reading the entry point codes that you suggested, I tried to summarize the code path and breakdown for password: * Registration of username and password field this._formFillService.markAsLoginManagerField [1] is for adding a username field for autocomplete. I suppose there would be a similar code for password: if (passwordField) { this._formFillService.markAsLoginManagerField(passwordField); } Do we need another function to handle password field or just reuse _formFillService.markAsLoginManagerField? * loginsFound -> Handle search event/request I think receiveMessage[2] and loginsFound[3] can be reused for password part. Do we need another object to handle password field or just reuse UserAutoCompleteResult[4]? * Handle popup showing event The message "FormAutoComplete:MaybeOpenPopup" can be for password part as well. There should be an argument (element) to distinguish password or username field in [5]. * Handle Enter key If it's a password input, the password should be filled into the input even the item is shown in username. Could you help to point out the reuse part and re-implement part? Thank you! [1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/passwordmgr/LoginManagerContent.jsm#1015 [2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/passwordmgr/LoginManagerContent.jsm#229,239 [3] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/passwordmgr/LoginManagerContent.jsm#518 [4] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/passwordmgr/LoginManagerContent.jsm#1204 [5] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/content/browser-content.js#1471 [6] https://dxr.mozilla.org/mozilla-central/rev/9079d167112122805f99f57bb8856e1b1675af0f/toolkit/components/satchel/AutoCompletePopup.jsm#246

(In reply to Ryan Feeley [:rfeeley] from comment #6) > Created attachment 8800160 [details] > no-username.png > > I'll discuss with Philipp. Assuming that the user can only save one > username-less password per site, We can only assume that until bug 1200472 (in progress) which I will finish within the next month.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #9) > Hi MattN, > > After reading the entry point codes that you suggested, I tried to summarize > the code path and breakdown for password: > * Registration of username and password field > this._formFillService.markAsLoginManagerField [1] is for adding a > username field for autocomplete. > I suppose there would be a similar code for password: > if (passwordField) { > this._formFillService.markAsLoginManagerField(passwordField); > } > Do we need another function to handle password field or just reuse > _formFillService.markAsLoginManagerField? I think it would be fine to re-use this method for password fields and (check the type attribute if we need to distinguish between username and password) but for now I think we should just attach the login manager autocomplete to all type=password, regardless and don't worry about marking. > * loginsFound -> Handle search event/request > I think receiveMessage[2] and loginsFound[3] can be reused for password > part. > Do we need another object to handle password field or just reuse > UserAutoCompleteResult[4]? I think we can re-use UserAutoCompleteResult but just have it's logic differ slightly if it's on a type=password field (which we may have to tell it) > * Handle popup showing event > The message "FormAutoComplete:MaybeOpenPopup" can be for password part > as well. There should be an argument (element) to distinguish password or > username field in [5]. I'm not sure we need to pass that information there since I think the getResultsFromController line above is where the return value would differ based on username vs. password. > * Handle Enter key > If it's a password input, the password should be filled into the input > even the item is shown in username. This should just be a matter of changing getValueAt to have a different value than getCommentAt from AutoCompleteTreeView which should happen in UserAutoCompleteResult: https://dxr.mozilla.org/mozilla-central/rev/7c8216f48c38a8498f251fe044509b930af44de6/toolkit/components/passwordmgr/LoginManagerContent.jsm#1249-1258 Right now the value and label are the same but the code should make them differ when the input is a password field. This may mean changing the constructor for UserAutoCompleteResult since I don't think it currently knows about the input. https://dxr.mozilla.org/mozilla-central/rev/7c8216f48c38a8498f251fe044509b930af44de6/toolkit/components/passwordmgr/LoginManagerParent.jsm#273-274

Marking as P1 because this needs to land in Firefox 52.

Matt, can you confirm that all the strings we need for this bug are already in the Context Menu? And hence this bug shouldn't require any new strings (or changes to existing ones)? Thanks!

https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties#59-65 The main two I can think of are: > loginHostAge=%1$S (%2$S) > noUsername=No username which are accessible from the passwordmgr directory so I think we're good unless there's another I'm not thinking of.

Hi Sean, Do you have any work in progress code for this patch? If so, can you post it here? Also, are you taking this bug for sure? If so, can you assign yourself?

Hi Tanvi, attachment 8807096 [details] is a WIP patch for showing an autocomplete popup at password field. In this patch, there is a "[password]" string at right side of a richitem in Paassword autocomplete popup. Do you think we should give autocomplete popup at password field a different style to distinguish username and password field?

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; Hi MattN, Thanks for your suggestions. Autocomplete popup for password field can be shown now. Here are the rest items should be implemented: 1. Fill password when selecting a richitem. 2. Refine the style based on comment 17 probably. If I miss anything, please share your thoughts.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review90424 Looks good overall! f+ For testing I would recommend: * a unit (xpcshell) test[1] testing the UserAutoCompleteResult implementation (specificcally combinations of isSecure and isPasswordField). * * e.g. ```let actual = new UserAutoCompleteResult(…, { isSecure: …, isPasswordField: …, }); equal(actual.getValueAt()…, …); equal(actual.getLabelAt()…, …); equal(actual.getStyleAt()…, …); …``` * Add a file like test_basic_form_autocomplete.html e.g. test_password_field_autocomplete.html but don't copy the whole file since I don't think we need many of the cases. Maybe test with and without the insecure warning pref on. [1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1034 (Diff revision 1) > // but even with one account we should refill if the user edits. > - if (usernameField) > + if (usernameField) { > this._formFillService.markAsLoginManagerField(usernameField); > + } > + > + if (passwordField) { We already bail `if (passwordField == null) {` on line 971 so the `if` isn't necessary. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1035 (Diff revision 1) > - if (usernameField) > + if (usernameField) { > this._formFillService.markAsLoginManagerField(usernameField); > + } > + > + if (passwordField) { > + this._formFillService.markAsLoginManagerField(passwordField); You should also be developing with "signon.autofillForms.http" set to False as that is what we will ship and you will see that this doesn't work since we bail above with `AUTOFILL_RESULT.INSECURE` recorded so this needs to move much higher up in the function. Maybe right after the null or readonly/disabled check of passwordField. I would prefer doing it before the readonly/disabled check so we can handle the field switching to be writeable/enabled but I don't know if there will be side effects (like handling events we shouldn't [e.g. focus] so please test that). ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1280 (Diff revision 1) > > getValueAt(index) { > if (index < 0 || index >= this.matchCount) > throw new Error("Index out of range."); > > + let appendPasswordString = this._isPasswordField ? " [PASSWORD]" : ""; My understanding was that the design would be the same and we wouldn't include any indication that the password would be filled in. Safari doesn't change their UI between username and password fields FWIW. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1286 (Diff revision 1) > + > if (this._showInsecureFieldWarning && index === 0) { > return ""; > } > > - return this.logins[index - this._showInsecureFieldWarning].username; > + return this.logins[index - this._showInsecureFieldWarning].username + appendPasswordString; > 1. Fill password when selecting a richitem. To fix that you just need to have getValueAt return .password instead of .username if `this._isPasswordField` and leave getLabelAt always returning .username. You may need to fix tests that assume that the value and label are the same but probably not since we didn't have autocomplete on password fields before. ::: toolkit/components/passwordmgr/nsLoginManager.js:487 (Diff revision 1) > // aPreviousResult is an nsIAutoCompleteResult, aElement is > // nsIDOMHTMLInputElement > > let form = LoginFormFactory.createFromField(aElement); > let isSecure = InsecurePasswordUtils.isFormSecure(form); > + let isPasswordField = aElement.getAttribute("type") == "password"; In general, never use an attribute if there is a property getter as the property is faster and does normalization. You want `aElement.type == "password"` ::: toolkit/components/passwordmgr/nsLoginManager.js:508 (Diff revision 1) > previousResult = null; > } > > let rect = BrowserUtils.getElementBoundingScreenRect(aElement); > let autoCompleteLookupPromise = this._autoCompleteLookupPromise = > LoginManagerContent._autoCompleteSearchAsync(aSearchString, previousResult, I don't think we want to filter by the search string on password fields as that discloses contents of the password (and we don't want to filter based on the username) and we either want to only show password suggestions when there is no search string or we want to always show all password suggestions for the domain regardless of the value of the password field. I think we should show the insecure warning regardless of the .value of the password field (e.g. when it's empty and as the user types). I think you will want to use an empty string for the search string to get all relevant results.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review90424 The latest patch is with the verification for the labels of each menu username/password items. I will implement xpcshell-based_unit_tests then. Besides, I think we should verify the "security.insecure_field_warning.contextual.enabled" === false case as well. I am trying to put all test cases with and without "security.insecure_field_warning.contextual.enabled" in the same file ("test_password_field_autocomplete.html") in the meanwhile. Do you think it's worth to do? I suppose the benfits will be easier to maintain and reducing the copied code. > You should also be developing with "signon.autofillForms.http" set to False as that is what we will ship and you will see that this doesn't work since we bail above with `AUTOFILL_RESULT.INSECURE` recorded so this needs to move much higher up in the function. Maybe right after the null or readonly/disabled check of passwordField. I would prefer doing it before the readonly/disabled check so we can handle the field switching to be writeable/enabled but I don't know if there will be side effects (like handling events we shouldn't [e.g. focus] so please test that). I am not sure if I understand correctly, so I confirmed my thoughts here. * Implement the case with "signon.autofillForms.http" set to False which shows the insecure warning entry without any logins. * Write a test to verify the behaviour of a password field which is writeable/readonly and enabled/diabled. BTW, I suppose Bug 1314478 is implementing the similiar thing. Should I consider it in this bug?

(In reply to Sean Lee [:seanlee][:weilonge] from comment #17) > Created attachment 8807099 [details] > sample design > > Hi Tanvi, > > attachment 8807096 [details] is a WIP patch for showing an autocomplete > popup at password field. > In this patch, there is a "[password]" string at right side of a richitem in > Paassword autocomplete popup. > Do you think we should give autocomplete popup at password field a different > style to distinguish username and password field? I believe we want to copy directly what the context menu does here. If you right click on a password field and have a saved login, you will see a Fill Password submenu. We should use what that submenu has (except we shouldn't include the "View Saved Logins" part, that is another bug). Here are the scenarios that describe what should happen when the user focuses on the password field: 1) If the user is on an HTTP page that has Saved Logins: * See warning, as you have in attached screenshot * See a list of usernames for all saved logins (i.e. don't include the "[PASSWORD]" string) * If there is a saved login with No username associated, use "No Username (Insert_Date_Saved)", as shown here: https://bug1289913.bmoattachments.org/attachment.cgi?id=8800160 2) If the user is on an HTTP page that has no Saved Logins: * See warning, as you have in attached screenshot 3) If the user is on an HTTPS page that has Saved Logins: * No warning in the drop down * See a list of usernames for all saved logins (i.e. don't include the "[PASSWORD]" string) * If there is a saved login with No username associated, use "No Username (Insert_Date_Saved)", as shown here: https://bug1289913.bmoattachments.org/attachment.cgi?id=8800160 4) If the user is on an HTTPS page that has no Saved Logins: No drop down Please let me know if you need more details.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; Add a new unit (xpcshell) test at toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review90424 > I am not sure if I understand correctly, so I confirmed my thoughts here. > * Implement the case with "signon.autofillForms.http" set to False which shows the insecure warning entry without any logins. > * Write a test to verify the behaviour of a password field which is writeable/readonly and enabled/diabled. > > BTW, I suppose Bug 1314478 is implementing the similiar thing. Should I consider it in this bug? What I was saying is that if you were manually testing with `signon.autofillForms.http` = False then you would have realized that your code wouldn't work so I'm suggesting you test with it set to False. First you can manually test the disabled/readonly and then add it to the automated tests. Bug 1314478 is similar but about username fields so the location of the markAsLoginManagerField may itentionally be different.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review91334 ::: toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html:380 (Diff revision 3) > + doKey("return"); > + yield spinEventLoop(); > + checkACFormPasswordField("testpass2"); > +}); > + > +add_task(function* test_form1_delete_second() { Can you reduce this test file to just cases that would reasonably be affected by this bug. I think there's a lot of duplication of cases which are testing how autocomplete works but some of that is covered in the original version of this code and others are covered by the unit test. I'm just concerned that this is going to be a maintenance problem with multiple copies of redundant testcases. ::: toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js:1 (Diff revision 3) > +Components.utils.import("resource://gre/modules/LoginManagerContent.jsm"); Nit: Cu ::: toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js:20 (Diff revision 3) > +let expectedResults = [ > + { > + insecureFieldWarningEnabled: true, > + isSecure: true, The test looks good :)

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; I guess we just have to do some testing with disabled/readonly and then manual testing sanity checks.

Hi MattN, Tanvi, thanks for Tanvi's explanation in comment 22, and that's pretty clear for me now. I suppose that we can do that in this bug if we can uplift this patch in aurora 52. This option needs more time for this bug. Another option is to fire a follow-up bug for the implementation of comment 22. This option gets this bug landed sooner. May I know your thoughts?

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; Here are some updates in the patch: 1. Consider the case with "signon.autofillForms.http" 2. Update test_user_autocomplete_result.js with "signon.autofillForms.http" and the case of readonly/disabled. 3. Update test_password_field_autocomplete with "signon.autofillForms.http" and "security.insecure_field_warning.contextual.enabled". However, the test of "test_password_field_autocomplete" is weird when changing the pref value between tasks. It seems only the first task will be affected with SpecialPowers.pushPrefEnv correctly. The following tasks are still in the pref env even I assigned a set of particular pref for each task. I am trying to fix this.

When I'm testing the insecure warning message in http://www.radiotunes.com/, the warning message can be enabled/disabled by toggling "security.insecure_field_warning.contextual.enabled" in about:config without reloading radiotunes page. So I suppose the pref env should be changed between the tasks in test_password_field_autocomplete.js. However, I need to find a solution in the test to make the pref env affected.

The issue mentioned in comment 30 and 31 is resolved with the solution of resetting the content (innerHTML) of each form to force the constructor of UserAutocompleteResult invoked at the task beginning.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #28) > Hi MattN, Tanvi, > > thanks for Tanvi's explanation in comment 22, and that's pretty clear for me > now. > > I suppose that we can do that in this bug if we can uplift this patch in > aurora 52. This option needs more time for this bug. > > Another option is to fire a follow-up bug for the implementation of comment > 22. This option gets this bug landed sooner. > > May I know your thoughts? Getting this bug landed sooner sounds better to me. But it is unclear what parts would be in the followup. We should at least remove the "[PASSWORD]" string from https://bug1289913.bmoattachments.org/attachment.cgi?id=8807099. Also, the drop down box looks misaligned in that screenshot, but perhaps you have resolved that. Can I have a screenshot based on your updated patch?

Hi Tanvi, here you go~

(In reply to Sean Lee [:seanlee][:weilonge] from comment #36) > Created attachment 8809224 [details] > the screenshot for the patch in comment 32 > > Hi Tanvi, here you go~ Hi Sean, In general, this looks good. Please go ahead with this and file a followup bug to use the Context Menu information here. What will happen if there is no username associated with a passwword? Will you see a blank entry in the autocomplete drop down? If so, I still think that is okay as long as we file, fix, and uplift the Context Menu bug. Thanks!

(In reply to Tanvi Vyas [:tanvi] from comment #37) > > Hi Sean, > > In general, this looks good. Please go ahead with this and file a followup > bug to use the Context Menu information here. Great! Except the follow-up bug we mentioned here, "Learn More" link should be fixed as well. The relative information is at the first item of bug 1217162 comment 32. > > What will happen if there is no username associated with a passwword? Will > you see a blank entry in the autocomplete drop down? If so, I still think > that is okay as long as we file, fix, and uplift the Context Menu bug. In the current patch, the password item without username is not in the autocomplete drop down. What should the blank entry be shown in "this patch"? "No username" with the date like the context menu? (https://bug1289913.bmoattachments.org/attachment.cgi?id=8800160) Eventually, we will have the design mentioned in comment 22. > > Thanks!

Please see the screenshot of the password without username.

Update the patch for fixing tests.

The latest patch is updated with the label of a password with empty username. The corresponding test cases are added as well.

Emailed Sean in response to the needinfo yesterday night. Copy/pasting it here. (In reply to Sean Lee [:seanlee][:weilonge] from comment #38) > (In reply to Tanvi Vyas [:tanvi] from comment #37) > > > > Hi Sean, > > > > In general, this looks good. Please go ahead with this and file a followup > > bug to use the Context Menu information here. > Great! Except the follow-up bug we mentioned here, "Learn More" link should > be fixed as well. The relative information is at the first item of bug > 1217162 comment 32. > Learn More link can be same bug or different bug. Probably better in a different bug so it can land quickly. > > > > What will happen if there is no username associated with a passwword? Will > > you see a blank entry in the autocomplete drop down? If so, I still think > > that is okay as long as we file, fix, and uplift the Context Menu bug. > In the current patch, the password item without username is not in the > autocomplete drop down. > > What should the blank entry be shown in "this patch"? "No username" with the > date like the context menu? > (https://bug1289913.bmoattachments.org/attachment.cgi?id=8800160) "No username" plus date like in the context menu is good. You should use some sort of internal date method (if there is one) so that it formats properly for the different locals. See what the context menu does. > > Eventually, we will have the design mentioned in comment 22. > > > > Thanks!

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review92216 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:13 (Diff revisions 3 - 7) > const PREF_INSECURE_FIELD_WARNING_ENABLED = "security.insecure_field_warning.contextual.enabled"; > +const PREF_INSECURE_AUTOFILLFORMS_ENABLED = "signon.autofillForms.http"; If you move these two prefs inside LoginHelper[1][2] then the prefs would be live and you can avoid the innerHTML change. [1] https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/toolkit/components/passwordmgr/LoginHelper.jsm#35 [2] https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/toolkit/components/passwordmgr/LoginHelper.jsm#54 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1256 (Diff revisions 3 - 7) > + if (!prefShowInsecureAutoFillForms) { > + matchingLogins = []; > + } Don't you only want to do this if `!prefShowInsecureAutoFillForms && !isSecure`? With what you have I think you're preventing autocomplete suggestions for both secure and insecure. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1331 (Diff revisions 3 - 7) > + // If login is empty or duplicated we want to append a modification date to it. > + if (!username || this._duplicateUsernames.has(username)) { > + if (!username) { > + username = getLocalizedString("noUsername"); Ideally we wouldn't be duplicating this logic from the context menu code… I can let it go only because of the merge deadline. This logic is expected to get more complicated with upcoming password manager changes so forking this isn't a good idea.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review92216 > If you move these two prefs inside LoginHelper[1][2] then the prefs would be live and you can avoid the innerHTML change. > > > [1] https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/toolkit/components/passwordmgr/LoginHelper.jsm#35 > [2] https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/toolkit/components/passwordmgr/LoginHelper.jsm#54 After using "LoginHelper.insecureAutofill" and "LoginHelper.showInsecureFieldWarning" in UserAutoCompleteResult, it still needs "innerHTML" to complete the test. So I leave the implementation here. > Don't you only want to do this if `!prefShowInsecureAutoFillForms && !isSecure`? > > With what you have I think you're preventing autocomplete suggestions for both secure and insecure. Use !LoginHelper.insecureAutofill && !isSecure now and add the corresponding test for it. > Ideally we wouldn't be duplicating this logic from the context menu code… I can let it go only because of the merge deadline. This logic is expected to get more complicated with upcoming password manager changes so forking this isn't a good idea. I will fire a follow-up bug to fix it.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review92404 Thanks ::: toolkit/components/passwordmgr/LoginHelper.jsm:65 (Diff revision 8) > logger.maxLogLevel = getMaxLogLevel(); > }, false); > > + Services.prefs.addObserver("security.insecure_field_warning.", () => { > + this.showInsecureFieldWarning = Services.prefs.getBoolPref("security.insecure_field_warning.contextual.enabled"); > + logger.maxLogLevel = getMaxLogLevel(); This line isn't needed as it's only relevant when signon.debug changes IIUC

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/53d701cacdea Show autocomplete UI on password fields.; r=MattN

After rebasing the patch to the latest m-c, the test is more stable now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead03f821af258da0fb0a69ff9bd7385015c5598

Backed out for failing xpcshell test_user_autocomplete_result.js on Android and mochitest test_password_field_autocomplete: https://hg.mozilla.org/integration/autoland/rev/162e1d1d9565e6d825c02b3e738bfaa1bf9766d6 Mochitest failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6537717&repo=autoland 22:16:50 INFO - SpawnTask.js | Entering test test_form1_initial_empty 22:16:50 INFO - must wait for load 22:16:50 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html | Checking form1 password is: 22:16:50 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html | Check popup is initially closed 22:16:50 INFO - SpawnTask.js | Leaving test test_form1_initial_empty 22:16:50 INFO - SpawnTask.js | Entering test test_form1_enabledInsecureFieldWarning_enabledInsecureAutoFillForm 22:16:50 INFO - Buffered messages finished 22:16:50 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html | Test timed out. 22:16:50 INFO - reportError@SimpleTest/TestRunner.js:114:7 22:16:50 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7 22:16:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:156:5 Xpcshell failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6539073&repo=autoland [task 2016-11-12T12:34:45.852495Z] 12:34:45 INFO - TEST-START | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js [task 2016-11-12T12:35:01.011705Z] 12:35:01 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | xpcshell return code: 0 [task 2016-11-12T12:35:01.012326Z] 12:35:01 INFO - TEST-INFO took 15159ms [task 2016-11-12T12:35:01.012484Z] 12:35:01 INFO - >>>>>>> [task 2016-11-12T12:35:01.017770Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/passwordmgr/test/unit [task 2016-11-12T12:35:01.018088Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/passwordmgr/test/unit/head.js"]; -e const _TAIL_FILES = []; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_user_autocomplete_result.js"]; -e const _TEST_NAME = "toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js" -e _execute_test(); quit(0); [task 2016-11-12T12:35:01.018227Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | [6160] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 172 [task 2016-11-12T12:35:01.018322Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | Warning: MOZILLA_FIVE_HOME not set. [task 2016-11-12T12:35:01.018465Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | [6160] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /home/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2816 [task 2016-11-12T12:35:01.018561Z] 12:35:01 INFO - ReferenceError: Intl is not defined at test_user_autocomplete_result.js:27 [task 2016-11-12T12:35:01.018627Z] 12:35:01 INFO - @test_user_autocomplete_result.js:27:5 [task 2016-11-12T12:35:01.018694Z] 12:35:01 INFO - load_file@/storage/sdcard/tests/xpc/head.js:660:7 [task 2016-11-12T12:35:01.018760Z] 12:35:01 INFO - _load_files@/storage/sdcard/tests/xpc/head.js:672:3 [task 2016-11-12T12:35:01.018816Z] 12:35:01 INFO - _execute_test@/storage/sdcard/tests/xpc/head.js:515:3 [task 2016-11-12T12:35:01.018866Z] 12:35:01 INFO - @-e:1:1 [task 2016-11-12T12:35:01.018930Z] 12:35:01 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2016-11-12T12:35:01.018996Z] 12:35:01 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2) [task 2016-11-12T12:35:01.019060Z] 12:35:01 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) [task 2016-11-12T12:35:01.019106Z] 12:35:01 INFO - running event loop [task 2016-11-12T12:35:01.019185Z] 12:35:01 INFO - toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | Starting test_common_initialize [task 2016-11-12T12:35:01.019256Z] 12:35:01 INFO - (xpcshell/head.js) | test test_common_initialize pending (2) [task 2016-11-12T12:35:01.019326Z] 12:35:01 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2) [task 2016-11-12T12:35:01.019449Z] 12:35:01 INFO - PROCESS | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | [6160] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 2622 [task 2016-11-12T12:35:01.019519Z] 12:35:01 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2) [task 2016-11-12T12:35:01.019589Z] 12:35:01 INFO - (xpcshell/head.js) | test test_common_initialize finished (2) [task 2016-11-12T12:35:01.019656Z] 12:35:01 INFO - (xpcshell/head.js) | test run_next_test 1 finished (1) [task 2016-11-12T12:35:01.019708Z] 12:35:01 INFO - exiting test Intl doesn't ship yet on Android, see bug 1215247.

the patch is updated for fixing the failed xpcshell test_user_autocomplete_result.js on Android. I am investigating test_password_field_autocomplete.html failed.

Hi Aryx, please see comment 59 and test_user_autocomplete_result.js on Android should be fixed now. For test_password_field_autocomplete.html, I suppose this only happens on Linux pgo platform. (Please see M (5) of [1].) Do you see any M (5) failed in other try? Thank you. [1] https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=53d701cacdeacc6b49f958844ea69d8670e3c1fa&selectedJob=6537717 (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #57) > Backed out for failing xpcshell test_user_autocomplete_result.js on Android > and mochitest test_password_field_autocomplete: > > https://hg.mozilla.org/integration/autoland/rev/ > 162e1d1d9565e6d825c02b3e738bfaa1bf9766d6 > > Mochitest failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=6537717&repo=autoland > > 22:16:50 INFO - SpawnTask.js | Entering test test_form1_initial_empty > 22:16:50 INFO - must wait for load > 22:16:50 INFO - TEST-PASS | > toolkit/components/passwordmgr/test/mochitest/ > test_password_field_autocomplete.html | Checking form1 password is: > 22:16:50 INFO - TEST-PASS | > toolkit/components/passwordmgr/test/mochitest/ > test_password_field_autocomplete.html | Check popup is initially closed > 22:16:50 INFO - SpawnTask.js | Leaving test test_form1_initial_empty > 22:16:50 INFO - SpawnTask.js | Entering test > test_form1_enabledInsecureFieldWarning_enabledInsecureAutoFillForm > 22:16:50 INFO - Buffered messages finished > 22:16:50 INFO - TEST-UNEXPECTED-FAIL | > toolkit/components/passwordmgr/test/mochitest/ > test_password_field_autocomplete.html | Test timed out. > 22:16:50 INFO - reportError@SimpleTest/TestRunner.js:114:7 > 22:16:50 INFO - > TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7 > 22:16:50 INFO - setTimeout > handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:156:5 >

This also happens on Linux debug: https://treeherder.mozilla.org/logviewer.html#?job_id=6536172&repo=autoland Windows 7 VM debug: https://treeherder.mozilla.org/logviewer.html#?job_id=6537169&repo=autoland

The new patch is updated with an observer of prefs. Here are two try server link for inspecting the result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e5ec2af6b61a8283ab1fbfafbbf735411a39e4 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7405185f85c3f524cd423e4f7674f173f69889e8

The test failed is not reproduced in my local build of MacOSX, Linux Debug, and Windows 7 Debug. The test hangs at promiseACShown [1], so I suspect it's relative to that UserAutoCompleteResult do not get the expected preferences to show the autocomplete drop down and the correct menu item. The solution I tried is to add a pref observer in UserAutoComplete to get the latest one and update the corresponding variable. It doesn't work in try server but works in my local build. When the innerHTML is removed, the test gets another error of getCommentAt function in browser-content.js. I wonder if we should keep matching logins be always the same after new UserAutoCompleteResult(). Another idea in my mind is to separate these tests into couple files for each preference case. [1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/toolkit/components/satchel/test/satchel_common.js#246

Push a try to see if the prefs are configured correctly. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1f2eaafac0d289ec53ef6f2d8d35acc365868e

I found the prefs are configured correctly before promiseACShown, but promiseACShown is still not resolved after doKey("down");. I am going to see if there is any solution to make promiseACShown more stable.

In the latest version, some pref-related test are added prefs checking like this: is(SpecialPowers.getBoolPref("security.insecure_field_warning.contextual.enabled"), true, "verify pref security.insecure_field_warning.contextual.enabled"); is(SpecialPowers.getBoolPref("signon.autofillForms.http"), true, "verify pref signon.autofillForms.http"); I know it looks like testing getBoolPref and pushPrefEnv of SpecialPowers rather than AutoComplete, but it does work for test fixing. I think it’s worth to verify because that would be useful to investigate the failed test. Please see Linux x64 debug tc-M(10) of [1]. That's where the failed test of test_password_field_autocomplete.html happens, and 12 test tasks are all pass. Another change is adding a pref observer in UserAutoCompleteResult. However, innerHTML is still needed, and that’s the best solution I found so far. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=268371b416e9b465fc455efe5f04b202ba3e5caf

Hi Aryx, could you help to verify the latest patch attachment 8807096 [details]? I think test_password_field_autocomplete.html is fixed as well. Thanks.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; Hi MattN, could you see comment 69 and review the patch again? Here are the changes since last r+: https://reviewboard.mozilla.org/r/90374/diff/10-13/ There are many changes in modules/libpref/init/all.js due to the patch rebasing. Thanks.

This is the try-link corresponding the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b71ea101303cf248b5597c17a5e00eb4ae37025 The one in comment 69 contains some debugging logs.

Comment on attachment 8807096 [details] Bug 1289913 - Show autocomplete UI on password fields.; https://reviewboard.mozilla.org/r/90374/#review92624 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1327 (Diff revision 13) > - return this.logins[index - this._showInsecureFieldWarning].username; > + let that = this; > + > + function getLocalizedString(key, formatArgs) { In the future use an arrow function to avoid the bind/that: let getLocalizedString = (key, formatArgs) => { … };

Hi MattN, Thanks a lot for the quick review. I will fix the arrow function part in a follow-up bug. Could you help to land the patch?

Queued for landing

Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/b6d9732b3240 Show autocomplete UI on password fields.; r=MattN

verified as fixed on 52.0a2 20170122004006 on Windows 10, Ubuntu 16.04 and Mac OSX 10.10