Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533173002:
MD Settings: Listen to the Enter key in the Add User ChromeOS dialog. (Closed)
|
Created:
4 years ago by tommycli Modified:
4 years ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Listen to the Enter key in the Add User ChromeOS dialog.
Adds the Enter key listener in the same way as the Add Site dialog.
This uses the iron-a11y-keys method.
BUG=662366
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0ec6daa43735dfda250f19510fefb8b302e9bafe
Cr-Commit-Position: refs/heads/master@{#435387}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== MD Settings: Listen to the Enter key in the Add User ChromeOS dialog. Adds the Enter key listener in the same way as the Add Site dialog. This uses the iron-a11y-keys method. BUG=662366 ========== to ========== MD Settings: Listen to the Enter key in the Add User ChromeOS dialog. Adds the Enter key listener in the same way as the Add Site dialog. This uses the iron-a11y-keys method. BUG=662366 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + michaelpg@chromium.org
michaelpg: PTAL thanks!
https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/users_add_user_dialog.html (right): https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23: <iron-a11y-keys id="keys" keys="enter" on-keys-pressed="addUser_"> the Polymer example sets the "target" attribute so the key is registered on the <paper-input>. if you don't do that, does the Enter key anywhere in the dialog trigger addUser_? in particular, would Enter on the Cancel button trigger addUser_?
michaelpg: thanks! https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/users_add_user_dialog.html (right): https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23: <iron-a11y-keys id="keys" keys="enter" on-keys-pressed="addUser_"> On 2016/11/29 02:54:10, michaelpg wrote: > the Polymer example sets the "target" attribute so the key is registered on the > <paper-input>. if you don't do that, does the Enter key anywhere in the dialog > trigger addUser_? in particular, would Enter on the Cancel button trigger > addUser_? Hello. If there is no target attr set, the target element is the parent node -- and that works nicely for me: https://github.com/PolymerElements/iron-a11y-keys/blob/master/iron-a11y-keys.... Since the Cancel button is not in the Body div but instead in the button-container div, the Enter key is not captured for the cancel button. I have manually confirmed this with console logs. See here for another example of an element without the target attr set: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...
The CQ bit was checked by tommycli@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.
lgtm https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/users_add_user_dialog.html (right): https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23: <iron-a11y-keys id="keys" keys="enter" on-keys-pressed="addUser_"> On 2016/11/30 15:48:34, tommycli wrote: > On 2016/11/29 02:54:10, michaelpg wrote: > > the Polymer example sets the "target" attribute so the key is registered on > the > > <paper-input>. if you don't do that, does the Enter key anywhere in the dialog > > trigger addUser_? in particular, would Enter on the Cancel button trigger > > addUser_? > > Hello. If there is no target attr set, the target element is the parent node -- > and that works nicely for me: > https://github.com/PolymerElements/iron-a11y-keys/blob/master/iron-a11y-keys.... > > Since the Cancel button is not in the Body div but instead in the > button-container div, the Enter key is not captured for the cancel button. I > have manually confirmed this with console logs. > > See here for another example of an element without the target attr set: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s... well gee, thanks Polymer for mentioning that in your docs.... Maybe it'd be clearer to put it inside the <paper-input>, then? or am I bikeshedding now? anyway, main point is that this works as expected even if it's not intuitive.
thanks! https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/users_add_user_dialog.html (right): https://codereview.chromium.org/2533173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23: <iron-a11y-keys id="keys" keys="enter" on-keys-pressed="addUser_"> On 2016/11/30 18:02:19, michaelpg wrote: > On 2016/11/30 15:48:34, tommycli wrote: > > On 2016/11/29 02:54:10, michaelpg wrote: > > > the Polymer example sets the "target" attribute so the key is registered on > > the > > > <paper-input>. if you don't do that, does the Enter key anywhere in the > dialog > > > trigger addUser_? in particular, would Enter on the Cancel button trigger > > > addUser_? > > > > Hello. If there is no target attr set, the target element is the parent node > -- > > and that works nicely for me: > > > https://github.com/PolymerElements/iron-a11y-keys/blob/master/iron-a11y-keys.... > > > > Since the Cancel button is not in the Body div but instead in the > > button-container div, the Enter key is not captured for the cancel button. I > > have manually confirmed this with console logs. > > > > See here for another example of an element without the target attr set: > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s... > > well gee, thanks Polymer for mentioning that in your docs.... > > Maybe it'd be clearer to put it inside the <paper-input>, then? or am I > bikeshedding now? anyway, main point is that this works as expected even if it's > not intuitive. Done.
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2533173002/#ps20001 (title: "address comments")
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": 20001, "attempt_start_ts": 1480529706692860,
"parent_rev": "0bf8ff4f12201c898171588737398a40967a78f4", "commit_rev":
"ec8df9f53b6dbe665cc33966ecebbfe190bfd914"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Listen to the Enter key in the Add User ChromeOS dialog. Adds the Enter key listener in the same way as the Add Site dialog. This uses the iron-a11y-keys method. BUG=662366 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Listen to the Enter key in the Add User ChromeOS dialog. Adds the Enter key listener in the same way as the Add Site dialog. This uses the iron-a11y-keys method. BUG=662366 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0ec6daa43735dfda250f19510fefb8b302e9bafe Cr-Commit-Position: refs/heads/master@{#435387} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0ec6daa43735dfda250f19510fefb8b302e9bafe Cr-Commit-Position: refs/heads/master@{#435387} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
