[Passwords] Detect whether a form is ready for submission before TTF

Before this CL, to submit or not was decided after filling by TTF (PasswordAutofillAgent::TriggerSubmission). We want to variate the button title and other UI strings in TTF depending on whether TTF is going to submit form or not after filling. In order to variate strings, the renderer should calculate whether a form is ready for submission when it shows TTF (PasswordAutofillAgent::TryToShowTouchToFill) and propagate it to the browser. TODO in another CL: propagate the verdict to Java and variate the button title. Tests: - TouchToFillControllerTest_Show_Fill_And_Submit - TouchToFillControllerTest_Show_Fill_And_Dont_Submit (new) - TouchToFillControllerTest_Show_And_Fill_Auth_Available_Success - PasswordAutofillAgentTest_TryToShowTouchToFill* Bug: 1283004 Change-Id: I9cf93aa61a213618c8821e33a3988fb801d1d979 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3477297 Reviewed-by: Colin Blundell <[email protected]> Reviewed-by: Dave Tapuska <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Vasilii Sukhanov <[email protected]> Commit-Queue: Maxim Kolosovskiy <[email protected]> Cr-Commit-Position: refs/heads/main@{#976298}
stjohnalex · Mar 1, 2022 · 95d009a · 95d009a
1 parent 3c219c1
commit 95d009a
Show file tree

Hide file tree

Showing 16 changed files with 168 additions and 48 deletions.
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -423,15 +423,15 @@ bool ChromePasswordManagerClient::PromptUserToChooseCredentials(
 #endif
 }
 
-void ChromePasswordManagerClient::ShowTouchToFill(
-    PasswordManagerDriver* driver) {
+void ChromePasswordManagerClient::ShowTouchToFill(PasswordManagerDriver* driver,
+                                                  bool trigger_submission) {
 #if BUILDFLAG(IS_ANDROID)
   GetOrCreateTouchToFillController()->Show(
       credential_cache_
           .GetCredentialStore(url::Origin::Create(
               driver->GetLastCommittedURL().DeprecatedGetOriginAsURL()))
           .GetCredentials(),
-      driver->AsWeakPtr());
+      driver->AsWeakPtr(), trigger_submission);
 #endif
 }
 

diff --git a/chrome/browser/password_manager/chrome_password_manager_client.h b/chrome/browser/password_manager/chrome_password_manager_client.h
@@ -124,8 +124,8 @@ class ChromePasswordManagerClient
       std::vector<std::unique_ptr<password_manager::PasswordForm>> local_forms,
       const url::Origin& origin,
       CredentialsCallback callback) override;
-  void ShowTouchToFill(
-      password_manager::PasswordManagerDriver* driver) override;
+  void ShowTouchToFill(password_manager::PasswordManagerDriver* driver,
+                       bool trigger_submission) override;
 
 #if BUILDFLAG(IS_ANDROID)
   // Notifies `PasswordReuseDetectionManager` about passwords selected from

diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -4324,7 +4324,7 @@ class MockPrerenderPasswordManagerDriver
                int options,
                const gfx::RectF& bounds),
               (override));
-  MOCK_METHOD(void, ShowTouchToFill, (), (override));
+  MOCK_METHOD(void, ShowTouchToFill, (bool), (override));
   MOCK_METHOD(void,
               CheckSafeBrowsingReputation,
               (const GURL& form_action, const GURL& frame_url),
@@ -4390,9 +4390,10 @@ class MockPrerenderPasswordManagerDriver
           impl_->ShowPasswordSuggestions(text_direction, typed_username,
                                          options, bounds);
         });
-    ON_CALL(*this, ShowTouchToFill).WillByDefault([this]() {
-      impl_->ShowTouchToFill();
-    });
+    ON_CALL(*this, ShowTouchToFill)
+        .WillByDefault([this](bool trigger_submission) {
+          impl_->ShowTouchToFill(trigger_submission);
+        });
     ON_CALL(*this, CheckSafeBrowsingReputation)
         .WillByDefault([this](const GURL& form_action, const GURL& frame_url) {
           impl_->CheckSafeBrowsingReputation(form_action, frame_url);

diff --git a/chrome/browser/touch_to_fill/touch_to_fill_controller.cc b/chrome/browser/touch_to_fill/touch_to_fill_controller.cc
@@ -75,9 +75,12 @@ TouchToFillController::~TouchToFillController() {
 }
 
 void TouchToFillController::Show(base::span<const UiCredential> credentials,
-                                 base::WeakPtr<PasswordManagerDriver> driver) {
+                                 base::WeakPtr<PasswordManagerDriver> driver,
+                                 bool trigger_submission) {
   DCHECK(!driver_ || driver_.get() == driver.get());
   driver_ = std::move(driver);
+  // TODO(crbug.com/1283004): Propagte this to Java to variate UI strings.
+  trigger_submission_ = trigger_submission;
 
   base::UmaHistogramCounts100("PasswordManager.TouchToFill.NumCredentialsShown",
                               credentials.size());
@@ -185,9 +188,11 @@ void TouchToFillController::FillCredential(const UiCredential& credential) {
 
   driver_->FillSuggestion(credential.username(), credential.password());
 
-  if (base::FeatureList::IsEnabled(
-          password_manager::features::kTouchToFillPasswordSubmission)) {
-    driver_->TriggerFormSubmission();
+  if (trigger_submission_) {
+    if (base::FeatureList::IsEnabled(
+            password_manager::features::kTouchToFillPasswordSubmission)) {
+      driver_->TriggerFormSubmission();
+    }
   }
   driver_ = nullptr;
 

diff --git a/chrome/browser/touch_to_fill/touch_to_fill_controller.h b/chrome/browser/touch_to_fill/touch_to_fill_controller.h
@@ -69,7 +69,8 @@ class TouchToFillController {
 
   // Instructs the controller to show the provided |credentials| to the user.
   void Show(base::span<const password_manager::UiCredential> credentials,
-            base::WeakPtr<password_manager::PasswordManagerDriver> driver);
+            base::WeakPtr<password_manager::PasswordManagerDriver> driver,
+            bool trigger_submission);
 
   // Informs the controller that the user has made a selection. Invokes both
   // FillSuggestion() and TouchToFillDismissed() on |driver_|. No-op if invoked
@@ -109,6 +110,10 @@ class TouchToFillController {
   // OnCredentialSelected() or OnDismissed() gets called.
   base::WeakPtr<password_manager::PasswordManagerDriver> driver_;
 
+  // Whether the controller should trigger submission when a credential is
+  // filled in.
+  bool trigger_submission_ = false;
+
   // Authenticator used to trigger a biometric auth before filling.
   scoped_refptr<device_reauth::BiometricAuthenticator> authenticator_;
 

diff --git a/chrome/browser/touch_to_fill/touch_to_fill_controller_unittest.cc b/chrome/browser/touch_to_fill/touch_to_fill_controller_unittest.cc
@@ -161,7 +161,8 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_No_Auth) {
 
   EXPECT_CALL(*weak_view, Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                                ElementsAreArray(credentials)));
-  controller_no_auth->Show(credentials, driver().AsWeakPtr());
+  controller_no_auth->Show(credentials, driver().AsWeakPtr(),
+                           /*trigger_submission=*/false);
 
   // Test that we correctly log the absence of an Android credential.
   EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"alice"),
@@ -201,7 +202,8 @@ TEST_F(TouchToFillControllerTest, Show_Fill_And_Submit) {
 
   EXPECT_CALL(*weak_view, Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                                ElementsAreArray(credentials)));
-  controller_no_auth->Show(credentials, driver().AsWeakPtr());
+  controller_no_auth->Show(credentials, driver().AsWeakPtr(),
+                           /*trigger_submission=*/true);
 
   EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"alice"),
                                        std::u16string(u"p4ssw0rd")));
@@ -211,13 +213,43 @@ TEST_F(TouchToFillControllerTest, Show_Fill_And_Submit) {
   controller_no_auth->OnCredentialSelected(credentials[0]);
 }
 
+TEST_F(TouchToFillControllerTest, Show_Fill_And_Dont_Submit) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      password_manager::features::kTouchToFillPasswordSubmission);
+
+  std::unique_ptr<TouchToFillController> controller_no_auth =
+      CreateNoAuthController();
+  std::unique_ptr<MockTouchToFillView> mock_view =
+      std::make_unique<MockTouchToFillView>();
+  MockTouchToFillView* weak_view = mock_view.get();
+  controller_no_auth->set_view(std::move(mock_view));
+
+  UiCredential credentials[] = {
+      MakeUiCredential({.username = "alice", .password = "p4ssw0rd"})};
+
+  EXPECT_CALL(*weak_view, Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
+                               ElementsAreArray(credentials)));
+  controller_no_auth->Show(credentials, driver().AsWeakPtr(),
+                           /*trigger_submission=*/false);
+
+  EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"alice"),
+                                       std::u16string(u"p4ssw0rd")));
+  EXPECT_CALL(driver(), TouchToFillClosed(ShowVirtualKeyboard(false)));
+
+  EXPECT_CALL(driver(), TriggerFormSubmission()).Times(0);
+
+  controller_no_auth->OnCredentialSelected(credentials[0]);
+}
+
 TEST_F(TouchToFillControllerTest, Show_And_Fill_No_Auth_Available) {
   UiCredential credentials[] = {
       MakeUiCredential({.username = "alice", .password = "p4ssw0rd"})};
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 
   // Test that we correctly log the absence of an Android credential.
   EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"alice"),
@@ -248,7 +280,8 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_Auth_Available_Success) {
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/true);
 
   EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"alice"),
                                        std::u16string(u"p4ssw0rd")));
@@ -260,6 +293,9 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_Auth_Available_Success) {
   EXPECT_CALL(*authenticator(),
               Authenticate(BiometricAuthRequester::kTouchToFill, _))
       .WillOnce(RunOnceCallback<1>(true));
+  // Without |kTouchToFillPasswordSubmission|, |trigger_submission=true| has no
+  // effect.
+  EXPECT_CALL(driver(), TriggerFormSubmission()).Times(0);
   touch_to_fill_controller().OnCredentialSelected(credentials[0]);
 }
 
@@ -269,7 +305,8 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_Auth_Available_Failure) {
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 
   EXPECT_CALL(driver(), FillSuggestion(_, _)).Times(0);
   EXPECT_CALL(driver(), TouchToFillClosed(ShowVirtualKeyboard(true)));
@@ -289,7 +326,8 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_Auth_Available_Failure) {
 
 TEST_F(TouchToFillControllerTest, Show_Empty) {
   EXPECT_CALL(view(), Show).Times(0);
-  touch_to_fill_controller().Show({}, driver().AsWeakPtr());
+  touch_to_fill_controller().Show({}, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
   histogram_tester().ExpectUniqueSample(
       "PasswordManager.TouchToFill.NumCredentialsShown", 0, 1);
 }
@@ -304,7 +342,8 @@ TEST_F(TouchToFillControllerTest, Show_Insecure_Origin) {
   EXPECT_CALL(view(),
               Show(Eq(GURL("http://example.com")), IsOriginSecure(false),
                    ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 }
 
 TEST_F(TouchToFillControllerTest, Show_And_Fill_Android_Credential) {
@@ -326,7 +365,8 @@ TEST_F(TouchToFillControllerTest, Show_And_Fill_Android_Credential) {
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 
   // Test that we correctly log the presence of an Android credential.
   EXPECT_CALL(driver(), FillSuggestion(std::u16string(u"bob"),
@@ -378,7 +418,8 @@ TEST_F(TouchToFillControllerTest, Show_Orders_Credentials) {
   UiCredential credentials[] = {alice, bob, charlie, david};
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            testing::ElementsAre(charlie, alice, bob, david)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 }
 
 TEST_F(TouchToFillControllerTest, Dismiss) {
@@ -387,7 +428,8 @@ TEST_F(TouchToFillControllerTest, Dismiss) {
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 
   EXPECT_CALL(driver(), TouchToFillClosed(ShowVirtualKeyboard(true)));
   touch_to_fill_controller().OnDismiss();
@@ -408,7 +450,8 @@ TEST_F(TouchToFillControllerTest, DestroyedWhileAuthRunning) {
 
   EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
                            ElementsAreArray(credentials)));
-  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr());
+  touch_to_fill_controller().Show(credentials, driver().AsWeakPtr(),
+                                  /*trigger_submission=*/false);
 
   EXPECT_CALL(*authenticator(),
               CanAuthenticate(BiometricAuthRequester::kTouchToFill))

diff --git a/chrome/renderer/autofill/fake_mojo_password_manager_driver.h b/chrome/renderer/autofill/fake_mojo_password_manager_driver.h
@@ -31,15 +31,20 @@ class FakeMojoPasswordManagerDriver
 
   // mojom::PasswordManagerDriver:
   // TODO(crbug.com/948062): Migrate the other methods to GMock as well.
-  MOCK_METHOD1(PasswordFormCleared, void(const autofill::FormData&));
-
-  MOCK_METHOD0(ShowTouchToFill, void());
-
-  MOCK_METHOD4(ShowPasswordSuggestions,
-               void(base::i18n::TextDirection,
-                    const std::u16string&,
-                    int,
-                    const gfx::RectF&));
+  MOCK_METHOD(void,
+              PasswordFormCleared,
+              (const autofill::FormData&),
+              (override));
+
+  MOCK_METHOD(void, ShowTouchToFill, (bool), (override));
+
+  MOCK_METHOD(void,
+              ShowPasswordSuggestions,
+              (base::i18n::TextDirection,
+               const std::u16string&,
+               int,
+               const gfx::RectF&),
+              (override));
 
   bool called_show_not_secure_warning() const {
     return called_show_not_secure_warning_;

diff --git a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
@@ -1774,7 +1774,12 @@ TEST_F(PasswordAutofillAgentTest, TryToShowTouchToFillUsername) {
   EXPECT_EQ(WebAutofillState::kPreviewed, username_element_.GetAutofillState());
   EXPECT_EQ(WebAutofillState::kPreviewed, password_element_.GetAutofillState());
 
+// TODO(crbug.com/1299430): Consider to disable |ShowTouchToFill| on Desktop.
+#if BUILDFLAG(IS_ANDROID)
+  EXPECT_CALL(fake_driver_, ShowTouchToFill(true));
+#else
   EXPECT_CALL(fake_driver_, ShowTouchToFill);
+#endif
   base::RunLoop().RunUntilIdle();
 }
 
@@ -1786,7 +1791,29 @@ TEST_F(PasswordAutofillAgentTest, TryToShowTouchToFillPassword) {
   EXPECT_TRUE(password_autofill_agent_->ShouldSuppressKeyboard());
   EXPECT_EQ(WebAutofillState::kPreviewed, password_element_.GetAutofillState());
 
+// TODO(crbug.com/1299430): Consider to disable |ShowTouchToFill| on Desktop.
+#if BUILDFLAG(IS_ANDROID)
+  EXPECT_CALL(fake_driver_, ShowTouchToFill(true));
+#else
   EXPECT_CALL(fake_driver_, ShowTouchToFill);
+#endif
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(PasswordAutofillAgentTest, TryToShowTouchToFillButDontEnableSubmission) {
+  LoadHTML(kPasswordChangeFormHTML);
+  UpdateUrlForHTML(kPasswordChangeFormHTML);
+  UpdateUsernameAndPasswordElements();
+  // Enable filling for the old password field.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  EXPECT_TRUE(
+      password_autofill_agent_->TryToShowTouchToFill(password_element_));
+  EXPECT_TRUE(password_autofill_agent_->ShouldSuppressKeyboard());
+  EXPECT_EQ(WebAutofillState::kPreviewed, password_element_.GetAutofillState());
+
+  // As there are other input fields, don't enable automatic submission.
+  EXPECT_CALL(fake_driver_, ShowTouchToFill(/*trigger_submission=*/false));
   base::RunLoop().RunUntilIdle();
 }
 

diff --git a/components/autofill/content/common/mojom/autofill_driver.mojom b/components/autofill/content/common/mojom/autofill_driver.mojom
@@ -137,8 +137,9 @@ interface PasswordManagerDriver {
                           mojo_base.mojom.String16 typed_username,
                           int32 options, gfx.mojom.RectF bounds);
 
-  // Instructs the browser to show the Touch To Fill UI.
-  ShowTouchToFill();
+  // Instructs the browser to show the Touch To Fill UI and whether form
+  // submission should be triggered after filling.
+  ShowTouchToFill(bool trigger_submission);
 
   // Checks the safe browsing reputation of the website where the focused
   // username/password field is on.