[iOS] Make password picker have single selection
* Updates password picker to new mocks to allow selecting only one
credential
* Next button is enabled by default with the first row selected by
default
Bug: 1463882
Change-Id: I625f3a3b8a09655dea92010f55201e156105535e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4951733
Commit-Queue: Rafał Godlewski <rgod@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Ernesto Izquierdo Clua <eic@google.com>
Cr-Commit-Position: refs/heads/main@{#1215110}
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator.mm
index 9fc60d9f..523a49c 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator.mm
@@ -98,10 +98,9 @@
}
- (void)passwordPickerClosed:(PasswordPickerViewController*)controller
- withSelectedCredentials:
- (const std::vector<password_manager::CredentialUIEntry>&)credentials {
- [self.delegate passwordPickerCoordinator:self
- didSelectCredentials:credentials];
+ withSelectedCredential:
+ (const password_manager::CredentialUIEntry&)credential {
+ [self.delegate passwordPickerCoordinator:self didSelectCredential:credential];
}
@end
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator_delegate.h b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator_delegate.h
index 5958ea28..c3fba54 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator_delegate.h
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_coordinator_delegate.h
@@ -20,9 +20,8 @@
// Called when the user confirms the selection by clicking the next button.
- (void)passwordPickerCoordinator:(PasswordPickerCoordinator*)coordinator
- didSelectCredentials:
- (const std::vector<password_manager::CredentialUIEntry>&)
- credentials;
+ didSelectCredential:
+ (const password_manager::CredentialUIEntry&)credential;
@end
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller.mm
index 698424e..b02e46a 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller.mm
@@ -8,7 +8,6 @@
#import "components/password_manager/core/browser/password_ui_utils.h"
#import "components/password_manager/core/browser/ui/credential_ui_entry.h"
#import "ios/chrome/browser/net/crurl.h"
-#import "ios/chrome/browser/shared/ui/symbols/symbols.h"
#import "ios/chrome/browser/shared/ui/table_view/cells/table_view_url_item.h"
#import "ios/chrome/browser/shared/ui/table_view/table_view_favicon_data_source.h"
#import "ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_presentation_delegate.h"
@@ -27,9 +26,6 @@
ItemTypeCredential = kItemTypeEnumZero,
};
-// Size of the accessory view symbol.
-const CGFloat kAccessorySymbolSize = 22;
-
} // namespace
@interface PasswordPickerViewController () {
@@ -59,13 +55,10 @@
style:UIBarButtonItemStylePlain
target:self
action:@selector(nextButtonTapped)];
- nextButton.enabled = NO;
self.navigationItem.rightBarButtonItem = nextButton;
self.navigationItem.rightBarButtonItem.accessibilityIdentifier =
kPasswordPickerNextButtonId;
- self.tableView.allowsMultipleSelection = YES;
-
[self loadModel];
}
@@ -80,20 +73,28 @@
}
}
+- (void)viewDidAppear:(BOOL)animated {
+ [super viewDidAppear:animated];
+
+ // Select first row by default.
+ NSIndexPath* indexPath = [NSIndexPath indexPathForRow:0 inSection:0];
+ [self.tableView selectRowAtIndexPath:indexPath
+ animated:NO
+ scrollPosition:UITableViewScrollPositionNone];
+}
+
#pragma mark - UITableViewDelegate
- (void)tableView:(UITableView*)tableView
didSelectRowAtIndexPath:(NSIndexPath*)indexPath {
- [tableView cellForRowAtIndexPath:indexPath].accessoryView =
- [[UIImageView alloc] initWithImage:[self checkmarkCircleIcon]];
- [self setNextButtonStatus];
+ [tableView cellForRowAtIndexPath:indexPath].accessoryType =
+ UITableViewCellAccessoryCheckmark;
}
- (void)tableView:(UITableView*)tableView
didDeselectRowAtIndexPath:(NSIndexPath*)indexPath {
- [tableView cellForRowAtIndexPath:indexPath].accessoryView =
- [[UIImageView alloc] initWithImage:[self circleIcon]];
- [self setNextButtonStatus];
+ [tableView cellForRowAtIndexPath:indexPath].accessoryType =
+ UITableViewCellAccessoryNone;
}
#pragma mark - UITableViewDataSource
@@ -115,9 +116,11 @@
cell.userInteractionEnabled = YES;
cell.textLabel.numberOfLines = 1;
cell.detailTextLabel.numberOfLines = 1;
- cell.accessoryView = [[UIImageView alloc]
- initWithImage:cell.isSelected ? [self checkmarkCircleIcon]
- : [self circleIcon]];
+ if (indexPath.row == tableView.indexPathForSelectedRow.row) {
+ cell.accessoryType = UITableViewCellAccessoryCheckmark;
+ } else {
+ cell.accessoryType = UITableViewCellAccessoryNone;
+ }
TableViewItem* item = [self.tableViewModel itemAtIndexPath:indexPath];
TableViewURLItem* URLItem =
@@ -156,32 +159,15 @@
#pragma mark - Private
-- (UIImage*)checkmarkCircleIcon {
- return DefaultSymbolWithPointSize(kCheckmarkCircleFillSymbol,
- kAccessorySymbolSize);
-}
-
-- (UIImage*)circleIcon {
- return DefaultSymbolWithPointSize(kCircleSymbol, kAccessorySymbolSize);
-}
-
- (void)cancelButtonTapped {
[self.delegate passwordPickerWasDismissed:self];
}
- (void)nextButtonTapped {
- std::vector<password_manager::CredentialUIEntry> selectedCredentials;
- for (NSIndexPath* indexPath in self.tableView.indexPathsForSelectedRows) {
- selectedCredentials.push_back(_credentials[indexPath.row]);
- }
- [self.delegate passwordPickerClosed:self
- withSelectedCredentials:selectedCredentials];
-}
-
-// Enables next button if any row is selected or disables it otherwise.
-- (void)setNextButtonStatus {
- self.navigationItem.rightBarButtonItem.enabled =
- self.tableView.indexPathsForSelectedRows.count > 0;
+ [self.delegate
+ passwordPickerClosed:self
+ withSelectedCredential:_credentials[self.tableView.indexPathForSelectedRow
+ .row]];
}
@end
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_presentation_delegate.h b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_presentation_delegate.h
index e108d08..3f6887b 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_presentation_delegate.h
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_presentation_delegate.h
@@ -17,10 +17,10 @@
// Called when the user clicks cancel button or dismisses the view by swiping.
- (void)passwordPickerWasDismissed:(PasswordPickerViewController*)controller;
-// Called when the user clicks next button with selected credentials.
+// Called when the user clicks next button with selected credential.
- (void)passwordPickerClosed:(PasswordPickerViewController*)controller
- withSelectedCredentials:
- (const std::vector<password_manager::CredentialUIEntry>&)credentials;
+ withSelectedCredential:
+ (const password_manager::CredentialUIEntry&)credential;
@end
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_unittest.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_unittest.mm
index 29d512c..33c9a00 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_unittest.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_picker_view_controller_unittest.mm
@@ -44,6 +44,16 @@
static_cast<PasswordPickerViewController*>(controller());
[password_picker_controller setCredentials:credentials];
}
+
+ void CheckCellAccessoryType(UITableViewCellAccessoryType accessoryType,
+ int section,
+ int row) {
+ UITableViewCell* cell =
+ [controller() tableView:controller().tableView
+ cellForRowAtIndexPath:[NSIndexPath indexPathForRow:row
+ inSection:section]];
+ EXPECT_EQ(cell.accessoryType, accessoryType);
+ }
};
TEST_F(PasswordPickerViewControllerTest, TestPasswordPickerLayout) {
@@ -62,3 +72,45 @@
base::SysUTF8ToNSString(item.URL.gurl.host()));
}
}
+
+TEST_F(PasswordPickerViewControllerTest, TestNextButtonEnabledByDefault) {
+ SetCredentials(/*amount=*/3);
+
+ EXPECT_EQ(NumberOfSections(), 1);
+ EXPECT_EQ(NumberOfItemsInSection(0), 3);
+
+ PasswordPickerViewController* passwordPickerController =
+ base::apple::ObjCCastStrict<PasswordPickerViewController>(controller());
+ EXPECT_TRUE(
+ passwordPickerController.navigationItem.rightBarButtonItem.isEnabled);
+}
+
+TEST_F(PasswordPickerViewControllerTest, TestSettingAccessoryType) {
+ SetCredentials(/*amount=*/4);
+
+ EXPECT_EQ(NumberOfSections(), 1);
+ EXPECT_EQ(NumberOfItemsInSection(0), 4);
+
+ // Check that the first row has a checkmark by default.
+ CheckCellAccessoryType(UITableViewCellAccessoryCheckmark, 0, 0);
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 1);
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 2);
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 3);
+
+ // Select last row.
+ PasswordPickerViewController* passwordPickerController =
+ base::apple::ObjCCastStrict<PasswordPickerViewController>(controller());
+ NSIndexPath* indexPath = [NSIndexPath indexPathForRow:3 inSection:0];
+ [passwordPickerController.tableView
+ selectRowAtIndexPath:indexPath
+ animated:NO
+ scrollPosition:UITableViewScrollPositionNone];
+ [passwordPickerController tableView:passwordPickerController.tableView
+ didSelectRowAtIndexPath:indexPath];
+
+ // Check that the last row has a checkmark.
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 0);
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 1);
+ CheckCellAccessoryType(UITableViewCellAccessoryNone, 0, 2);
+ CheckCellAccessoryType(UITableViewCellAccessoryCheckmark, 0, 3);
+}
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_coordinator.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_coordinator.mm
index 7d3b6b5..ff71c4fd 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_coordinator.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_coordinator.mm
@@ -131,9 +131,9 @@
GetForBrowserState(browserState)];
// With more than 1 credential an additional UI will be presented to select
- // which ones should be shared.
+ // which one should be shared.
if (_credentials.size() == 1) {
- self.mediator.selectedCredentials = _credentials;
+ self.mediator.selectedCredential = _credentials[0];
}
}
@@ -197,10 +197,9 @@
}
- (void)passwordPickerCoordinator:(PasswordPickerCoordinator*)coordinator
- didSelectCredentials:
- (const std::vector<password_manager::CredentialUIEntry>&)
- credentials {
- self.mediator.selectedCredentials = credentials;
+ didSelectCredential:
+ (const password_manager::CredentialUIEntry&)credential {
+ self.mediator.selectedCredential = credential;
[self startFamilyPickerCoordinator];
}
@@ -249,7 +248,7 @@
}
- (void)startPasswordSharing {
- [self.mediator sendSelectedPasswordsToSelectedRecipients];
+ [self.mediator sendSelectedCredentialToSelectedRecipients];
}
#pragma mark - Private
@@ -287,10 +286,8 @@
- (void)startSharingStatusCoordinator {
[self.sharingStatusCoordinator stop];
- // TODO(crbug.com/1463882): Remove multiple credential selection, before it's
- // done use the first one.
password_manager::CredentialUIEntry credential =
- self.mediator.selectedCredentials[0];
+ self.mediator.selectedCredential;
self.sharingStatusCoordinator = [[SharingStatusCoordinator alloc]
initWithBaseViewController:self.navigationController
browser:self.browser
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_egtest.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_egtest.mm
index 5e63216..71f0856 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_egtest.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_egtest.mm
@@ -372,19 +372,16 @@
selectElementWithMatcher:grey_accessibilityID(kPasswordShareButtonId)]
performAction:grey_tap()];
- // Check that the next button is not visible without any rows selected.
- [[EarlGrey selectElementWithMatcher:grey_accessibilityID(
- kPasswordPickerNextButtonId)]
- assertWithMatcher:grey_not(grey_enabled())];
-
- // Select first row and click "Next".
- [[EarlGrey
- selectElementWithMatcher:grey_allOf(grey_accessibilityID(@"username1"),
- grey_sufficientlyVisible(), nil)]
- performAction:grey_tap()];
+ // Check that the next button is enabled by default.
[[EarlGrey selectElementWithMatcher:grey_accessibilityID(
kPasswordPickerNextButtonId)]
assertWithMatcher:grey_enabled()];
+
+ // Select second row and click "Next".
+ [[EarlGrey
+ selectElementWithMatcher:grey_allOf(grey_accessibilityID(@"username2"),
+ grey_sufficientlyVisible(), nil)]
+ performAction:grey_tap()];
[[EarlGrey selectElementWithMatcher:grey_accessibilityID(
kPasswordPickerNextButtonId)]
performAction:grey_tap()];
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.h b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.h
index cb89b46..e897c063b 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.h
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.h
@@ -45,14 +45,14 @@
- (instancetype)init NS_UNAVAILABLE;
-// Fetches corresponding password forms for all `selectedCredentials` and
-// invokes SendPasswords method of PasswordSenderService with all forms for each
-// of the `selectedRecipients`.
-- (void)sendSelectedPasswordsToSelectedRecipients;
+// Fetches corresponding password forms for `selectedCredential` and invokes
+// SendPasswords method of PasswordSenderService with all forms for each of the
+// `selectedRecipients`.
+- (void)sendSelectedCredentialToSelectedRecipients;
-// Credentials selected by the user to be shared.
-@property(nonatomic, assign) std::vector<password_manager::CredentialUIEntry>
- selectedCredentials;
+// Credential selected by the user to be shared.
+@property(nonatomic, assign)
+ password_manager::CredentialUIEntry selectedCredential;
// Recipients selected by the user to receive the shared passwords.
@property(nonatomic, strong)
diff --git a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.mm b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.mm
index 455f868..5b23639 100644
--- a/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.mm
+++ b/ios/chrome/browser/ui/settings/password/password_sharing/password_sharing_mediator.mm
@@ -82,15 +82,10 @@
return self;
}
-- (void)sendSelectedPasswordsToSelectedRecipients {
- std::vector<password_manager::PasswordForm> passwords;
- for (const password_manager::CredentialUIEntry& credential :
- self.selectedCredentials) {
- std::vector<password_manager::PasswordForm> credential_forms =
- _savedPasswordsPresenter->GetCorrespondingPasswordForms(credential);
- passwords.insert(passwords.end(), credential_forms.begin(),
- credential_forms.end());
- }
+- (void)sendSelectedCredentialToSelectedRecipients {
+ std::vector<password_manager::PasswordForm> passwords =
+ _savedPasswordsPresenter->GetCorrespondingPasswordForms(
+ self.selectedCredential);
for (RecipientInfoForIOSDisplay* recipient in self.selectedRecipients) {
_passwordSenderService->SendPasswords(
passwords, {.user_id = base::SysNSStringToUTF8(recipient.userID),
