Skip to content

Commit

Permalink
Change password bubble for Mac.
Browse files Browse the repository at this point in the history 
This CL contains all UI changes related to change password support on Mac. On Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/ and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. It's the same as password save bubble with "Save"->"Update"
2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a dropbox with a list of all credentials for current site.

Also in case when there are no stored credentials and no username form found username-password row is not shown in the bubble.

Screenshots of UI: https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9BA/edit#

Update bubble is currently behind a flag until UI review.

BUG=359315

Review URL: https://codereview.chromium.org/1515553006

Cr-Commit-Position: refs/heads/master@{#365796}
  • Loading branch information
dvadym authored and Commit bot committed Dec 17, 2015
1 parent 46e701f commit 5cc29b8
Show file tree
Hide file tree
Showing 19 changed files with 514 additions and 60 deletions.
7 changes: 6 additions & 1 deletion chrome/browser/password_manager/chrome_password_manager_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,12 @@ bool ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled() {

bool ChromePasswordManagerClient::IsUpdatePasswordUIEnabled() const {
#if defined(OS_MACOSX)
return false;
if (!ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled()) {
// Currently Password update UI is implemented only for Bubble UI.
return false;
}
return base::FeatureList::IsEnabled(
password_manager::features::kEnablePasswordChangeSupport);
#else
return IsTheHotNewBubbleUIEnabled();
#endif
Expand Down
105 changes: 62 additions & 43 deletions chrome/browser/password_manager/password_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class ObservingAutofillClient : public autofill::TestAutofillClient {
DISALLOW_COPY_AND_ASSIGN(ObservingAutofillClient);
};

// For simplicity we assume that password store contains only 1 credentials.
void CheckThatCredentialsStored(
password_manager::TestPasswordStore* password_store,
const base::string16& username,
Expand Down Expand Up @@ -405,17 +406,9 @@ IN_PROC_BROWSER_TEST_F(
EXPECT_FALSE(prompt_observer->IsShowingPrompt());
}

// Disabled on Mac due to flakiness: crbug.com/493263
#if defined(OS_MACOSX)
#define MAYBE_NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage \
DISABLED_NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage
#else
#define MAYBE_NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage \
NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage
#endif
IN_PROC_BROWSER_TEST_F(
PasswordManagerBrowserTestBase,
MAYBE_NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage) {
NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage) {
NavigateToFile("/password/multi_frames.html");

// Make sure that we don't prompt to save the password for a failed login
Expand Down Expand Up @@ -974,10 +967,8 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
run_loop.RunUntilIdle();
EXPECT_FALSE(password_store->IsEmpty());

#if !defined(OS_MACOSX)
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("temp"),
base::ASCIIToUTF16("random"));
#endif
}

// Test for checking that no prompt is shown for URLs with file: scheme.
Expand Down Expand Up @@ -1645,6 +1636,16 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,

IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
ChangePwdFormBubbleShown) {
// TODO(dvadym): Remove appending kEnablePasswordChangeSupport switch as soon as
// it is removed on Mac. http://crbug.com/359315
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
NavigateToFile("/password/password_form.html");

NavigationObserver observer(WebContents());
Expand All @@ -1658,17 +1659,21 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
"document.getElementById('chg_submit_button').click()";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
observer.Wait();
// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if defined(OS_MACOSX)
EXPECT_FALSE(prompt_observer->IsShowingPrompt());
#else
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
#endif
}

IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
ChangePwdFormPushStateBubbleShown) {
// TODO(dvadym): Remove appending kEnablePasswordChangeSupport switch as soon as
// it is removed on Mac. http://crbug.com/359315
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
NavigateToFile("/password/password_push_state.html");

NavigationObserver observer(WebContents());
Expand All @@ -1683,13 +1688,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
"document.getElementById('chg_submit_button').click()";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
observer.Wait();
// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if defined(OS_MACOSX)
EXPECT_FALSE(prompt_observer->IsShowingPrompt());
#else
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
#endif
}

IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, NoPromptOnBack) {
Expand Down Expand Up @@ -2037,11 +2036,16 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
iframe_killed.Wait();
}

// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if !defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
ChangePwdNoAccountStored) {
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
NavigateToFile("/password/password_form.html");

Expand Down Expand Up @@ -2077,13 +2081,17 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16(""),
base::ASCIIToUTF16("new_pw"));
}
#endif

// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if !defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
ChangePwd1AccountStored) {
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
// At first let us save credentials to the PasswordManager.
scoped_refptr<password_manager::TestPasswordStore> password_store =
Expand Down Expand Up @@ -2128,13 +2136,17 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("temp"),
base::ASCIIToUTF16("new_pw"));
}
#endif

// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if !defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
PasswordOverridenUpdateBubbleShown) {
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
// At first let us save credentials to the PasswordManager.
scoped_refptr<password_manager::TestPasswordStore> password_store =
Expand Down Expand Up @@ -2173,13 +2185,17 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("temp"),
base::ASCIIToUTF16("new_pw"));
}
#endif

// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if !defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
PasswordNotOverridenUpdateBubbleNotShown) {
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
// At first let us save credentials to the PasswordManager.
scoped_refptr<password_manager::TestPasswordStore> password_store =
Expand Down Expand Up @@ -2210,13 +2226,17 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("temp"),
base::ASCIIToUTF16("pw"));
}
#endif

// TODO(dvadym): Turn on this test when Change password UI will be implemented
// for Mac. http://crbug.com/359315
#if !defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
ChangePwdWhenTheFormContainNotUsernameTextfield) {
#if defined(OS_MACOSX)
// Add the enable-password-change-support feature.
base::FeatureList::ClearInstanceForTesting();
scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(
password_manager::features::kEnablePasswordChangeSupport.name, "");
base::FeatureList::SetInstance(std::move(feature_list));
#endif
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
// At first let us save credentials to the PasswordManager.
scoped_refptr<password_manager::TestPasswordStore> password_store =
Expand Down Expand Up @@ -2259,7 +2279,6 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("temp"),
base::ASCIIToUTF16("new_pw"));
}
#endif

// Test whether the password form with the username and password fields having
// ambiguity in id attribute gets autofilled correctly.
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class ManagePasswordsControllerTest : public CocoaProfileTest {
ManagePasswordsBubbleModel* GetModelAndCreateIfNull();

// Sets the appropriate state for ManagePasswordsBubbleModel.
void SetUpPendingState();
void SetUpSavePendingState(bool empty_username);
void SetUpUpdatePendingState(bool multiple_forms);
void SetUpConfirmationState();
void SetUpManageState();
void SetUpAccountChooser(
Expand Down
24 changes: 23 additions & 1 deletion chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h"

#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h"
#include "components/password_manager/core/browser/mock_password_store.h"
Expand Down Expand Up @@ -48,8 +49,11 @@
return model_.get();
}

void ManagePasswordsControllerTest::SetUpPendingState() {
void ManagePasswordsControllerTest::SetUpSavePendingState(bool empty_username) {
autofill::PasswordForm form;
if (!empty_username) {
form.username_value = base::ASCIIToUTF16("username");
}
EXPECT_CALL(*ui_controller_, GetPendingPassword()).WillOnce(ReturnRef(form));
std::vector<const autofill::PasswordForm*> forms;
EXPECT_CALL(*ui_controller_, GetCurrentForms()).WillOnce(ReturnRef(forms));
Expand All @@ -61,6 +65,24 @@
ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(ui_controller_));
}

void ManagePasswordsControllerTest::SetUpUpdatePendingState(
bool multiple_forms) {
autofill::PasswordForm form;
EXPECT_CALL(*ui_controller_, GetPendingPassword()).WillOnce(ReturnRef(form));
std::vector<const autofill::PasswordForm*> forms;
forms.push_back(&form);
if (multiple_forms) {
forms.push_back(&form);
}
EXPECT_CALL(*ui_controller_, GetCurrentForms()).WillOnce(ReturnRef(forms));
GURL origin(kSiteOrigin);
EXPECT_CALL(*ui_controller_, GetOrigin()).WillOnce(ReturnRef(origin));
EXPECT_CALL(*ui_controller_, GetState())
.WillOnce(Return(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE));
GetModelAndCreateIfNull();
ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(ui_controller_));
}

void ManagePasswordsControllerTest::SetUpConfirmationState() {
GURL origin(kSiteOrigin);
EXPECT_CALL(*ui_controller_, GetOrigin()).WillOnce(ReturnRef(origin));
Expand Down
27 changes: 27 additions & 0 deletions chrome/browser/ui/cocoa/passwords/credentials_selection_view.h
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_UI_COCOA_PASSWORDS_CREDENTIALS_SELECTION_VIEW_H_
#define CHROME_BROWSER_UI_COCOA_PASSWORDS_CREDENTIALS_SELECTION_VIEW_H_

#import <Cocoa/Cocoa.h>

#import "base/mac/scoped_nsobject.h"

class ManagePasswordsBubbleModel;
namespace autofill {
struct PasswordForm;
} // namespace autofill

// Shows a combobox for choosing username and obscured password in a single row.
@interface CredentialsSelectionView : NSView {
@private
ManagePasswordsBubbleModel* model_; // weak
base::scoped_nsobject<NSPopUpButton> usernamePopUpButton_;
base::scoped_nsobject<NSSecureTextField> passwordField_;
}
- (id)initWithModel:(ManagePasswordsBubbleModel*)model;
- (const autofill::PasswordForm*)getSelectedCredentials;
@end

#endif // CHROME_BROWSER_UI_COCOA_PASSWORDS_CREDENTIALS_SELECTION_VIEW_H_
Loading

0 comments on commit 5cc29b8

Please sign in to comment.