From df96855961928dc8b0edf16e49cc053c5c2016fc Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Thu, 4 Jul 2019 23:32:01 +0000 Subject: [PATCH] Bug 1558242 - On initial page load select the first login if available. r=sfoster Differential Revision: https://phabricator.services.mozilla.com/D36640 --HG-- extra : moz-landing-system : lando --- .../aboutlogins/content/aboutLogins.js | 5 +- .../content/components/login-item.js | 31 +++++++--- .../content/components/login-list-item.js | 4 ++ .../content/components/login-list.js | 59 +++++++++++++++---- .../browser_aaa_eventTelemetry_run_first.js | 6 +- .../tests/browser/browser_openFiltered.js | 17 ++++-- .../tests/chrome/test_login_item.html | 20 +++---- .../tests/chrome/test_login_list.html | 38 +++++------- 8 files changed, 118 insertions(+), 62 deletions(-) diff --git a/browser/components/aboutlogins/content/aboutLogins.js b/browser/components/aboutlogins/content/aboutLogins.js index b482c5806fa2..640a51fff453 100644 --- a/browser/components/aboutlogins/content/aboutLogins.js +++ b/browser/components/aboutlogins/content/aboutLogins.js @@ -18,10 +18,7 @@ document.addEventListener("DOMContentLoaded", () => { } gElements.newLoginButton.addEventListener("click", () => { - window.dispatchEvent(new CustomEvent("AboutLoginsLoginSelected", { - detail: {}, - })); - + window.dispatchEvent(new CustomEvent("AboutLoginsCreateLogin")); recordTelemetryEvent({object: "new_login", method: "new"}); }); diff --git a/browser/components/aboutlogins/content/components/login-item.js b/browser/components/aboutlogins/content/components/login-item.js index ea3aefdb99bb..c916d6db0002 100644 --- a/browser/components/aboutlogins/content/components/login-item.js +++ b/browser/components/aboutlogins/content/components/login-item.js @@ -51,11 +51,15 @@ export default class LoginItem extends HTMLElement { this._originInput.addEventListener("blur", this); this._cancelButton.addEventListener("click", this); + this._copyPasswordButton.addEventListener("click", this); + this._copyUsernameButton.addEventListener("click", this); this._deleteButton.addEventListener("click", this); this._editButton.addEventListener("click", this); this._openSiteButton.addEventListener("click", this); this._originInput.addEventListener("click", this); this._saveChangesButton.addEventListener("click", this); + window.addEventListener("AboutLoginsCreateLogin", this); + window.addEventListener("AboutLoginsInitialLoginSelected", this); window.addEventListener("AboutLoginsLoginSelected", this); } @@ -73,6 +77,14 @@ export default class LoginItem extends HTMLElement { handleEvent(event) { switch (event.type) { + case "AboutLoginsCreateLogin": { + this.setLogin({}); + break; + } + case "AboutLoginsInitialLoginSelected": { + this.setLogin(event.detail, {skipFocusChange: true}); + break; + } case "AboutLoginsLoginSelected": { this.setLogin(event.detail); break; @@ -101,17 +113,15 @@ export default class LoginItem extends HTMLElement { // Prevent form submit behavior on the following buttons. event.preventDefault(); if (classList.contains("cancel-button")) { - if (this._login.guid) { + let wasExistingLogin = !!this._login.guid; + if (wasExistingLogin) { this.setLogin(this._login); } else { - // TODO, should select the first login if it exists - // or show the no-logins view otherwise - this._toggleEditing(); - this.render(); + window.dispatchEvent(new CustomEvent("AboutLoginsClearSelection")); } recordTelemetryEvent({ - object: this._login.guid ? "existing_login" : "new_login", + object: wasExistingLogin ? "existing_login" : "new_login", method: "cancel", }); return; @@ -197,8 +207,11 @@ export default class LoginItem extends HTMLElement { /** * @param {login} login The login that should be displayed. The login object is * a plain JS object representation of nsILoginInfo/nsILoginMetaInfo. + * @param {boolean} skipFocusChange Optional, if present and set to true, the Edit button of the + * login will not get focus automatically. This is used to prevent + * stealing focus from the search filter upon page load. */ - setLogin(login) { + setLogin(login, {skipFocusChange} = {}) { this._login = login; this._form.reset(); @@ -212,7 +225,9 @@ export default class LoginItem extends HTMLElement { this._revealCheckbox.checked = false; - this._editButton.focus(); + if (!skipFocusChange) { + this._editButton.focus(); + } this.render(); } diff --git a/browser/components/aboutlogins/content/components/login-list-item.js b/browser/components/aboutlogins/content/components/login-list-item.js index 9bf9ed8a3b6c..9c368614207c 100644 --- a/browser/components/aboutlogins/content/components/login-list-item.js +++ b/browser/components/aboutlogins/content/components/login-list-item.js @@ -55,6 +55,10 @@ export default class LoginListItem extends HTMLElement { handleEvent(event) { switch (event.type) { case "click": { + if (!this._login.guid) { + return; + } + this.dispatchEvent(new CustomEvent("AboutLoginsLoginSelected", { bubbles: true, composed: true, diff --git a/browser/components/aboutlogins/content/components/login-list.js b/browser/components/aboutlogins/content/components/login-list.js index 1947f044d1d0..f2b549e09a22 100644 --- a/browser/components/aboutlogins/content/components/login-list.js +++ b/browser/components/aboutlogins/content/components/login-list.js @@ -29,33 +29,43 @@ export default class LoginList extends HTMLElement { document.l10n.connectRoot(shadowRoot); shadowRoot.appendChild(loginListTemplate.content.cloneNode(true)); - this._list = this.shadowRoot.querySelector("ol"); this._count = this.shadowRoot.querySelector(".count"); + this._list = this.shadowRoot.querySelector("ol"); + this._sortSelect = this.shadowRoot.querySelector("#login-sort"); this.render(); this.shadowRoot.getElementById("login-sort") .addEventListener("change", this); + window.addEventListener("AboutLoginsClearSelection", this); + window.addEventListener("AboutLoginsCreateLogin", this); + window.addEventListener("AboutLoginsInitialLoginSelected", this); window.addEventListener("AboutLoginsLoginSelected", this); window.addEventListener("AboutLoginsFilterLogins", this); this.addEventListener("keydown", this); } - render() { + /** + * + * @param {object} options optional + * createLogin: When set to true will show and select + * a blank login-list-item. + */ + render(options = {}) { this._list.textContent = ""; - if (!this._logins.length) { - document.l10n.setAttributes(this._count, "login-list-count", {count: 0}); - return; - } - - if (!this._selectedGuid) { + if (options.createLogin) { this._blankLoginListItem.classList.add("selected"); this._blankLoginListItem.setAttribute("aria-selected", "true"); this._list.setAttribute("aria-activedescendant", this._blankLoginListItem.id); this._list.append(this._blankLoginListItem); } + if (!this._logins.length) { + document.l10n.setAttributes(this._count, "login-list-count", {count: 0}); + return; + } + for (let login of this._logins) { let listItem = new LoginListItem(login); if (login.guid == this._selectedGuid) { @@ -73,22 +83,37 @@ export default class LoginList extends HTMLElement { handleEvent(event) { switch (event.type) { case "change": { - const sort = event.target.value; + const sort = this._sortSelect.value; this._logins = this._logins.sort((a, b) => sortFnOptions[sort](a, b)); this.render(); break; } + case "AboutLoginsClearSelection": { + if (!this._logins.length) { + return; + } + window.dispatchEvent(new CustomEvent("AboutLoginsLoginSelected", { + detail: this._logins[0], + })); + break; + } + case "AboutLoginsCreateLogin": { + this._selectedGuid = null; + this.render({createLogin: true}); + break; + } case "AboutLoginsFilterLogins": { this._filter = event.detail.toLocaleLowerCase(); this.render(); break; } + case "AboutLoginsInitialLoginSelected": case "AboutLoginsLoginSelected": { if (this._selectedGuid == event.detail.guid) { return; } - this._selectedGuid = event.detail.guid || null; + this._selectedGuid = event.detail.guid; this.render(); break; } @@ -104,7 +129,21 @@ export default class LoginList extends HTMLElement { */ setLogins(logins) { this._logins = logins; + const sort = this._sortSelect.value; + this._logins = this._logins.sort((a, b) => sortFnOptions[sort](a, b)); + this.render(); + + if (!this._selectedGuid || + !this._logins.findIndex(login => login.guid == this._selectedGuid) != -1) { + // Select the first visible login after any possible filter is applied. + let firstVisibleLogin = this._list.querySelector("login-list-item[data-guid]:not([hidden])"); + if (firstVisibleLogin) { + window.dispatchEvent(new CustomEvent("AboutLoginsInitialLoginSelected", { + detail: firstVisibleLogin._login, + })); + } + } } /** diff --git a/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js index 6e42b6a31dc5..d7b584af2288 100644 --- a/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js +++ b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js @@ -18,6 +18,10 @@ add_task(async function setup() { (_, data) => data == "addLogin"); TEST_LOGIN1 = Services.logins.addLogin(TEST_LOGIN1); await storageChangedPromised; + storageChangedPromised = TestUtils.topicObserved("passwordmgr-storage-changed", + (_, data) => data == "addLogin"); + TEST_LOGIN2 = Services.logins.addLogin(TEST_LOGIN2); + await storageChangedPromised; await BrowserTestUtils.openNewForegroundTab({gBrowser, url: "about:logins"}); registerCleanupFunction(() => { BrowserTestUtils.removeTab(gBrowser.selectedTab); @@ -35,7 +39,7 @@ add_task(async function test_telemetry_events() { await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() { let loginList = content.document.querySelector("login-list"); - let loginListItem = loginList.shadowRoot.querySelector("login-list-item[data-guid]"); + let loginListItem = loginList.shadowRoot.querySelector("login-list-item:nth-child(2)"); loginListItem.click(); }); await waitForTelemetryEventCount(1); diff --git a/browser/components/aboutlogins/tests/browser/browser_openFiltered.js b/browser/components/aboutlogins/tests/browser/browser_openFiltered.js index a41c5721b645..0ccc1e8c5109 100644 --- a/browser/components/aboutlogins/tests/browser/browser_openFiltered.js +++ b/browser/components/aboutlogins/tests/browser/browser_openFiltered.js @@ -34,14 +34,21 @@ add_task(async function test_query_parameter_filter() { return loginList._logins.length == 2; }, "Waiting for logins to be cached"); - let loginFilter = Cu.waiveXrays(content.document.querySelector("login-filter")); - is(loginFilter.value, logins[0].origin, "The filter should be prepopulated"); + let loginItem = Cu.waiveXrays(content.document.querySelector("login-item")); + await ContentTaskUtils.waitForCondition(() => loginItem._login.guid == logins[0].guid, + "Waiting for TEST_LOGIN1 to be selected for the login-item view"); + + let loginFilter = content.document.querySelector("login-filter"); + let xRayLoginFilter = Cu.waiveXrays(loginFilter); + is(xRayLoginFilter.value, logins[0].origin, "The filter should be prepopulated"); + is(content.document.activeElement, loginFilter, "login-filter should be focused"); + is(loginFilter.shadowRoot.activeElement, loginFilter.shadowRoot.querySelector(".filter"), + "the actual input inside of login-filter should be focused"); let hiddenLoginListItems = loginList.shadowRoot.querySelectorAll("login-list-item[hidden]"); let visibleLoginListItems = loginList.shadowRoot.querySelectorAll("login-list-item:not([hidden])"); - is(visibleLoginListItems.length, 2, "The 'new' login and one login should be visible"); - ok(!visibleLoginListItems[0].dataset.guid, "The 'new' login should be visible"); - is(visibleLoginListItems[1].dataset.guid, logins[0].guid, "TEST_LOGIN1 should be visible"); + is(visibleLoginListItems.length, 1, "The one login should be visible"); + is(visibleLoginListItems[0].dataset.guid, logins[0].guid, "TEST_LOGIN1 should be visible"); is(hiddenLoginListItems.length, 1, "One login should be hidden"); is(hiddenLoginListItems[0].dataset.guid, logins[1].guid, "TEST_LOGIN2 should be hidden"); }); diff --git a/browser/components/aboutlogins/tests/chrome/test_login_item.html b/browser/components/aboutlogins/tests/chrome/test_login_item.html index 685adfd27b19..f6acb4515be1 100644 --- a/browser/components/aboutlogins/tests/chrome/test_login_item.html +++ b/browser/components/aboutlogins/tests/chrome/test_login_item.html @@ -112,20 +112,16 @@ add_task(async function test_edit_login() { }); add_task(async function test_edit_login_cancel() { - for (let login of [{}, TEST_LOGIN_1]) { - let isNewLogin = !login.username; - info("Testing with" + (isNewLogin ? "out" : "") + " a login"); - gLoginItem.setLogin(login); - gLoginItem.shadowRoot.querySelector(".edit-button").click(); + gLoginItem.setLogin(TEST_LOGIN_1); + gLoginItem.shadowRoot.querySelector(".edit-button").click(); - ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode"); - is(!!gLoginItem.dataset.isNewLogin, isNewLogin, - "loginItem should " + (isNewLogin ? "" : "not ") + "be in 'isNewLogin' mode"); + ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode"); + is(!!gLoginItem.dataset.isNewLogin, false, + "loginItem should not be in 'isNewLogin' mode"); - gLoginItem.shadowRoot.querySelector(".cancel-button").click(); - ok(!gLoginItem.dataset.editing, "loginItem should not be in 'edit' mode"); - ok(!gLoginItem.dataset.isNewLogin, "loginItem should not be in 'isNewLogin' mode"); - } + gLoginItem.shadowRoot.querySelector(".cancel-button").click(); + ok(!gLoginItem.dataset.editing, "loginItem should not be in 'edit' mode"); + ok(!gLoginItem.dataset.isNewLogin, "loginItem should not be in 'isNewLogin' mode"); }); add_task(async function test_reveal_password_change_selected_login() { diff --git a/browser/components/aboutlogins/tests/chrome/test_login_list.html b/browser/components/aboutlogins/tests/chrome/test_login_list.html index 29c980fd020a..5446fc1bdd6a 100644 --- a/browser/components/aboutlogins/tests/chrome/test_login_list.html +++ b/browser/components/aboutlogins/tests/chrome/test_login_list.html @@ -126,29 +126,26 @@ add_task(async function test_empty_login_username_in_list() { gLoginList.setLogins([TEST_LOGIN_3]); let loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); - is(loginListItems.length, 2, "A blank login and the one stored login should be displayed"); - ok(!loginListItems[0].dataset.guid, "first login-list-item should be the 'new' item"); - is(loginListItems[1].dataset.guid, TEST_LOGIN_3.guid, "login-list-item should have correct guid attribute"); + is(loginListItems.length, 1, "The one stored login should be displayed"); + is(loginListItems[0].dataset.guid, TEST_LOGIN_3.guid, "login-list-item should have correct guid attribute"); - loginListItems[1].render(); - let loginUsername = loginListItems[1].shadowRoot.querySelector(".username"); + loginListItems[0].render(); + let loginUsername = loginListItems[0].shadowRoot.querySelector(".username"); is(loginUsername.getAttribute("data-l10n-id"), "login-list-item-subtitle-missing-username", "login should show missing username text"); }); add_task(async function test_populated_list() { gLoginList.setLogins([TEST_LOGIN_1, TEST_LOGIN_2]); let loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); - is(loginListItems.length, 3, "A blank login and the two stored logins should be displayed"); - ok(!loginListItems[0].dataset.guid, "first login-list-item should be the 'new' item"); - is(loginListItems[1].dataset.guid, TEST_LOGIN_1.guid, "login-list-item should have correct guid attribute"); - is(loginListItems[1].shadowRoot.querySelector(".title").textContent, TEST_LOGIN_1.title, + is(loginListItems.length, 2, "The two stored logins should be displayed"); + is(loginListItems[0].dataset.guid, TEST_LOGIN_1.guid, "login-list-item should have correct guid attribute"); + is(loginListItems[0].shadowRoot.querySelector(".title").textContent, TEST_LOGIN_1.title, "login-list-item origin should match"); - is(loginListItems[1].shadowRoot.querySelector(".username").textContent, TEST_LOGIN_1.username, + is(loginListItems[0].shadowRoot.querySelector(".username").textContent, TEST_LOGIN_1.username, "login-list-item username should match"); ok(loginListItems[0].classList.contains("selected"), "The first item should be selected by default"); ok(!loginListItems[1].classList.contains("selected"), "The second item should not be selected by default"); - ok(!loginListItems[2].classList.contains("selected"), "The third item should not be selected by default"); - loginListItems[1].click(); + loginListItems[0].click(); loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); is(loginListItems.length, 2, "After selecting one, only the two stored logins should be displayed"); ok(loginListItems[0].classList.contains("selected"), "The first item should be selected"); @@ -274,26 +271,23 @@ add_task(async function test_sorted_list() { // sort by last used gLoginList.shadowRoot.getElementById("login-sort").selectedIndex = 1; let loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); - is(loginListItems.length, 4, "The list should contain the 'new' login and the three stored logins"); - ok(!loginListItems[0]._login.guid, "The 'new' login should always be first (last used)"); - let timeUsed = loginListItems[1]._login.timeLastUsed; - let timeUsed2 = loginListItems[2]._login.timeLastUsed; + is(loginListItems.length, 3, "The list should contain the three stored logins"); + let timeUsed = loginListItems[0]._login.timeLastUsed; + let timeUsed2 = loginListItems[1]._login.timeLastUsed; is(timeUsed2 > timeUsed, true, "Last used login should be displayed at top of list"); // sort by name gLoginList.shadowRoot.getElementById("login-sort").selectedIndex = 0; loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); - ok(!loginListItems[0]._login.guid, "The 'new' login should always be first (name)"); - let title = loginListItems[1]._login.title; - let title2 = loginListItems[2]._login.title; + let title = loginListItems[0]._login.title; + let title2 = loginListItems[1]._login.title; is(title.localeCompare(title2), -1, "Logins should be sorted alphabetically by hostname"); // sort by last changed gLoginList.shadowRoot.getElementById("login-sort").selectedIndex = 2; loginListItems = gLoginList.shadowRoot.querySelectorAll("login-list-item"); - ok(!loginListItems[0]._login.guid, "The 'new' login should always be first (last changed)"); - let pwChanged = loginListItems[1]._login.timePasswordChanged; - let pwChanged2 = loginListItems[2]._login.timePasswordChanged; + let pwChanged = loginListItems[0]._login.timePasswordChanged; + let pwChanged2 = loginListItems[1]._login.timePasswordChanged; is(pwChanged2 > pwChanged, true, "Login with most recently changed password should be displayed at top of list"); });