Skip to content

Commit

Permalink
[TTF] Move password generation call under AutofillAgent::ShowSuggestions
Browse files Browse the repository at this point in the history 
Currently the password generation is triggered in
PasswordGenerationAgent::HandleFocusChangeComplete(), which is triggered
in AutofillAgent::HandleFocusChangeComplete().
The password suggestions are triggered in
PasswordAutofillAgent::ShowSuggestions, which is triggered in
AutofillAgent::ShowSuggestions.

Moving the call PasswordGenerationAgent::HandleFocusChangeComplete()
also under AutofillAgent::ShowSuggestions would make the flow of the
code more straightforward, because all the possible autofilling option
calls would be placed in one method in the order from most specific to
the least specific:
 - Try to call password generation. Return early if it worked.
 - Try to call password suggestions. Return early if it worked.
 - Call other autofill suggestions.

Password generation will not be triggered by JS any more (as now it's
triggered from the method, which is called only when there is a user
gesture).

Bug: 1469829
Change-Id: Ifed8b35a8eee10d2baf3dfd1f2c6288f690116cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4761404
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Anna Tsvirchkova <atsvirchkova@google.com>
Cr-Commit-Position: refs/heads/main@{#1185109}
  • Loading branch information
Anna Tsvirchkova authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent aaf082e commit 596072c
Show file tree
Hide file tree
Showing 10 changed files with 182 additions and 86 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/password_manager/chrome_password_manager_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,10 @@ void ChromePasswordManagerClient::AutomaticGenerationAvailable(

generation_controller->OnAutomaticGenerationAvailable(
base::AsWeakPtr(driver), ui_data, element_bounds_in_screen_space);
// Trigger password suggestions. This is a fallback case if the field was
// wrongly classified as new password field.
driver->GetPasswordAutofillManager()->MaybeShowPasswordSuggestions(
element_bounds_in_screen_space, ui_data.text_direction);
#else
password_manager::ContentPasswordManagerDriver* driver =
GetDriverFactory()->GetDriverForFrame(rfh);
Expand Down
83 changes: 65 additions & 18 deletions chrome/browser/password_manager/password_generation_interactive_uitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/events/base_event_utils.h"
#include "ui/events/keycodes/keyboard_codes.h"
#include "ui/gfx/geometry/point_f.h"

namespace {

Expand Down Expand Up @@ -101,14 +102,52 @@ class PasswordGenerationInteractiveTest
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
}

void WaitForFieldFocused(const std::string& field_id) {
const std::string script = base::StringPrintf(
"element = document.getElementById('%s');"
"new Promise(resolve => {"
" if (!element) {"
" resolve(%d);"
" }"
" if (document.activeElement && element.id == "
"document.activeElement.id) {"
" resolve(%d);"
" } else {"
" element.onfocus = function() {"
" resolve(%d);"
" element.onfocus = undefined;"
" }"
" }"
"});",
field_id.c_str(), RETURN_CODE_NO_ELEMENT, RETURN_CODE_OK,
RETURN_CODE_OK);
EXPECT_EQ(RETURN_CODE_OK,
content::EvalJs(RenderFrameHost(), script,
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
}

void WaitForPasswordFieldFocused() { WaitForFieldFocused("password_field"); }

std::string GetFocusedElement() {
return content::EvalJs(WebContents(), "document.activeElement.id")
.ExtractString();
}

void SimulateMouseClickOrTapAt(content::RenderWidgetHost* rwh,
gfx::PointF point) {
blink::WebMouseEvent mouse_event(
blink::WebInputEvent::Type::kMouseDown,
blink::WebInputEvent::kNoModifiers,
blink::WebInputEvent::GetStaticTimeStampForTests());
mouse_event.button = blink::WebPointerProperties::Button::kLeft;
mouse_event.SetPositionInWidget(point.x(), point.y());
rwh->ForwardMouseEvent(mouse_event);
}

void FocusPasswordField() {
ASSERT_TRUE(content::ExecJs(
WebContents(), "document.getElementById('password_field').focus()"));
content::SimulateMouseClickOrTapElementWithId(WebContents(),
"password_field");
WaitForPasswordFieldFocused();
}

void FocusUsernameField() {
Expand Down Expand Up @@ -152,6 +191,14 @@ class PasswordGenerationInteractiveTest
EXPECT_TRUE(GenerationPopupShowing());
}

void WaitForEditingPopupShowing() {
if (EditingPopupShowing()) {
return;
}
observer_.WaitForStatusChange();
EXPECT_TRUE(EditingPopupShowing());
}

private:
TestGenerationPopupObserver observer_;
};
Expand All @@ -173,7 +220,7 @@ class PasswordGenerationAutofillPopupInteractiveTest
IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
PopupShownAndPasswordSelected) {
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();
base::HistogramTester histogram_tester;
SendKeyToPopup(ui::VKEY_DOWN);
SendKeyToPopup(ui::VKEY_RETURN);
Expand All @@ -187,7 +234,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,

// Re-focusing the password field should show the editing popup.
FocusPasswordField();
EXPECT_TRUE(EditingPopupShowing());
WaitForEditingPopupShowing();

// The metrics are recorded when the form manager is destroyed. Closing the
// tab enforces it.
Expand All @@ -202,7 +249,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
PopupShownAutomaticallyAndPasswordErased) {
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();
SendKeyToPopup(ui::VKEY_DOWN);
SendKeyToPopup(ui::VKEY_RETURN);

Expand All @@ -211,7 +258,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,

// Re-focusing the password field should show the editing popup.
FocusPasswordField();
EXPECT_TRUE(EditingPopupShowing());
WaitForEditingPopupShowing();

// Delete the password. The generation prompt should be visible.
base::HistogramTester histogram_tester;
Expand Down Expand Up @@ -246,7 +293,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,

// Re-focusing the password field should show the editing popup.
FocusPasswordField();
EXPECT_TRUE(EditingPopupShowing());
WaitForEditingPopupShowing();

// Delete the password. The generation prompt should not be visible.
SimulateUserDeletingFieldContent("password_field");
Expand Down Expand Up @@ -280,6 +327,7 @@ IN_PROC_BROWSER_TEST_F(
ChromePasswordManagerClient::FromWebContents(WebContents()),
autofill::ContentAutofillClient::FromWebContents(WebContents()));
WaitForStatus(TestGenerationPopupObserver::GenerationPopup::kShown);
WaitForGenerationPopupShowing();
EXPECT_TRUE(GenerationPopupShowing());

// Click on the password field to display the autofill popup.
Expand All @@ -294,7 +342,7 @@ IN_PROC_BROWSER_TEST_F(
IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
PopupShownAndDismissed) {
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();

FocusUsernameField();

Expand All @@ -305,7 +353,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
PopupShownAndDismissedByKeyPress) {
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();

SendKeyToPopup(ui::VKEY_ESCAPE);

Expand All @@ -316,7 +364,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
PopupShownAndDismissedByScrolling) {
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();

ASSERT_TRUE(content::ExecJs(WebContents(), "window.scrollTo(100, 0);"));

Expand All @@ -330,12 +378,11 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
// Execute the script in the context of the iframe so that it kinda receives a
// user gesture.
content::RenderFrameHost* child_frame = ChildFrameAt(WebContents(), 0);
gfx::PointF click_point =
GetCenterCoordinatesOfElementWithId(child_frame, "password_field");
SimulateMouseClickOrTapAt(child_frame->GetRenderWidgetHost(), click_point);

std::string focus_script =
"document.getElementById('password_field').focus();";

ASSERT_TRUE(content::ExecJs(child_frame, focus_script));
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();
}

IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
Expand Down Expand Up @@ -392,7 +439,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS).get());

FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();
SendKeyToPopup(ui::VKEY_DOWN);
SendKeyToPopup(ui::VKEY_RETURN);

Expand Down Expand Up @@ -428,7 +475,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationInteractiveTest,
NavigatingAwayClosesPopup) {
// Open popup.
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();

// Simulate navigating to a different page.
NavigateToFile("/password/signup_form.html");
Expand Down Expand Up @@ -463,7 +510,7 @@ IN_PROC_BROWSER_TEST_F(PasswordGenerationPopupViewPrerenderingTest,
PasswordGenerationPopupControllerInPrerendering) {
// Open popup.
FocusPasswordField();
EXPECT_TRUE(GenerationPopupShowing());
WaitForGenerationPopupShowing();

auto prerender_url = embedded_test_server()->GetURL("/empty.html");
// Loads a page in the prerender.
Expand Down
11 changes: 6 additions & 5 deletions chrome/renderer/autofill/password_autofill_agent_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2932,7 +2932,7 @@ TEST_F(PasswordAutofillAgentTest,
/*new_password_id=*/"password", /*confirm_password_id=*/nullptr);
// Simulate the user clicks on a password field, that leads to showing
// generaiton pop-up. GeneratedPasswordAccepted can't be called without it.
FocusElement(kPasswordName);
SimulateElementClick(kPasswordName);

std::u16string password = u"NewPass22";
EXPECT_CALL(fake_pw_client_, PresaveGeneratedPassword(_, Eq(password)));
Expand All @@ -2953,7 +2953,7 @@ TEST_F(PasswordAutofillAgentTest,
/*new_password_id=*/"password", /*confirm_password_id=*/nullptr);
// Simulate the user clicks on a password field, that leads to showing
// generaiton pop-up. GeneratedPasswordAccepted can't be called without it.
FocusElement(kPasswordName);
SimulateElementClick(kPasswordName);
std::u16string password = u"NewPass22";
EXPECT_CALL(fake_pw_client_, PresaveGeneratedPassword(_, Eq(password)));
password_generation_->GeneratedPasswordAccepted(password);
Expand Down Expand Up @@ -3022,9 +3022,10 @@ TEST_F(PasswordAutofillAgentTest, PasswordGenerationSupersedesAutofill) {

// Simulate the field being clicked to start typing. This should trigger
// generation but not password autofill.
SetFocused(password_element_);
EXPECT_CALL(fake_pw_client_, AutomaticGenerationAvailable(_));
FocusElement("new_password");
SimulateElementClick("new_password");
// TODO(crbug.com/1473553): Expect the call precisely once.
EXPECT_CALL(fake_pw_client_, AutomaticGenerationAvailable(_))
.Times(testing::AtLeast(1));
base::RunLoop().RunUntilIdle();
testing::Mock::VerifyAndClearExpectations(&fake_pw_client_);
EXPECT_CALL(fake_driver_, ShowPasswordSuggestions).Times(0);
Expand Down

0 comments on commit 596072c

Please sign in to comment.