Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(273)

Issue 2441043004: Accessibility fixes for quick unlock settings. (Closed)

Created:
4 years, 2 months ago by jdufault
Modified:
4 years, 1 month ago
Reviewers:
tommycli, Dan Beam
CC:
chromium-reviews, dbeam+watch-elements_chromium.org, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Accessibility fixes for quick unlock settings. This also changes the warning/error behavior when setting up a PIN so it is less aggressive. BUG=606516, 641184 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/715df8e71fe09d4aed71eca4dec20faa4c2e6c5a Cr-Commit-Position: refs/heads/master@{#431942}

Patch Set 1 : Initial patch #

Total comments: 13

Patch Set 2 : Rebase #

Patch Set 3 : Address comments, fix test #

Total comments: 4

Patch Set 4 : Address comments #

Total comments: 4

Patch Set 5 : Fix tests #

Patch Set 6 : Update comment #

Total comments: 10

Patch Set 7 : Address comments #

Total comments: 6

Patch Set 8 : Address comments #

Total comments: 14

Patch Set 9 : Address comments #

Total comments: 2

Patch Set 10 : Rebase #

Patch Set 11 : Use aria-label instead of alt #

Messages

Total messages: 74 (50 generated)
jdufault
tommycli@ PTAL when you get a chance - thanks!
4 years, 1 month ago (2016-10-24 21:50:34 UTC) #18
tommycli
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js#newcode91 chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); Two questions: 1. Why doesn't autofocus attribute work ...
4 years, 1 month ago (2016-10-25 18:13:50 UTC) #19
tommycli
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js#newcode91 chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); Two questions: 1. Why doesn't autofocus attribute work ...
4 years, 1 month ago (2016-10-25 18:13:51 UTC) #20
jdufault
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js#newcode91 chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); On 2016/10/25 18:13:50, tommycli wrote: > Two questions: ...
4 years, 1 month ago (2016-10-26 22:38:36 UTC) #22
Dan Beam
https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js#newcode91 chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); don't start a focus storm https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resources/settings/settings_page/settings_subpage.html File chrome/browser/resources/settings/settings_page/settings_subpage.html ...
4 years, 1 month ago (2016-10-26 22:52:09 UTC) #25
jdufault
https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resources/settings/people_page/password_prompt_dialog.js#newcode91 chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); On 2016/10/26 22:52:09, Dan Beam wrote: > don't ...
4 years, 1 month ago (2016-10-26 23:21:08 UTC) #28
tommycli
lgtm other than: https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/setup_pin_dialog.js File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/setup_pin_dialog.js#newcode199 chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; On 2016/10/26 22:38:36, ...
4 years, 1 month ago (2016-10-27 00:13:25 UTC) #29
Dan Beam
https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js#newcode60 ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:60: return this.i18n('close'); this 'close' message is added where? in ...
4 years, 1 month ago (2016-10-27 00:26:27 UTC) #30
Dan Beam
https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc#newcode72 chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:72: {"close", IDS_CLOSE}, yeah, don't add a message here and ...
4 years, 1 month ago (2016-10-27 00:31:37 UTC) #31
Dan Beam
https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html#newcode97 ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/27 00:13:25, tommycli wrote: > On 2016/10/26 ...
4 years, 1 month ago (2016-10-27 01:25:48 UTC) #34
jdufault
https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html#newcode97 ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/27 01:25:48, Dan Beam wrote: > On ...
4 years, 1 month ago (2016-10-27 19:13:55 UTC) #37
jdufault
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/setup_pin_dialog.js File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resources/settings/people_page/setup_pin_dialog.js#newcode199 chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; On 2016/10/27 00:13:25, tommycli wrote: > ...
4 years, 1 month ago (2016-10-27 19:22:08 UTC) #39
Dan Beam
https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html#newcode21 chrome/browser/resources/settings/people_page/password_prompt_dialog.html:21: <dialog is="cr-dialog" id="dialog" on-close="onClose_"> close-text="$i18n{close}" https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): ...
4 years, 1 month ago (2016-10-27 19:33:42 UTC) #41
jdufault
https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html#newcode21 chrome/browser/resources/settings/people_page/password_prompt_dialog.html:21: <dialog is="cr-dialog" id="dialog" on-close="onClose_"> On 2016/10/27 19:33:41, Dan Beam ...
4 years, 1 month ago (2016-10-27 22:34:30 UTC) #45
Dan Beam
https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html#newcode5 chrome/browser/resources/settings/people_page/password_prompt_dialog.html:5: <link rel="import" href="/i18n_setup.html"> why is this needed? https://codereview.chromium.org/2441043004/diff/160001/components/components_strings.grd File ...
4 years, 1 month ago (2016-10-27 22:45:10 UTC) #47
jdufault
(trybots are red due to a bad commit somewhere else) https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resources/settings/people_page/password_prompt_dialog.html#newcode5 ...
4 years, 1 month ago (2016-10-28 23:07:59 UTC) #54
Dan Beam
not lgtm https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resources/options_resources.grd File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resources/options_resources.grd#newcode68 chrome/browser/resources/options_resources.grd:68: file="settings/route.html" wat https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resources/options_resources.grd#newcode70 chrome/browser/resources/options_resources.grd:70: flattenhtml="true" drop flattenhtml="true" ...
4 years, 1 month ago (2016-10-29 00:11:18 UTC) #55
jdufault
https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resources/options_resources.grd File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resources/options_resources.grd#newcode68 chrome/browser/resources/options_resources.grd:68: file="settings/route.html" On 2016/10/29 00:11:18, Dan Beam wrote: > wat ...
4 years, 1 month ago (2016-10-31 21:10:30 UTC) #60
Dan Beam
https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resources/settings/settings_page/settings_subpage.html File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resources/settings/settings_page/settings_subpage.html#newcode46 chrome/browser/resources/settings/settings_page/settings_subpage.html:46: alt="$i18n{back}"> why are you using alt= here? that's an ...
4 years, 1 month ago (2016-11-01 00:12:09 UTC) #61
jdufault
https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resources/settings/settings_page/settings_subpage.html File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resources/settings/settings_page/settings_subpage.html#newcode46 chrome/browser/resources/settings/settings_page/settings_subpage.html:46: alt="$i18n{back}"> On 2016/11/01 00:12:09, Dan Beam wrote: > why ...
4 years, 1 month ago (2016-11-08 23:29:12 UTC) #66
Dan Beam
lgtm
4 years, 1 month ago (2016-11-14 18:51:08 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2441043004/240001
4 years, 1 month ago (2016-11-14 19:02:59 UTC) #70
commit-bot: I haz the power
Committed patchset #11 (id:240001)
4 years, 1 month ago (2016-11-14 22:59:30 UTC) #72
commit-bot: I haz the power
4 years, 1 month ago (2016-11-14 23:12:44 UTC) #74
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/715df8e71fe09d4aed71eca4dec20faa4c2e6c5a
Cr-Commit-Position: refs/heads/master@{#431942}

Powered by Google App Engine
This is Rietveld 408576698