[iOS] UX/UI review implementation for Password Grouping (Part 1)

- Fix string difference for compromised password and for the delete button - Fix eg test delete button string value - Delete button should be left aligned in Password Details - Gray colored text when not editable in the Password Details - Remove pen icons when editing a password in Password Details Bug: 1409115 Change-Id: If0b83b0f514e73d4aa2e7df0f78c419ba2f2c59d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4185255 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Veronique Nguyen <veronguyen@google.com> Cr-Commit-Position: refs/heads/main@{#1097543}
chromium · Jan 26, 2023 · c0658a1 · c0658a1
1 parent e033c37
commit c0658a1
Show file tree

Hide file tree

Showing 12 changed files with 103 additions and 16 deletions.
diff --git a/ios/chrome/app/strings/ios_google_chrome_strings.grd b/ios/chrome/app/strings/ios_google_chrome_strings.grd
@@ -561,7 +561,7 @@ Things to consider:
         Chrome couldn't check all passwords. Try again later.
       </message>
       <message name="IDS_IOS_CHANGE_COMPROMISED_PASSWORD_DESCRIPTION_BRANDED" desc="Text inside Password Details screen shown when password was compromised. [iOS only]" meaning="Text which explains the user that current password is compromised and it should be changed.">
-        This password was found in a data breach. Google Password Manager recommends changing your password now.
+          Your password was exposed in a data breach. Google Password Manager recommends changing it now.
       </message>
       <message name="IDS_IOS_GOOGLE_SERVICES_SETTINGS_ALLOW_SIGNIN_DETAIL" desc="Feature detail text in the Google services Settings for the user to enable/disable Chrome express sign-in from the web [iOS only]">
         Shows prompts to sign in to Chrome.

diff --git a/...oogle_chrome_strings_grd/IDS_IOS_CHANGE_COMPROMISED_PASSWORD_DESCRIPTION_BRANDED.png.sha1 b/...oogle_chrome_strings_grd/IDS_IOS_CHANGE_COMPROMISED_PASSWORD_DESCRIPTION_BRANDED.png.sha1
@@ -1 +1 @@
-19692bf3c1301ddc77a34ad23a0d906f12645329
+9b95f55f26c701625a8869b31a70219a9d24394d
diff --git a/ios/chrome/browser/ui/settings/password/password_details/BUILD.gn b/ios/chrome/browser/ui/settings/password/password_details/BUILD.gn
@@ -22,6 +22,7 @@ source_set("password_details") {
     "//base",
     "//components/autofill/core/common",
     "//components/password_manager/core/browser/form_parsing",
+    "//components/password_manager/core/common:features",
     "//components/strings",
     "//components/url_formatter",
     "//ios/chrome/app/strings",

diff --git a/...ome/browser/ui/settings/password/password_details/cells/table_view_stacked_details_item.h b/...ome/browser/ui/settings/password/password_details/cells/table_view_stacked_details_item.h
@@ -19,6 +19,10 @@
 // Texts displayed in the details labels of the cell.
 @property(nonatomic, copy) NSArray<NSString*>* detailTexts;
 
+// Text color for the details labels of the cell. Default is [UIColor
+// colorNamed:kTextPrimaryColor].
+@property(nonatomic, strong) UIColor* detailTextColor;
+
 @end
 
 // TableViewCell displaying a title and multiple single-line details texts.
@@ -28,6 +32,7 @@
 @property(nonatomic, strong, readonly) NSArray<UILabel*>* detailLabels;
 
 - (void)setDetails:(NSArray<NSString*>*)detailTexts;
+- (void)setDetailTextColor:(UIColor*)detailTextColor;
 
 @end
 

diff --git a/...me/browser/ui/settings/password/password_details/cells/table_view_stacked_details_item.mm b/...me/browser/ui/settings/password/password_details/cells/table_view_stacked_details_item.mm
@@ -29,6 +29,9 @@ - (void)configureCell:(TableViewStackedDetailsCell*)cell
   cell.titleLabel.text = self.titleText;
 
   [cell setDetails:self.detailTexts];
+  [cell setDetailTextColor:self.detailTextColor
+                               ? self.detailTextColor
+                               : [UIColor colorNamed:kTextPrimaryColor]];
 
   cell.accessibilityLabel = [self accessibilityLabelForCell];
 }
@@ -84,6 +87,12 @@ - (void)setDetails:(NSArray<NSString*>*)detailTexts {
                 self.traitCollection.preferredContentSizeCategory)];
 }
 
+- (void)setDetailTextColor:(UIColor*)detailTextColor {
+  for (UILabel* detailsLabel in self.detailLabels) {
+    detailsLabel.textColor = detailTextColor;
+  }
+}
+
 #pragma mark - UITableViewCell
 
 - (void)prepareForReuse {
@@ -158,7 +167,6 @@ - (void)setDetailsStyling {
   for (UILabel* detailsLabel in _detailLabels) {
     detailsLabel.font = [UIFont preferredFontForTextStyle:UIFontTextStyleBody];
     detailsLabel.adjustsFontForContentSizeCategory = YES;
-    detailsLabel.textColor = [UIColor colorNamed:kTextPrimaryColor];
   }
 }
 

diff --git a/ios/chrome/browser/ui/settings/password/password_details/password_details_coordinator.mm b/ios/chrome/browser/ui/settings/password/password_details/password_details_coordinator.mm
@@ -10,6 +10,7 @@
 #import "components/password_manager/core/browser/password_manager_metrics_util.h"
 #import "components/password_manager/core/browser/ui/affiliated_group.h"
 #import "components/password_manager/core/browser/ui/credential_ui_entry.h"
+#import "components/password_manager/core/common/password_manager_features.h"
 #import "components/strings/grit/components_strings.h"
 #import "ios/chrome/browser/credential_provider_promo/features.h"
 #import "ios/chrome/browser/main/browser.h"
@@ -326,8 +327,14 @@ - (void)showPasswordDeleteDialogWithOrigin:(NSString*)origin
 
   __weak __typeof(self) weakSelf = self;
 
+  NSString* deleteButtonString =
+      l10n_util::GetNSString(base::FeatureList::IsEnabled(
+                                 password_manager::features::kPasswordsGrouping)
+                                 ? IDS_IOS_DELETE_ACTION_TITLE
+                                 : IDS_IOS_CONFIRM_PASSWORD_DELETION);
+
   [self.actionSheetCoordinator
-      addItemWithTitle:l10n_util::GetNSString(IDS_IOS_CONFIRM_PASSWORD_DELETION)
+      addItemWithTitle:deleteButtonString
                 action:^{
                   [weakSelf passwordDeletionConfirmedForCompromised:
                                 compromisedPassword

diff --git a/...e/browser/ui/settings/password/password_details/password_details_table_view_controller.mm b/...e/browser/ui/settings/password/password_details/password_details_table_view_controller.mm
@@ -227,6 +227,10 @@ - (TableViewStackedDetailsItem*)websiteItemForPasswordDetails:
       IsPasswordGroupingEnabled() ? IDS_IOS_SHOW_PASSWORD_VIEW_SITES
                                   : IDS_IOS_SHOW_PASSWORD_VIEW_SITE);
   item.detailTexts = passwordDetails.websites;
+  if (IsPasswordGroupingEnabled()) {
+    item.detailTextColor = [UIColor colorNamed:kTextSecondaryColor];
+    item.accessibilityTraits = UIAccessibilityTraitNotEnabled;
+  }
 
   return item;
 }
@@ -242,7 +246,7 @@ - (TableViewTextEditItem*)usernameItemForPasswordDetails:
   // If password is missing (federated credential) don't allow to edit username.
   if (passwordDetails.credentialType != CredentialTypeFederation) {
     item.textFieldEnabled = self.tableView.editing;
-    item.hideIcon = !self.tableView.editing;
+    item.hideIcon = !self.tableView.editing || IsPasswordGroupingEnabled();
     item.autoCapitalizationType = UITextAutocapitalizationTypeNone;
     item.delegate = self;
   } else {
@@ -251,6 +255,9 @@ - (TableViewTextEditItem*)usernameItemForPasswordDetails:
   }
   item.textFieldPlaceholder = l10n_util::GetNSString(
       IDS_IOS_PASSWORD_SETTINGS_USERNAME_PLACEHOLDER_TEXT);
+  if (IsPasswordGroupingEnabled() && !self.tableView.editing) {
+    item.textFieldTextColor = [UIColor colorNamed:kTextSecondaryColor];
+  }
   return item;
 }
 
@@ -265,7 +272,7 @@ - (TableViewTextEditItem*)passwordItemForPasswordDetails:
                             ? passwordDetails.password
                             : kMaskedPassword;
   item.textFieldEnabled = self.tableView.editing;
-  item.hideIcon = !self.tableView.editing;
+  item.hideIcon = !self.tableView.editing || IsPasswordGroupingEnabled();
   item.autoCapitalizationType = UITextAutocapitalizationTypeNone;
   item.keyboardType = UIKeyboardTypeURL;
   item.returnKeyType = UIReturnKeyDone;
@@ -293,6 +300,9 @@ - (TableViewTextEditItem*)passwordItemForPasswordDetails:
         [self isPasswordShown] ? IDS_IOS_SETTINGS_PASSWORD_HIDE_BUTTON
                                : IDS_IOS_SETTINGS_PASSWORD_SHOW_BUTTON);
   }
+  if (IsPasswordGroupingEnabled() && !self.tableView.editing) {
+    item.textFieldTextColor = [UIColor colorNamed:kTextSecondaryColor];
+  }
   return item;
 }
 
@@ -336,7 +346,9 @@ - (TableViewTextButtonItem*)deleteButtonItemForPasswordDetails:
     (PasswordDetails*)passwordDetails {
   TableViewTextButtonItem* item = [[TableViewTextButtonItem alloc]
       initWithType:PasswordDetailsItemTypeDeleteButton];
-  item.buttonText = l10n_util::GetNSString(IDS_IOS_SETTINGS_TOOLBAR_DELETE);
+  item.buttonText = l10n_util::GetNSString(IDS_IOS_CONFIRM_PASSWORD_DELETION);
+  item.buttonContentHorizontalAlignment =
+      UIControlContentHorizontalAlignmentLeft;
   item.boldButtonText = NO;
   item.disableButtonIntrinsicWidth = YES;
   item.buttonTextColor = [UIColor colorNamed:kRedColor];

diff --git a/ios/chrome/browser/ui/settings/password/password_manager_egtest.mm b/ios/chrome/browser/ui/settings/password/password_manager_egtest.mm
@@ -213,13 +213,23 @@
       nullptr);
 }
 
-// Matcher for the Delete button in Confirmation Alert for password deletion.
+// TODO(crbug.com/1359392): Remove this override when kPasswordsGrouping flag is
+// removed. Matcher for the Delete button in Confirmation Alert for password
+// deletion.
 id<GREYMatcher> DeleteConfirmationButton() {
   return grey_allOf(ButtonWithAccessibilityLabel(l10n_util::GetNSString(
                         IDS_IOS_CONFIRM_PASSWORD_DELETION)),
                     grey_interactable(), nullptr);
 }
 
+// Matcher for the Delete button in Confirmation Alert for password deletion
+// when password grouping is enabled.
+id<GREYMatcher> DeleteConfirmationButtonForGrouping() {
+  return grey_allOf(ButtonWithAccessibilityLabel(
+                        l10n_util::GetNSString(IDS_IOS_DELETE_ACTION_TITLE)),
+                    grey_interactable(), nullptr);
+}
+
 // Matcher for the Delete button in the list view, located at the bottom of the
 // screen.
 id<GREYMatcher> DeleteButtonAtBottom() {
@@ -764,9 +774,17 @@ - (void)testSavedFormDeletionInDetailView {
   [[EarlGrey selectElementWithMatcher:NavigationBarEditButton()]
       performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteButton()] performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:[self groupingEnabled]
+                                          ? DeleteButtonForUsernameAndPassword(
+                                                @"concrete username",
+                                                @"concrete password")
+                                          : DeleteButton()]
+      performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButton()]
+  [[EarlGrey
+      selectElementWithMatcher:[self groupingEnabled]
+                                   ? DeleteConfirmationButtonForGrouping()
+                                   : DeleteConfirmationButton()]
       performAction:grey_tap()];
 
   // Wait until the alert and the detail view are dismissed.
@@ -821,9 +839,17 @@ - (void)testSavedFormDeletionInDetailViewWithBlockedSites {
   [[EarlGrey selectElementWithMatcher:NavigationBarEditButton()]
       performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteButton()] performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:[self groupingEnabled]
+                                          ? DeleteButtonForUsernameAndPassword(
+                                                @"concrete username",
+                                                @"concrete password")
+                                          : DeleteButton()]
+      performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButton()]
+  [[EarlGrey
+      selectElementWithMatcher:[self groupingEnabled]
+                                   ? DeleteConfirmationButtonForGrouping()
+                                   : DeleteConfirmationButton()]
       performAction:grey_tap()];
 
   // Wait until the alert and the detail view are dismissed.
@@ -1057,7 +1083,12 @@ - (void)testCancelDeletionInDetailView {
   [[EarlGrey selectElementWithMatcher:NavigationBarEditButton()]
       performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteButton()] performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:[self groupingEnabled]
+                                          ? DeleteButtonForUsernameAndPassword(
+                                                @"concrete username",
+                                                @"concrete password")
+                                          : DeleteButton()]
+      performAction:grey_tap()];
 
   // Close the dialog by taping on Password Details screen.
   [[EarlGrey selectElementWithMatcher:grey_accessibilityID(
@@ -2666,7 +2697,7 @@ - (void)testPasswordsDeletionNavigation {
                                           @"user1", @"password1")]
       performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButton()]
+  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButtonForGrouping()]
       performAction:grey_tap()];
 
   // Check that the current view is still the password details since there is
@@ -2689,7 +2720,7 @@ - (void)testPasswordsDeletionNavigation {
                                           @"user2", @"password2")]
       performAction:grey_tap()];
 
-  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButton()]
+  [[EarlGrey selectElementWithMatcher:DeleteConfirmationButtonForGrouping()]
       performAction:grey_tap()];
 
   // Check that the current view is now the password manager since we deleted

diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.h b/ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.h
@@ -18,6 +18,10 @@
 // Text being displayed above the button alignment.
 @property(nonatomic, readwrite, assign) NSTextAlignment textAlignment;
 
+// Button text alignment.
+@property(nonatomic, readwrite, assign)
+    UIControlContentHorizontalAlignment buttonContentHorizontalAlignment;
+
 // Text for cell button.
 @property(nonatomic, readwrite, strong) NSString* buttonText;
 

diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.mm b/ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.mm
@@ -34,6 +34,9 @@
 const CGFloat kButtonTitleFontSize = 17.0;
 // Default Text alignment.
 const NSTextAlignment kDefaultTextAlignment = NSTextAlignmentCenter;
+// Default Text alignment.
+const UIControlContentHorizontalAlignment kDefaultContentHorizontalAlignment =
+    UIControlContentHorizontalAlignmentCenter;
 }  // namespace
 
 @implementation TableViewTextButtonItem
@@ -48,6 +51,7 @@ - (instancetype)initWithType:(NSInteger)type {
     self.cellClass = [TableViewTextButtonCell class];
     _enabled = YES;
     _textAlignment = kDefaultTextAlignment;
+    _buttonContentHorizontalAlignment = kDefaultContentHorizontalAlignment;
     _boldButtonText = YES;
     _dimBackgroundWhenDisabled = YES;
   }
@@ -86,6 +90,14 @@ - (void)configureCell:(TableViewCell*)tableCell
     [cell.button setTitleColor:[UIColor colorNamed:kSolidButtonTextColor]
                       forState:UIControlStateNormal];
   }
+  cell.button.contentHorizontalAlignment =
+      self.buttonContentHorizontalAlignment;
+  if (self.buttonContentHorizontalAlignment ==
+      UIControlContentHorizontalAlignmentLeft) {
+    cell.button.contentEdgeInsets = UIEdgeInsetsMake(
+        kButtonTitleVerticalContentInset, 0, kButtonTitleVerticalContentInset,
+        kButtonTitleHorizontalContentInset);
+  }
   cell.button.accessibilityIdentifier = self.buttonAccessibilityIdentifier;
   // Decide cell.button.backgroundColor in order:
   //   1. self.buttonBackgroundColor
@@ -210,6 +222,7 @@ - (void)prepareForReuse {
   [super prepareForReuse];
   [self.button setTitleColor:[UIColor colorNamed:kSolidButtonTextColor]
                     forState:UIControlStateNormal];
+  self.button.contentHorizontalAlignment = kDefaultContentHorizontalAlignment;
   self.textLabel.textAlignment = kDefaultTextAlignment;
   [self disableButtonIntrinsicWidth:NO];
 }

diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_text_edit_item.h b/ios/chrome/browser/ui/table_view/cells/table_view_text_edit_item.h
@@ -35,6 +35,10 @@ typedef NS_ENUM(NSInteger, TableViewTextEditItemIconType) {
 // The value of the text field.
 @property(nonatomic, copy) NSString* textFieldValue;
 
+// Text color for the text field of the cell. Default is [UIColor
+// colorNamed:kTextPrimaryColor].
+@property(nonatomic, strong) UIColor* textFieldTextColor;
+
 // An icon identifying the text field or its current value, if any.
 @property(nonatomic, copy) UIImage* identifyingIcon;
 

diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_text_edit_item.mm b/ios/chrome/browser/ui/table_view/cells/table_view_text_edit_item.mm
@@ -91,7 +91,9 @@ - (void)configureCell:(TableViewTextEditCell*)cell
   cell.textField.enabled = self.textFieldEnabled;
 
   if (self.hideIcon) {
-    cell.textField.textColor = [UIColor colorNamed:kTextPrimaryColor];
+    cell.textField.textColor = self.textFieldTextColor
+                                   ? self.textFieldTextColor
+                                   : [UIColor colorNamed:kTextPrimaryColor];
 
     [cell setIcon:TableViewTextEditItemIconTypeNone];
   } else {