[Bling Password Manager] Using button click as indicator of submission.
On other platforms pretty sophisticated mechanisms for finding form
submissions are used (for example checking results of XHR requests,
listening history.pushState calls). On IOS before this CL only normal
HTML form submissions were used for finding form submissions.
According to UMA, ~40% password submissions are done without HTML
form submission, that makes huge gap in possibility to save passwords
on IOS and other platforms.
This CL adds processing of button clicks events as indicator of form
submissions. Namely, if password form has 1 button, clicking on this
button is considered as candidate for submission. The save popup is shown
if on next navigation the form is disappeared (i.e. the submissions is
considered to be correct). Note that touchend event is used instead of
click event, since many sites stop handling of click event and Password
Manager can't process it.
This CL alone wouldn't bring submission detection on IOS to the level of
other platforms, since submission detection is very hard. It's the first
step to improve it. This CL fixes saving on facebook and dropbox.
Bug: 620300, 714762, 626636
Change-Id: Ibe3dd8afbe963d1425c2adb6da7c54c96a900c7c
Reviewed-on: https://chromium-review.googlesource.com/666958
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503740}diff --git a/ios/chrome/browser/passwords/password_controller.mm b/ios/chrome/browser/passwords/password_controller.mm
index 3365d05..83654051 100644
--- a/ios/chrome/browser/passwords/password_controller.mm
+++ b/ios/chrome/browser/passwords/password_controller.mm
@@ -14,6 +14,7 @@
#include "base/json/json_reader.h"
#include "base/json/json_writer.h"
+#import "base/mac/bind_objc_block.h"
#include "base/mac/foundation_util.h"
#include "base/memory/ptr_util.h"
#include "base/strings/string16.h"
@@ -61,6 +62,10 @@
namespace {
// Types of password infobars to display.
enum class PasswordInfoBarType { SAVE, UPDATE };
+
+// Script command prefix for form changes. Possible command to be sent from
+// injected JS is 'form.buttonClicked'.
+constexpr char kCommandPrefix[] = "passwordForm";
}
@interface PasswordController ()
@@ -131,6 +136,9 @@
- (void)showInfoBarForForm:(std::unique_ptr<PasswordFormManager>)form
infoBarType:(PasswordInfoBarType)type;
+// Handler for injected JavaScript callbacks.
+- (BOOL)handleScriptCommand:(const base::DictionaryValue&)JSONCommand;
+
@end
namespace {
@@ -326,6 +334,15 @@
credentialManager_ = base::MakeUnique<CredentialManager>(
passwordManagerClient_.get(), webState_);
}
+
+ __weak PasswordController* weakSelf = self;
+ auto callback = base::BindBlockArc(^bool(const base::DictionaryValue& JSON,
+ const GURL& originURL,
+ bool userIsInteracting) {
+ // |originURL| and |isInteracting| aren't used.
+ return [weakSelf handleScriptCommand:JSON];
+ });
+ webState->AddScriptCommandCallback(callback, kCommandPrefix);
}
return self;
}
@@ -352,6 +369,8 @@
}
- (void)detach {
+ if (webState_)
+ webState_->RemoveScriptCommandCallback(kCommandPrefix);
webState_ = nullptr;
webStateObserverBridge_.reset();
passwordGenerationAgent_ = nil;
@@ -873,6 +892,29 @@
completionHandler:completionHandler];
}
+- (BOOL)handleScriptCommand:(const base::DictionaryValue&)JSONCommand {
+ std::string command;
+ if (!JSONCommand.GetString("command", &command))
+ return NO;
+
+ if (command != "passwordForm.submitButtonClick")
+ return NO;
+
+ GURL pageURL;
+ if (!GetPageURLAndCheckTrustLevel(webState_, &pageURL))
+ return NO;
+ autofill::PasswordForm form;
+ BOOL formParsedFromJSON =
+ [self getPasswordForm:&form fromDictionary:&JSONCommand pageURL:pageURL];
+ if (formParsedFromJSON && ![self isWebStateDestroyed]) {
+ self.passwordManager->OnPasswordFormSubmitted(self.passwordManagerDriver,
+ form);
+ return YES;
+ }
+
+ return NO;
+}
+
- (PasswordGenerationAgent*)passwordGenerationAgent {
return passwordGenerationAgent_;
}
diff --git a/ios/chrome/browser/passwords/password_controller_js_unittest.mm b/ios/chrome/browser/passwords/password_controller_js_unittest.mm
index 671bf85..5a09f1a 100644
--- a/ios/chrome/browser/passwords/password_controller_js_unittest.mm
+++ b/ios/chrome/browser/passwords/password_controller_js_unittest.mm
@@ -331,4 +331,60 @@
ExecuteJavaScriptWithFormat(@"__gCrWeb.findPasswordForms()"));
};
+// Checks that a touchend event from a button which contains in a password form
+// works as a submission indicator for this password form.
+TEST_F(PasswordControllerJsTest, TouchendAsSubmissionIndicator) {
+ LoadHtmlAndInject(
+ @"<html><body>"
+ "<form name='login_form' id='login_form'>"
+ " Name: <input type='text' name='username'>"
+ " Password: <input type='password' name='password'>"
+ " <button id='submit_button' value='Submit'>"
+ "</form>"
+ "</body></html>");
+
+ // Call __gCrWeb.findPasswordForms in order to set an event handler on the
+ // button touchend event.
+ ExecuteJavaScriptWithFormat(@"__gCrWeb.findPasswordForms()");
+
+ // Replace __gCrWeb.message.invokeOnHost with mock method for checking of call
+ // arguments.
+ ExecuteJavaScriptWithFormat(
+ @"var invokeOnHostArgument = null;"
+ "var invokeOnHostCalls = 0;"
+ "__gCrWeb.message.invokeOnHost = function(command) {"
+ " invokeOnHostArgument = command;"
+ " invokeOnHostCalls++;"
+ "}");
+
+ // Simulate touchend event on the button.
+ ExecuteJavaScriptWithFormat(
+ @"document.getElementsByName('username')[0].value = 'user1';"
+ "document.getElementsByName('password')[0].value = 'password1';"
+ "var e = new UIEvent('touchend');"
+ "document.getElementsByTagName('button')[0].dispatchEvent(e);");
+
+ // Check that there was only 1 call for invokeOnHost.
+ EXPECT_NSEQ(@1, ExecuteJavaScriptWithFormat(@"invokeOnHostCalls"));
+
+ NSString* expected_command = [NSString
+ stringWithFormat:
+ @"{\"action\":\"%s\","
+ "\"method\":null,"
+ "\"name\":\"login_form\","
+ "\"origin\":\"%s\","
+ "\"fields\":[{\"element\":\"username\",\"type\":\"text\"},"
+ "{\"element\":\"password\",\"type\":\"password\"}],"
+ "\"usernameElement\":\"username\","
+ "\"usernameValue\":\"user1\","
+ "\"passwords\":[{\"element\":\"password\",\"value\":\"password1\"}],"
+ "\"command\":\"passwordForm.submitButtonClick\"}",
+ BaseUrl().c_str(), BaseUrl().c_str()];
+
+ // Check that invokeOnHost was called with the correct argument.
+ EXPECT_NSEQ(
+ expected_command,
+ ExecuteJavaScriptWithFormat(@"__gCrWeb.stringify(invokeOnHostArgument)"));
+};
+
} // namespace
diff --git a/ios/chrome/browser/passwords/password_controller_unittest.mm b/ios/chrome/browser/passwords/password_controller_unittest.mm
index 3675c0f..a529927 100644
--- a/ios/chrome/browser/passwords/password_controller_unittest.mm
+++ b/ios/chrome/browser/passwords/password_controller_unittest.mm
@@ -161,7 +161,7 @@
void SetUp() override {
web::WebTestWithWebState::SetUp();
passwordController_ =
- CreatePasswordController(web_state(), store_.get(), nullptr);
+ CreatePasswordController(web_state(), store_.get(), &weak_client_);
@autoreleasepool {
// Make sure the temporary array is released after SetUp finishes,
// otherwise [passwordController_ suggestionProvider] will be retained
@@ -247,6 +247,8 @@
PasswordController* passwordController_;
scoped_refptr<password_manager::MockPasswordStore> store_;
+
+ MockPasswordManagerClient* weak_client_;
};
struct PasswordFormTestData {
@@ -1421,3 +1423,35 @@
return *p_get_logins_called;
});
}
+
+// Tests that a touchend event from a button which contains in a password form
+// works as a submission indicator for this password form.
+TEST_F(PasswordControllerTest, TouchendAsSubmissionIndicator) {
+ LoadHtml(
+ @"<html><body>"
+ "<form name='login_form' id='login_form'>"
+ " <input type='text' name='username'>"
+ " <input type='password' name='password'>"
+ " <button id='submit_button' value='Submit'>"
+ "</form>"
+ "</body></html>");
+ // Use a mock LogManager to detect that OnPasswordFormSubmitted has been
+ // called. TODO(crbug.com/598672): this is a hack, we should modularize the
+ // code better to allow proper unit-testing.
+ MockLogManager log_manager;
+ EXPECT_CALL(log_manager, IsLoggingActive()).WillRepeatedly(Return(true));
+ const char kExpectedMessage[] =
+ "Message: \"PasswordManager::ProvisionallySavePassword\"\n";
+ EXPECT_CALL(log_manager, LogSavePasswordProgress(kExpectedMessage));
+ EXPECT_CALL(log_manager,
+ LogSavePasswordProgress(testing::Ne(kExpectedMessage)))
+ .Times(testing::AnyNumber());
+ EXPECT_CALL(*weak_client_, GetLogManager())
+ .WillRepeatedly(Return(&log_manager));
+
+ ExecuteJavaScript(
+ @"document.getElementsByName('username')[0].value = 'user1';"
+ "document.getElementsByName('password')[0].value = 'password1';"
+ "var e = new UIEvent('touchend');"
+ "document.getElementsByTagName('button')[0].dispatchEvent(e);");
+}
diff --git a/ios/chrome/browser/passwords/resources/password_controller.js b/ios/chrome/browser/passwords/resources/password_controller.js
index e537dcc..3ba6e338 100644
--- a/ios/chrome/browser/passwords/resources/password_controller.js
+++ b/ios/chrome/browser/passwords/resources/password_controller.js
@@ -93,6 +93,35 @@
};
/**
+ * If |form| has no submit elements and exactly 1 button that button
+ * is assumed to be a submit button. This function adds onSubmitButtonClick_
+ * as a handler for touchend event of this button. Touchend event is used as
+ * a proxy for onclick event because onclick handling might be prevented by
+ * the site JavaScript.
+ */
+ var addSubmitButtonTouchEndHandler_ = function(form) {
+ if (form.querySelector('input[type=submit]'))
+ return;
+ var buttons = form.querySelectorAll('button');
+ if (buttons.length != 1)
+ return;
+ buttons[0].addEventListener('touchend', onSubmitButtonTouchEnd_);
+ };
+
+ /**
+ * Click handler for the submit button. It sends to the host
+ * form.submitButtonClick command.
+ */
+ var onSubmitButtonTouchEnd_ = function(evt) {
+ var form = evt.currentTarget.form;
+ var formData = __gCrWeb.getPasswordFormData(form);
+ if (!formData)
+ return;
+ formData["command"] = 'passwordForm.submitButtonClick';
+ __gCrWeb.message.invokeOnHost(formData);
+ };
+
+ /**
* Returns the password form with the given |name| as a JSON string.
* @param {string} name The name of the form to extract.
* @return {string} The password form.
@@ -412,6 +441,7 @@
var formData = __gCrWeb.getPasswordFormData(forms[i]);
if (formData) {
formDataList.push(formData);
+ addSubmitButtonTouchEndHandler_(forms[i]);
}
}
