Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2719023005:
Provide Copy button for username in password settings (Closed)
|
Created:
3 years, 9 months ago by vabr (Chromium) Modified:
3 years, 7 months ago Reviewers:
lpromero CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, gcasto+watchlist_chromium.org, srahim+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide Copy button for username in password settings
Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38
BUG=159166
Review-Url: https://codereview.chromium.org/2719023005
Cr-Commit-Position: refs/heads/master@{#468919}
Committed: https://chromium.googlesource.com/chromium/src/+/1c49164e024e59ded9ce1bcb84f376d4b47bdd93
Patch Set 1 #Patch Set 2 : Rebase and add EG test for the toast #
Total comments: 8
Patch Set 3 : Use unique accessibilityLabels rebase #Patch Set 4 : Improve some comments #
Total comments: 2
Patch Set 5 : Fix format string #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 25 (15 generated)
Description was changed from ========== [WIP] Provide Copy button for username in password settings Design doc: go/jgdrc TODO: Tests to be done. BUG=159166 ========== to ========== [WIP] Provide Copy button for username in password settings Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38 TODO: Tests to be done. BUG=159166 ==========
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Description was changed from ========== [WIP] Provide Copy button for username in password settings Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38 TODO: Tests to be done. BUG=159166 ========== to ========== Provide Copy button for username in password settings Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38 BUG=159166 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vabr@chromium.org changed reviewers: + lpromero@chromium.org
Hi lpromero@, Could you please review this? Screenshot link is in the summary, and pschaffner@ is OK with this change. I have two more CLs in this area which I will send to you, sorry about the avalanche. These CLs are not urgent, please feel free to throttle reviewing them as you see fit. :) Cheers, Vaclav https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm:74: int kCopyUsernameButtonItem = 1; I just noticed that this all should be const, given the naming. I fixed that in the follow-up CL https://codereview.chromium.org/2722853003/, so would suggest to leave it here as is already.
lgtm https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:172: item.text = l10n_util::GetNSString(IDS_IOS_SETTINGS_USERNAME_COPY_BUTTON); This might not be sufficient for accessibility. I suggest you set the accessibilityLabel to a concatenation of "Copy" and "Username", or make an entirely new string. https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:80: // to express its relative vertical position to the "Password" header. You could also use accessibility identifiers (accessibilityIdentifier) that you would set on the items. These are not user facing, only for testing. (Or you could also use the accessibility labels per previous comment, but accessibilityIdentifier-s are better in my opinion). https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:100: grey_layout(@[ above ], PasswordHeader()), nullptr); I didn't know about this feature, that's a nice one!
Thanks for the review! I actually have some questions about the accessibility below. Cheers, Vaclav https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:172: item.text = l10n_util::GetNSString(IDS_IOS_SETTINGS_USERNAME_COPY_BUTTON); On 2017/05/02 15:04:52, lpromero wrote: > This might not be sufficient for accessibility. I suggest you set the > accessibilityLabel to a concatenation of "Copy" and "Username", or make an > entirely new string. Oh, so if I set accessibilityLabel here to "Copy Username", that will override the creation of the accessibility label from text? When looking at https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/ce... it seemed to me that there is no way to decouple the text from the acecssibility label. My issue with changing the text would be that UI-wise it makes sense not to repeat "Username" if that's the header of the section. And given that EGTest can express the correct "Copy" matcher relative to the headers, I was hoping VoiceOver could also be helpful in this respect, in that the user would hear, e.g., "Username", <username itself>, "Copy", when progressing through the UI. Would that not work for accessibility? https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:80: // to express its relative vertical position to the "Password" header. On 2017/05/02 15:04:52, lpromero wrote: > You could also use accessibility identifiers (accessibilityIdentifier) that you > would set on the items. These are not user facing, only for testing. (Or you > could also use the accessibility labels per previous comment, but > accessibilityIdentifier-s are better in my opinion). My understanding was that if I could not write a reasonable EG test, then the accessibility of the UI is broken. In that case relying on user-invisible distinguishing features seems not helpful to the actual accessibility users. This position-based checking is a little verbose, but seems to make sense to me, as long as VoiceOver processes the UI top-down, and not in the crazy order the cells are actually inserted (what EGTest logs on error seems to have a random order). https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:100: grey_layout(@[ above ], PasswordHeader()), nullptr); On 2017/05/02 15:04:52, lpromero wrote: > I didn't know about this feature, that's a nice one! I had a lot of educational time in front of the API docs yesterday :). It is nice, assuming that the visually impaired user can actually process the UI in a similar way.
https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm (right): https://codereview.chromium.org/2719023005/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:172: item.text = l10n_util::GetNSString(IDS_IOS_SETTINGS_USERNAME_COPY_BUTTON); On 2017/05/02 15:29:09, vabr (Chromium) wrote: > On 2017/05/02 15:04:52, lpromero wrote: > > This might not be sufficient for accessibility. I suggest you set the > > accessibilityLabel to a concatenation of "Copy" and "Username", or make an > > entirely new string. > > Oh, so if I set accessibilityLabel here to "Copy Username", that will override > the creation of the accessibility label from text? When looking at > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/ce... > it seemed to me that there is no way to decouple the text from the acecssibility > label. > Indeed. This is an issue. The collection view text item should first see if there is an accessibility label, and only fallback to the code you linked. Can you file a bug and maybe send a CL to fix it? > My issue with changing the text would be that UI-wise it makes sense not to > repeat "Username" if that's the header of the section. The problem is that this namespacing is lost when a VoiceOver user is focused on "Copy". The user could have come from the bottom, or from the top, but also by moving their finger on the screen. Visually, having only "Copy" is fine, but not for visually impaired users. > And given that EGTest can express the correct "Copy" matcher relative to the > headers, I was hoping VoiceOver could also be helpful in this respect, in that > the user would hear, e.g., "Username", <username itself>, "Copy", when > progressing through the UI. Would that not work for accessibility? As explained above, it's not evidence that users will parse the UI in this order. But no need to change the EG tests, they are fine as is.
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review and helpful chat about the labels. I sent you also https://codereview.chromium.org/2856113002/ with the requested fix to the accessibilityLabel copying. Once you approve that, I will start landing the sequence you approved yesterday. Here I started using the separate accessibilityLabel, and still left the matchers for layout, to verify that the accessibility frames make sense. Thanks! Vaclav
Thank you for the fixes! lgtm https://codereview.chromium.org/2719023005/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm (right): https://codereview.chromium.org/2719023005/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:178: [NSString stringWithFormat:@"%@: :%@", Shouldn't it be @"%@: %@", or @"%@, %@"? The format string seems to have an issue. Idem below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2719023005/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm (right): https://codereview.chromium.org/2719023005/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:178: [NSString stringWithFormat:@"%@: :%@", On 2017/05/03 08:19:39, lpromero wrote: > Shouldn't it be @"%@: %@", or @"%@, %@"? > The format string seems to have an issue. Idem below. Well spotted, this was a typo. I removed the second colon now.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2719023005/#ps80001 (title: "Fix format string")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493800511693950,
"parent_rev": "887ed4b568cf6c67608c9838fef4be47fb962fd1", "commit_rev":
"1c49164e024e59ded9ce1bcb84f376d4b47bdd93"}
Message was sent while issue was closed.
Description was changed from ========== Provide Copy button for username in password settings Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38 BUG=159166 ========== to ========== Provide Copy button for username in password settings Design doc: go/jgdrc, screenshots https://crbug.com/159166#c38 BUG=159166 Review-Url: https://codereview.chromium.org/2719023005 Cr-Commit-Position: refs/heads/master@{#468919} Committed: https://chromium.googlesource.com/chromium/src/+/1c49164e024e59ded9ce1bcb84f3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1c49164e024e59ded9ce1bcb84f3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
