Adding a11y test coverage for passwords edit page.

ios/chrome/browser/ui/settings/settings_egtest.mm

 // Copyright 2016 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.
 
 #import <EarlGrey/EarlGrey.h>
 #import <UIKit/UIKit.h>
 #import <XCTest/XCTest.h>
 #include <map>
 
 #include "base/bind.h"
@@ skipping 104 matching lines @@
       IDS_OPTIONS_ADVANCED_SECTION_TITLE_PRIVACY);
 }
 // Matcher for the Clear Browsing Data cell on the Privacy screen.
 id<GREYMatcher> ClearBrowsingDataCell() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_CLEAR_BROWSING_DATA_TITLE);
 }
 
 // Matcher for the Search Engine cell on the main Settings screen.
 id<GREYMatcher> SearchEngineButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_SEARCH_ENGINE_SETTING_TITLE);
 }

[baxley, 2017/02/17 15:38:00]

Thank you for the clean up, outside of your code!

There are about 10 other matchers above this, would you mind fixing the spacing
there too?

[lindsayw, 2017/02/17 22:01:57]

Done.

-
 // Matcher for the Autofill Forms cell on the main Settings screen.
 id<GREYMatcher> AutofillButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_AUTOFILL);
 }
-
 // Matcher for the Google Apps cell on the main Settings screen.
 id<GREYMatcher> GoogleAppsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_GOOGLE_APPS_SM_SETTINGS);
 }
-
 // Matcher for the Google Chrome cell on the main Settings screen.
 id<GREYMatcher> GoogleChromeButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_PRODUCT_NAME);
 }
 
 // Matcher for the Google Chrome cell on the main Settings screen.
 id<GREYMatcher> VoiceSearchButton() {
   return grey_allOf(grey_accessibilityID(kSettingsVoiceSearchCellId),
                     grey_accessibilityTrait(UIAccessibilityTraitButton), nil);
 }
@@ skipping 12 matching lines @@
 // Matcher for the Privacy Block Popups button on the privacy UI.
 id<GREYMatcher> BlockPopupsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_BLOCK_POPUPS);
 }
 
 // Matcher for the Privacy Translate Settings button on the privacy UI.
 id<GREYMatcher> TranslateSettingsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_TRANSLATE_SETTING);
 }
 
+// Matcher for the save button in the save password bar.
+id<GREYMatcher> savePasswordButton() {
+  return ButtonWithAccessibilityLabelId(IDS_IOS_PASSWORD_MANAGER_SAVE_BUTTON);
+}

[baxley, 2017/02/17 15:38:00]

nit: blank line?

[lindsayw, 2017/02/17 22:01:56]

Done.

 // Asserts that there is no cookie in current web state.
 void AssertNoCookieExists() {
   NSError* error = nil;
   chrome_test_util::ExecuteJavaScript(
       base::SysUTF8ToNSString(kJavaScriptGetCookies), &error);
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kNoCookieText)]
       assertWithMatcher:grey_notNil()];
 }
 
@@ skipping 233 matching lines @@
       saved ? grey_sufficientlyVisible() : grey_notVisible();
   [[EarlGrey selectElementWithMatcher:
                  grey_text(l10n_util::GetNSString(
                      IDS_PASSWORD_MANAGER_SHOW_PASSWORDS_TAB_TITLE))]
       assertWithMatcher:visibilityMatcher];
 
   // Close the Settings.
   [self closeSubSettingsMenu];
 }
 
+// Helper to load a page with a login and submit it.

[baxley, 2017/02/17 15:38:00]

nit:s/Helper to load/Loads

[lindsayw, 2017/02/17 22:01:57]

Done.

+- (void)loadAndSubmitTheForm {

[baxley, 2017/02/17 15:38:00]

Would it be okay to call it loadFormAndLogin ?

"And" isn't great to use, but would it be more clear to say Login, than Submit?

[lindsayw, 2017/02/17 22:01:57]

Done.

+  std::map<GURL, std::string> responses;
+  const GURL URL = web::test::HttpServer::MakeUrl("http://testClearPasswords");
+  // TODO(crbug.com/432596): There looks to be a bug where the save password
+  // infobar is not displayed if the action is about:blank.
+  responses[URL] =
+      "<form method=\"POST\" action=\"dest\">"
+      "Username:<input type=\"text\" name=\"username\" value=\"name\" /><br />"
+      "Password:<input type=\"password\""
+      "name=\"password\" value=\"pass\"/><br />"
+      "<input type=\"submit\" value=\"Login\" id=\"Login\"/>"
+      "</form>";
+  const GURL destinationURL =
+      web::test::HttpServer::MakeUrl("http://testClearPasswords/dest");
+  responses[destinationURL] = "Logged in!";
+  web::test::SetUpSimpleHttpServer(responses);
+  // Login to page and click to save password and check that its saved.
+  [ChromeEarlGrey loadURL:URL];
+  chrome_test_util::TapWebViewElementWithId("Login");
+  [[EarlGrey selectElementWithMatcher:savePasswordButton()]
+      performAction:grey_tap()];
+}
+
+// Helper to open the passwords page.

[baxley, 2017/02/17 15:38:00]

nit: "Opens the passwords page. It requires no menus to be open."

If there are other constraints (NTP, web view, ...), feel free to include that
in the comment.

[lindsayw, 2017/02/17 22:01:57]

Done.

+- (void)openPasswordSettings {
+  // Open settings and verify data in the view controller.
+  [ChromeEarlGreyUI openToolsMenu];
+  [[EarlGrey
+      selectElementWithMatcher:grey_accessibilityID(kToolsMenuSettingsId)]
+      performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:PasswordsButton()]
+      performAction:grey_tap()];
+}
+
 // Checks for a given service that it is both recording and uploading, where
 // appropriate.
 - (void)assertMetricsServiceEnabled:(MetricsServiceType)serviceType {
   switch (serviceType) {
     case kMetrics:
       GREYAssertTrue(chrome_test_util::IsMetricsRecordingEnabled(),
                      @"Metrics recording should be enabled.");
       GREYAssertTrue(chrome_test_util::IsMetricsReportingEnabled(),
                      @"Metrics reporting should be enabled.");
       break;
@@ skipping 219 matching lines @@
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kResponse)]
       assertWithMatcher:grey_notNil()];
   AssertNoCookieExists();
   chrome_test_util::CloseAllTabs();
 }
 
 // Verifies that logging into a form on a web page allows the user to save and
 // then clear a password.
 - (void)testClearPasswords {
-  std::map<GURL, std::string> responses;
-  const GURL URL = web::test::HttpServer::MakeUrl("http://testClearPasswords");
-  // TODO(crbug.com/432596): There looks to be a bug where the save password
-  // infobar is not displayed if the action is about:blank.
-  responses[URL] =
-      "<form method=\"POST\" action=\"dest\">"
-      "Username:<input type=\"text\" name=\"username\" value=\"name\" /><br />"
-      "Password:<input type=\"password\""
-      "name=\"password\" value=\"pass\"/><br />"
-      "<input type=\"submit\" value=\"Login\" id=\"Login\"/>"
-      "</form>";
-  const GURL destinationURL =
-      web::test::HttpServer::MakeUrl("http://testClearPasswords/dest");
-  responses[destinationURL] = "Logged in!";
-  web::test::SetUpSimpleHttpServer(responses);
 
   // Enable password management.
   ios::ChromeBrowserState* browserState =
       chrome_test_util::GetOriginalBrowserState();
   PrefService* preferences = browserState->GetPrefs();
   bool originalPasswordManagerSavingEnabled = preferences->GetBoolean(
       password_manager::prefs::kPasswordManagerSavingEnabled);
   preferences->SetBoolean(
       password_manager::prefs::kPasswordManagerSavingEnabled, true);
   [self setTearDownHandler:^{
@@ skipping 11 matching lines @@
     preferences->SetBoolean(browsing_data::prefs::kDeleteCookies, true);
     preferences->SetBoolean(browsing_data::prefs::kDeletePasswords, false);
     preferences->SetBoolean(browsing_data::prefs::kDeleteFormData, false);
   }];
 
   // Clear passwords and check that none are saved.
   [self clearPasswords];
   [self checkIfPasswordsSaved:NO];
 
   // Login to page and click to save password and check that its saved.
-  [ChromeEarlGrey loadURL:URL];
-  chrome_test_util::TapWebViewElementWithId("Login");
-  [[EarlGrey selectElementWithMatcher:ButtonWithAccessibilityLabelId(
-                                          IDS_IOS_PASSWORD_MANAGER_SAVE_BUTTON)]
-      performAction:grey_tap()];
-
+  [self loadAndSubmitTheForm];
   [self checkIfPasswordsSaved:YES];
 
   // Clear passwords and check that none are saved.
   [self clearPasswords];
   [self checkIfPasswordsSaved:NO];
 }
 
 // Verifies that metrics reporting works properly under possible settings of the
 // preferences kMetricsReportingEnabled and kMetricsReportingWifiOnly.
 - (void)testMetricsReporting {
@@ skipping 219 matching lines @@
   chrome_test_util::VerifyAccessibilityForCurrentScreen();
   [self closeSubSettingsMenu];
 }
 
 // Verifies the UI elements are accessible on the Passwords page.
 - (void)testAccessibilityOnPasswords {
   [ChromeEarlGreyUI openToolsMenu];
   [[EarlGrey selectElementWithMatcher:SettingsButton()]
       performAction:grey_tap()];
   [[EarlGrey selectElementWithMatcher:PasswordsButton()]
       performAction:grey_tap()];

[baxley, 2017/02/17 15:38:00]

Could we make this test match the new test you wrote? I don't feel super
strongly about having the helper to open the password settings, but I think it
should be done both ways. Up to you what to do:
1. Delete the previous 3 commands and replace with the openPasswordSettings()
2. Delete the helper and call the previous 3 lines in the next test.

[lindsayw, 2017/02/17 22:01:56]

Done, Option 1.

   chrome_test_util::VerifyAccessibilityForCurrentScreen();
   [self closeSubSettingsMenu];
 }
 
+// Add a saved password and verify the edit password page is accessible.

[baxley, 2017/02/17 15:38:00]

nit: should start with an active verb. What about something like this? (feel
free to make it sound better)

"Tests that saved passwords are accessible in Password Settings".

(I'm not sure about capitalization)

[lindsayw, 2017/02/17 22:01:56]

Done. "Verifies that saved passwords are accessible in Passwords page."

+- (void)testAccessibilityOnPasswordEditing {
+  [self clearPasswords];
+  [self checkIfPasswordsSaved:NO];
+
+  [self loadAndSubmitTheForm];
+  [self openPasswordSettings];
+
+  // Switch on edit mode.
+  [[EarlGrey selectElementWithMatcher:ButtonWithAccessibilityLabelId(
+                                          IDS_IOS_NAVIGATION_BAR_EDIT_BUTTON)]
+      performAction:grey_tap()];
+  chrome_test_util::VerifyAccessibilityForCurrentScreen();
+
+  // Exit settings.
+  [[EarlGrey
+      selectElementWithMatcher:grey_allOf(
+                                   grey_accessibilityID(@"ic_arrow_back"),
+                                   grey_accessibilityTrait(
+                                       UIAccessibilityTraitButton),
+                                   nil)] performAction:grey_tap()];
+
+  [[EarlGrey
+      selectElementWithMatcher:chrome_test_util::ButtonWithAccessibilityLabelId(
+                                   IDS_IOS_NAVIGATION_BAR_DONE_BUTTON)]

[baxley, 2017/02/17 15:38:00]

I filed a bug to create a custom matcher (crbug.com/693579).

It's used about 50 times! in our tests, so definitely not part of this CL.

This is just FYI, no action needed on your part in this CL, or any follow up.

[lindsayw, 2017/02/17 22:01:56]

Acknowledged.

+      performAction:grey_tap()];
+
+  [self clearPasswords];

[baxley, 2017/02/17 15:38:00]

Is this safe to call if there are no saved passwords? If so, can we call
setTearDownHandler with this?

Doing that will guarantee this method gets called. If some assertion above (i.e.
VerifyAccessibilityOnCurrentScreen()) fails, then it exits and doesn't execute
this part of the test.

[lindsayw, 2017/02/17 22:01:56]

No, clearPasswords has the limitation discussed in crbug/689663 where an
improvement needs to be made to first eval the settings and only set it for
clearing if it's not already set, then go back and make sure all settings are
left in the default state. As such, I think usiI added the tearDown to this
method, but putting clearPasswords at the beginning of it dchecks and at the end
of it causes an error because of the dismissSettings call. Please let me know
what you think.

+}
+
 @end