|
|
Created:
4 years, 2 months ago by jdufault Modified:
4 years, 1 month ago 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. |
DescriptionAccessibility 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)
Description was changed from ========== Accessibility fixes on md-settings. BUG=606516 ========== to ========== Accessibility fixes on md-settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jdufault@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...
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Description was changed from ========== Accessibility fixes on md-settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Accessibility fixes on md-settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@chromium.org changed reviewers: + tommycli@chromium.org
Description was changed from ========== Accessibility fixes on md-settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Accessibility fixes for quick unlock settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Accessibility fixes for quick unlock settings. BUG=606516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== 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 ==========
tommycli@ PTAL when you get a chance - thanks!
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); Two questions: 1. Why doesn't autofocus attribute work the first time? 2. I see that you removed the autofocus attribute. Doesn't that conflict with the comment above? https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; Should we rename canSubmit? Since that means something different from enableSubmit_? https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> Can you use the $i18n{} macro here? https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:18: behaviors: [I18nBehavior], hopefully this is unneeded per the previous comment
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:91: this.$.passwordInput.focus(); Two questions: 1. Why doesn't autofocus attribute work the first time? 2. I see that you removed the autofocus attribute. Doesn't that conflict with the comment above? https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; Should we rename canSubmit? Since that means something different from enableSubmit_? https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> Can you use the $i18n{} macro here? https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:18: behaviors: [I18nBehavior], hopefully this is unneeded per the previous comment
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... 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: > > 1. Why doesn't autofocus attribute work the first time? > > 2. I see that you removed the autofocus attribute. Doesn't that conflict with > the comment above? I believe autofocus does not work because the content is in a dialog. The focus behavior is very weird - for example, moving the setTimeout call to the open method will cause document to get focused instead of passwordInput, but only after the dialog has been displayed once before. https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; On 2016/10/25 18:13:50, tommycli wrote: > Should we rename canSubmit? Since that means something different from > enableSubmit_? Renamed to isPinConfirmed_. https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/25 18:13:50, tommycli wrote: > Can you use the $i18n{} macro here? Nope :(. The other files in cr_elements/* use the same approach as this CL. https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:18: behaviors: [I18nBehavior], On 2016/10/25 18:13:50, tommycli wrote: > hopefully this is unneeded per the previous comment It's needed :(. I could remove it by calling loadTimeData.getString directly, though.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... 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/resource... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_subpage.html:45: <paper-icon-button icon="settings:arrow-back" on-tap="onTapBack_" alt="$i18n{back}"> need to wrap
The CQ bit was checked by jdufault@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...
https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... 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 start a focus storm Strange, using autofocus works as expected now. I've backed out the focus-related changes. https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_subpage.html:45: <paper-icon-button icon="settings:arrow-back" on-tap="onTapBack_" alt="$i18n{back}"> On 2016/10/26 22:52:09, Dan Beam wrote: > need to wrap Done.
lgtm other than: https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; On 2016/10/26 22:38:36, jdufault wrote: > On 2016/10/25 18:13:50, tommycli wrote: > > Should we rename canSubmit? Since that means something different from > > enableSubmit_? > > Renamed to isPinConfirmed_. Hey I'm still not sure what isPinConfirmed_ means. Does it mean that the confirmation pin matches the initial pin? Perhaps the comment needs an update. https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/26 22:38:36, jdufault wrote: > On 2016/10/25 18:13:50, tommycli wrote: > > Can you use the $i18n{} macro here? > > Nope :(. The other files in cr_elements/* use the same approach as this CL. Ah. A little gross, but if the other cr_elements do this too, I guess it's ok.
https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:60: return this.i18n('close'); this 'close' message is added where? in a setting handler?
https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:72: {"close", IDS_CLOSE}, yeah, don't add a message here and assume it's always going to exist for a shared UI component
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... 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 22:38:36, jdufault wrote: > > On 2016/10/25 18:13:50, tommycli wrote: > > > Can you use the $i18n{} macro here? > > > > Nope :(. The other files in cr_elements/* use the same approach as this CL. > > Ah. A little gross, but if the other cr_elements do this too, I guess it's ok. which other elements? the ones i've used generally inject strings from outer scope (i.e. cr_toolbar does this: https://cs.chromium.org/chromium/src/chrome/browser/resources/md_downloads/to...)
The CQ bit was checked by jdufault@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...
https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/27 01:25:48, Dan Beam wrote: > On 2016/10/27 00:13:25, tommycli wrote: > > On 2016/10/26 22:38:36, jdufault wrote: > > > On 2016/10/25 18:13:50, tommycli wrote: > > > > Can you use the $i18n{} macro here? > > > > > > Nope :(. The other files in cr_elements/* use the same approach as this CL. > > > > Ah. A little gross, but if the other cr_elements do this too, I guess it's ok. > > which other elements? the ones i've used generally inject strings from outer > scope (i.e. cr_toolbar does this: > https://cs.chromium.org/chromium/src/chrome/browser/resources/md_downloads/to...) cr_policy_indicator_behavior.js, cr_network_select.js, cr_network_list_item.js, cr_policy_network_indicator.js, cr_onc_types.js https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2441043004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:72: {"close", IDS_CLOSE}, On 2016/10/27 00:31:37, Dan Beam wrote: > yeah, don't add a message here and assume it's always going to exist for a > shared UI component Is there a better place to inject it? https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:60: return this.i18n('close'); On 2016/10/27 00:26:27, Dan Beam wrote: > this 'close' message is added where? in a setting handler? Yep
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2441043004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:199: this.enableSubmit_ = true; On 2016/10/27 00:13:25, tommycli wrote: > On 2016/10/26 22:38:36, jdufault wrote: > > On 2016/10/25 18:13:50, tommycli wrote: > > > Should we rename canSubmit? Since that means something different from > > > enableSubmit_? > > > > Renamed to isPinConfirmed_. > > Hey I'm still not sure what isPinConfirmed_ means. Does it mean that the > confirmation pin matches the initial pin? Perhaps the comment needs an update. Done; I've updated the comment.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resourc... 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/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:72: {"close", IDS_CLOSE}, leave these here https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> alt$="[[closeText]]" https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:18: behaviors: [I18nBehavior], remove https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:20: properties: { closeText: String,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/resourc... 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 wrote: > close-text="$i18n{close}" Done. https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2441043004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:72: {"close", IDS_CLOSE}, On 2016/10/27 19:33:41, Dan Beam wrote: > leave these here Done. https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: alt$="[[getCloseA11yText_()]]"> On 2016/10/27 19:33:41, Dan Beam wrote: > alt$="[[closeText]]" Done. https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:18: behaviors: [I18nBehavior], On 2016/10/27 19:33:41, Dan Beam wrote: > remove Done. https://codereview.chromium.org/2441043004/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:20: properties: { On 2016/10/27 19:33:41, Dan Beam wrote: > closeText: String, Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resourc... 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_... File components/components_strings.grd (right): https://codereview.chromium.org/2441043004/diff/160001/components/components_... components/components_strings.grd:217: <message name="IDS_BACK" desc="Used for Back on buttons"> why are you adding this to components strings? doesn't this already exist, generally, wherever you need it? https://codereview.chromium.org/2441043004/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/compiled_resources2.gyp (right): https://codereview.chromium.org/2441043004/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/compiled_resources2.gyp:10: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:i18n_behavior', revert
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
(trybots are red due to a bad commit somewhere else) https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2441043004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:5: <link rel="import" href="/i18n_setup.html"> On 2016/10/27 22:45:09, Dan Beam wrote: > why is this needed? Removed, it was needed for the previous patch. https://codereview.chromium.org/2441043004/diff/160001/components/components_... File components/components_strings.grd (right): https://codereview.chromium.org/2441043004/diff/160001/components/components_... components/components_strings.grd:217: <message name="IDS_BACK" desc="Used for Back on buttons"> On 2016/10/27 22:45:09, Dan Beam wrote: > why are you adding this to components strings? doesn't this already exist, > generally, wherever you need it? Ah, I looked before but I didn't see anything. I took another look and I can use a IDS_ACCNAME_BACK. https://codereview.chromium.org/2441043004/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/compiled_resources2.gyp (right): https://codereview.chromium.org/2441043004/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/compiled_resources2.gyp:10: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:i18n_behavior', On 2016/10/27 22:45:09, Dan Beam wrote: > revert Done.
not lgtm https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:68: file="settings/route.html" wat https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:70: flattenhtml="true" drop flattenhtml="true" and allowexternalscript="true" https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:72: <structure name="IDR_SETTINGS_I18N_SETUP_JS" there is no i18n_setup.js https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:73: file="settings/route.js" wat https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:75: flattenhtml="true" /> drop flattenhtml="true" https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:137: constexpr char kSettingsI18nSetupJSPath[] = "i18n_setup.js"; revert https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:271: path_to_idr_map_[kSettingsI18nSetupJSPath] = IDR_SETTINGS_I18N_SETUP_JS; revert
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:68: file="settings/route.html" On 2016/10/29 00:11:18, Dan Beam wrote: > wat Oops. The reason this worked was because the actual content of the script didn't matter too much, so long as it wasn't redirecting to the main md-settings page. https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:70: flattenhtml="true" On 2016/10/29 00:11:18, Dan Beam wrote: > drop flattenhtml="true" and allowexternalscript="true" Done. https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:72: <structure name="IDR_SETTINGS_I18N_SETUP_JS" On 2016/10/29 00:11:18, Dan Beam wrote: > there is no i18n_setup.js This is from a prior CL, I missed removing it (argh). https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:73: file="settings/route.js" On 2016/10/29 00:11:18, Dan Beam wrote: > wat See above. https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:75: flattenhtml="true" /> On 2016/10/29 00:11:18, Dan Beam wrote: > drop flattenhtml="true" Done. https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:137: constexpr char kSettingsI18nSetupJSPath[] = "i18n_setup.js"; On 2016/10/29 00:11:18, Dan Beam wrote: > revert Done. https://codereview.chromium.org/2441043004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:271: path_to_idr_map_[kSettingsI18nSetupJSPath] = IDR_SETTINGS_I18N_SETUP_JS; On 2016/10/29 00:11:18, Dan Beam wrote: > revert Done.
https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:46: alt="$i18n{back}"> why are you using alt= here? that's an <img> thing. can you use title= or aria-description?
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_subpage.html (right): https://codereview.chromium.org/2441043004/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_subpage.html:46: alt="$i18n{back}"> On 2016/11/01 00:12:09, Dan Beam wrote: > why are you using alt= here? that's an <img> thing. can you use title= or > aria-description? Done.
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2441043004/#ps240001 (title: "Use aria-label instead of alt")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/715df8e71fe09d4aed71eca4dec20faa4c2e6c5a Cr-Commit-Position: refs/heads/master@{#431942} |