New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740379 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
hobby only
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Feature

Blocking:
issue 739404



Sign in to add a comment

Allow showing and copying of blacklisted credential origin in iOS passwords settings

Project Member Reported by vabr@chromium.org, Jul 9 2017

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.
 
detail.png
99 KB View Download
list.png
168 KB View Download

Comment 1 by vabr@chromium.org, Jul 9 2017

The CL from which the screenshots are taken is at https://chromium-review.googlesource.com/c/563665.

Comment 2 by vabr@chromium.org, Jul 9 2017

Description: Show this description
Project Member

Comment 3 by bugdroid1@chromium.org, 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

This solutions sounds good to me. Thank you!
Cc: pschaffner@chromium.org pinkerton@chromium.org mard...@chromium.org

Comment 6 by vabr@chromium.org, Jul 10 2017

Status: Fixed (was: Started)
Thanks! Also here, I'm marking as fixed for now, but happy to reopen and address additional comments.
Cc: srahim@chromium.org
+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).

Comment 8 by vabr@chromium.org, Jul 11 2017

Status: Assigned (was: Fixed)
Thanks for pointing the issue with the delete button out. I'm reopening and will wait for Shimi's input.

Comment 9 by srahim@chromium.org, Jul 18 2017

When the "Delete Saved Password" button is pressed, does the entire item disappear from the list? If so, "Delete" seems fine. 

Comment 10 by vabr@chromium.org, Jul 19 2017

Status: Started (was: Assigned)
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.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Comment 12 by vabr@chromium.org, Jul 19 2017

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
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