Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632783003:
MD Settings People: Make Enter key work for Sync Page (Closed)
|
Created:
3 years, 11 months ago by tommycli Modified:
3 years, 11 months ago Reviewers:
dschuyler 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 People: Make Enter key work for Sync Page
Fixes these two cases:
1) Enter key for creating a new passphrase
2) Enter key for submitting the previously-set sync passphrase.
BUG=622816
TEST=MANUAL
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2632783003
Cr-Commit-Position: refs/heads/master@{#445491}
Committed: https://chromium.googlesource.com/chromium/src/+/c2f23d8d70cf93aef29ee64a957ab6a8714798b7
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix up iron-a11y-keys #
Total comments: 9
Messages
Total messages: 19 (8 generated)
Description was changed from ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL ========== to ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL. No test because i think the test would be kind of tautological
https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.html:228: <div id="create-password-box"> can we just use keyBindings and/or IronA11yKeysBehavior directly rather than making an <iron-a11y-keys> tag for this? https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.html:271: on-keys-pressed="onSubmitExistingPassphraseTap_"> if we do end up going with the tag, you should close it ;)
Thanks! https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.html:228: <div id="create-password-box"> On 2017/01/14 00:08:07, Dan Beam wrote: > can we just use keyBindings and/or IronA11yKeysBehavior directly rather than > making an <iron-a11y-keys> tag for this? I gave this a try locally, and the main problem is that it listens for Enter anywhere on the page rather than in the inputs. In this case I have to inspect the event.target to determine where it's coming from. Unfortunately event.target and event.currentTarget seem to be null for some reason. event.detail.keyboardEvent.target is the settings-sync-page. I'd recommend just sticking with iron-a11y-keys for sanity. The users_page keyBindings is dead code anyways, so IronA11yKeysBehavior is truly never used within settings (so far) https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.html:271: on-keys-pressed="onSubmitExistingPassphraseTap_"> On 2017/01/14 00:08:07, Dan Beam wrote: > if we do end up going with the tag, you should close it ;) Done.
On 2017/01/14 00:33:16, tommycli wrote: > Thanks! > > https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/sync_page.html (right): > > https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/sync_page.html:228: <div > id="create-password-box"> > On 2017/01/14 00:08:07, Dan Beam wrote: > > can we just use keyBindings and/or IronA11yKeysBehavior directly rather than > > making an <iron-a11y-keys> tag for this? > > I gave this a try locally, and the main problem is that it listens for Enter > anywhere on the page rather than in the inputs. > > In this case I have to inspect the event.target to determine where it's coming > from. Unfortunately event.target and event.currentTarget seem to be null for > some reason. event.detail.keyboardEvent.target is the settings-sync-page. > > I'd recommend just sticking with iron-a11y-keys for sanity. The users_page > keyBindings is dead code anyways, so IronA11yKeysBehavior is truly never used > within settings (so far) > > https://codereview.chromium.org/2632783003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/sync_page.html:271: > on-keys-pressed="onSubmitExistingPassphraseTap_"> > On 2017/01/14 00:08:07, Dan Beam wrote: > > if we do end up going with the tag, you should close it ;) > > Done. dbeam: ping, thanks!
tommycli@chromium.org changed reviewers: + dschuyler@chromium.org
dschuyler: PTAL, thanks!
https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:232: <iron-a11y-keys id="keys" keys="enter" Should this have a target=""? https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:233: on-keys-pressed="onSaveNewPassphraseTap_"> Does this work with modified enter key presses or will they be ignored (like shift+enter or alt+enter)? https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:234: </iron-a11y-keys> Does "esc" work for cancelling focus? Maybe that could be a separate bug/CL if doesn't work (or added to this CL if you prefer - which is why I'm mentioning it here).
dschulyer: Thanks! see below. https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:232: <iron-a11y-keys id="keys" keys="enter" On 2017/01/17 23:20:11, dschuyler wrote: > Should this have a target=""? I don't think it's necessary. Our other usages don't have that: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:233: on-keys-pressed="onSaveNewPassphraseTap_"> On 2017/01/17 23:20:12, dschuyler wrote: > Does this work with modified enter key presses or will they be ignored (like > shift+enter or alt+enter)? Unfortunately, yes, it captures modified Enter presses also. And it appears there's no way to prevent it from doing so: https://github.com/PolymerElements/iron-a11y-keys-behavior/issues/41 My recommendation is to let this through and I'll file a P3 bug to fix. This CL fixes a P1 at the expense of creating a P3. https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:234: </iron-a11y-keys> On 2017/01/17 23:20:12, dschuyler wrote: > Does "esc" work for cancelling focus? Maybe that could be a separate bug/CL if > doesn't work (or added to this CL if you prefer - which is why I'm mentioning it > here). It's not clear to me that esc should blur the input fields. They don't blur the default HTML input field for instance: http://www.w3schools.com/html/html_forms.asp
Description was changed from ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
lgtm https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:232: <iron-a11y-keys id="keys" keys="enter" On 2017/01/18 00:51:29, tommycli wrote: > On 2017/01/17 23:20:11, dschuyler wrote: > > Should this have a target=""? > > I don't think it's necessary. Our other usages don't have that: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... Acknowledged. https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:233: on-keys-pressed="onSaveNewPassphraseTap_"> On 2017/01/18 00:51:30, tommycli wrote: > On 2017/01/17 23:20:12, dschuyler wrote: > > Does this work with modified enter key presses or will they be ignored (like > > shift+enter or alt+enter)? > > Unfortunately, yes, it captures modified Enter presses also. And it appears > there's no way to prevent it from doing so: > https://github.com/PolymerElements/iron-a11y-keys-behavior/issues/41 > > My recommendation is to let this through and I'll file a P3 bug to fix. This CL > fixes a P1 at the expense of creating a P3. Thanks, I agree that trading a P1 for a P3 is a good deal. https://codereview.chromium.org/2632783003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:234: </iron-a11y-keys> On 2017/01/18 00:51:30, tommycli wrote: > On 2017/01/17 23:20:12, dschuyler wrote: > > Does "esc" work for cancelling focus? Maybe that could be a separate bug/CL if > > doesn't work (or added to this CL if you prefer - which is why I'm mentioning > it > > here). > > It's not clear to me that esc should blur the input fields. They don't blur the > default HTML input field for instance: > http://www.w3schools.com/html/html_forms.asp Acknowledged.
dschulyer: thanks!
The CQ bit was checked by tommycli@chromium.org
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": 1485201698418990,
"parent_rev": "8e6835872476554e6ccc30565424434faee1a6e3", "commit_rev":
"c2f23d8d70cf93aef29ee64a957ab6a8714798b7"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings People: Make Enter key work for Sync Page Fixes these two cases: 1) Enter key for creating a new passphrase 2) Enter key for submitting the previously-set sync passphrase. BUG=622816 TEST=MANUAL CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2632783003 Cr-Commit-Position: refs/heads/master@{#445491} Committed: https://chromium.googlesource.com/chromium/src/+/c2f23d8d70cf93aef29ee64a957a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c2f23d8d70cf93aef29ee64a957a... |
