Allow showing and copying of blacklisted credential origin in iOS passwords settings |
|||||||||
Issue description
Part of the support for viewing credentials on iOS (bug 739404) should be also viewing and copying the full origin of blacklisted credentials.
Blacklisted credentials have much less data than normal ones: just the origin
of the credential. Nevertheless, the origin in the list of all passwords cannot be shown
completely for space constraints and cannot be copied either, making it
inaccessible to the user if they need to check where they disabled saving the
credential.
One way to fix that would be to add a context menu button in the list to copy
the full URL. However, that would have these drawbacks:
(1) Still no easy way to show the full origin.
(2) Confusing difference between the presented string (which is not the full
URL) and the copied result.
(3) Inconsistency against what is done for the non-blacklisted credentials.
Another way is to expand the password detail view to
be able to display the blacklisted credentials meaningfully. This in fact means
just allowing the blacklisted credentials to be displayed there, and
suppressing all parts of the detail view except for the Site section
and the delete button.
Attached are screenshots for the latter.
,
Jul 9 2017
,
Jul 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b commit b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b Author: Vaclav Brozek <vabr@chromium.org> Date: Mon Jul 10 12:47:28 2017 Expand password detail view in iOS settings to blacklisted credentials Currently, only non-blacklisted credentials can be viewed in detail in password settings. Blacklisted credentials have much less data to show: just the origin of the credential. Nevertheless, the origin in the list cannot be shown completely for space constraints and cannot be copied either, making it inaccessible to the user if they need to check where they disabled saving the credential. One way to fix that would be to add a context menu button in the list to copy the full URL. However, that would have these drawbacks: (1) Still no easy way to show the full origin. (2) Confusing difference between the presented string (which is not the full URL) and the copied result. (3) Inconsistency against what is done for the non-blacklisted credentials. Another way, implemented in this CL, is to expand the password detail view to be able to display the blacklisted credentials meaningfully. This in fact means just allowing the blacklisted credentials to be displayed there, and suppressing all parts of the detail view except for the Site section and the delete button. The CL also simplifies the initialization of PasswordDetailsCollectionViewController by removing arguments derivable from the passed PasswordForm. Further, it fixes an unrealistic unittest by ensuring that the PasswordForm passed to the details controller on initialization always has a well-defined origin URL. Screenshots are at https://crbug.com/740379 . Bug: 740379 Change-Id: I9d8bf7c03b3f36c3df65ebf5645fc193679ad54b Reviewed-on: https://chromium-review.googlesource.com/563665 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#485244} [modify] https://crrev.com/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b/ios/chrome/browser/ui/settings/password_details_collection_view_controller.h [modify] https://crrev.com/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b/ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm [modify] https://crrev.com/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm [modify] https://crrev.com/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm [modify] https://crrev.com/b98c4727fcc0abfe5b319d6baf9d6e02ef477e6b/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm
,
Jul 10 2017
This solutions sounds good to me. Thank you!
,
Jul 10 2017
,
Jul 10 2017
Thanks! Also here, I'm marking as fixed for now, but happy to reopen and address additional comments.
,
Jul 11 2017
+Shimi: I'm a little worried about the delete button string used in the detail view (see detail.png in #0), as what we are deleting isn't really a "Saved Password", but rather a blacklisted item (as the bug's name suggests). Do you think just saying "Delete" would be OK? If so, would you change said string in the other "Saved Password" item scenarios? FWIW, I opted to use a direct object in said button to make sure the scope of operation was completely clear (the detached cell style of the button also helps to communicate the operation acts upon the item and not upon a piece of metadata).
,
Jul 11 2017
Thanks for pointing the issue with the delete button out. I'm reopening and will wait for Shimi's input.
,
Jul 18 2017
When the "Delete Saved Password" button is pressed, does the entire item disappear from the list? If so, "Delete" seems fine.
,
Jul 19 2017
Correct, the entire item disappears from the list (and the view changes from the detail view to the list view). Thanks for confirming, I will change the text from "Delete Saved Password" to "Delete" shortly.
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/574f0bf33a61976d995c14e979d295f3bea8bb39 commit 574f0bf33a61976d995c14e979d295f3bea8bb39 Author: Vaclav Brozek <vabr@chromium.org> Date: Wed Jul 19 10:00:48 2017 Adjust strings in passwords settings on iOS This CL adjusts strings used in the passwords detail view, based on the following guidance from UX: * https://crbug.com/740379#c9 Shortening "Delete Saved Password" to just "Delete" * https://crbug.com/740384#c11 Identifying the federation URL with "Signed In With" * https://crbug.com/742290#c1 Improving the copy toasts: URL -> address, simpler phrasing and no full-stops for one-sentence messages Bug: 740379 , 740384 , 742290 Change-Id: I1eec2a44fd5d2acd59166988431832e88f8737e6 Reviewed-on: https://chromium-review.googlesource.com/577528 Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#487810} [modify] https://crrev.com/574f0bf33a61976d995c14e979d295f3bea8bb39/ios/chrome/app/strings/ios_strings.grd
,
Jul 19 2017
,
Sep 22 2017
Verified (#3) on iOS 10.3.3, 11.0 on iPhone 6 +, iPhone 7+, iPad Pro 12'5 on build 62.0.3202.29 Beta. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by vabr@chromium.org
, Jul 9 2017