Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272553002:
MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab (Closed)
|
Created:
4 years, 4 months ago by tsergeant Modified:
4 years, 2 months ago Reviewers:
Dan Beam CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org, michaelpg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab
This brings the behavior of the more actions menu in MD History closer
in line with other menus in Chrome.
BUG=630102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8cf11a02e256b61c86a23e9c6126ff3183a628c5
Cr-Commit-Position: refs/heads/master@{#421777}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change behavior, rebase #
Total comments: 4
Patch Set 3 : Expand tests #
Total comments: 4
Patch Set 4 : dbeam@ review comments #
Messages
Total messages: 34 (21 generated)
Description was changed from ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 ========== to ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tsergeant@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...
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:160: e.preventDefault(); er, wat? so you want to stop tab from changing focus?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:160: e.preventDefault(); On 2016/08/23 05:50:40, Dan Beam wrote: > er, wat? so you want to stop tab from changing focus? It's complicated. I probably should put a comment here. Basically, without the preventDefault: 1. We close the menu 2. The focus change fires and focuses <body>, which is the element after the <cr-shared-menu> in the tab order. 3. The menu asynchronously refocuses the menu button which opened it. But for some reason, after number 3, even though document.activeElement indicates that the menu button is focused, the document acts as if <body> is focused. This looks likely to be a bug deeply nestled in Blink somewhere. I haven't had a chance to look into it yet. Adding preventDefault cuts out #2 and refocuses the menu button correctly.
https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:160: e.preventDefault(); On 2016/08/23 07:08:40, tsergeant wrote: > On 2016/08/23 05:50:40, Dan Beam wrote: > > er, wat? so you want to stop tab from changing focus? > > It's complicated. I probably should put a comment here. Basically, without the > preventDefault: > > 1. We close the menu > 2. The focus change fires and focuses <body>, which is the element after the > <cr-shared-menu> in the tab order. > 3. The menu asynchronously refocuses the menu button which opened it. > > But for some reason, after number 3, even though document.activeElement > indicates that the menu button is focused, the document acts as if <body> is > focused. > > This looks likely to be a bug deeply nestled in Blink somewhere. I haven't had a > chance to look into it yet. > > Adding preventDefault cuts out #2 and refocuses the menu button correctly. don't we want focus to go to the next focusable thing instead of be restored to the menu button?
Description was changed from ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 ========== to ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Alright, I've updated and changed this CL. It now focuses the next focusable element on close. PTAL. As an aside, I don't remember if I talked to you about moving the position of the <cr-shared-menu> element in the DOM tree so that it is a sibling of the anchor. This is a promising solution, since it fixes a few accessibility problems at once, but it also mucks up styling (since the element can potentially cross shadow-root styling boundaries) and stacking contexts (which there are a lot of inside an iron-list).
The CQ bit was checked by tsergeant@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/2272553002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:148: if (Polymer.IronA11yKeysBehavior.keyboardEventMatchesKeys(e, 'tab')) { what does this do on shift tab?
https://codereview.chromium.org/2272553002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (left): https://codereview.chromium.org/2272553002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:156: MockInteractions.pressAndReleaseKeyOn(items[0], 9, ['shift']); can we bring these back and test that shift tab also closes the menu?
The CQ bit was checked by tsergeant@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/2272553002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (left): https://codereview.chromium.org/2272553002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:156: MockInteractions.pressAndReleaseKeyOn(items[0], 9, ['shift']); On 2016/09/23 06:27:29, Dan Beam wrote: > can we bring these back and test that shift tab also closes the menu? Done. https://codereview.chromium.org/2272553002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:148: if (Polymer.IronA11yKeysBehavior.keyboardEventMatchesKeys(e, 'tab')) { On 2016/09/23 06:25:32, Dan Beam wrote: > what does this do on shift tab? The conditional matches on shift tab, so the menu will close and focus will move back to the previous element.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm w/question https://codereview.chromium.org/2272553002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2272553002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:15: menu.addEventListener('iron-overlay-opened', function f() { btw, listenOnce() is now a thing: https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=listenOn... https://codereview.chromium.org/2272553002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:91: 'keydown', this.onCaptureKeyDown_.bind(this), true); do we need a detached() equivalent?
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2272553002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2272553002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:15: menu.addEventListener('iron-overlay-opened', function f() { On 2016/09/29 03:22:59, Dan Beam wrote: > btw, listenOnce() is now a thing: > > https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=listenOn... Neat, done. https://codereview.chromium.org/2272553002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2272553002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:91: 'keydown', this.onCaptureKeyDown_.bind(this), true); On 2016/09/29 03:22:59, Dan Beam wrote: > do we need a detached() equivalent? Good point, done.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2272553002/#ps120001 (title: "dbeam@ review comments")
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.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Use arrow keys for navigation in cr-shared-menu, close on tab This brings the behavior of the more actions menu in MD History closer in line with other menus in Chrome. BUG=630102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8cf11a02e256b61c86a23e9c6126ff3183a628c5 Cr-Commit-Position: refs/heads/master@{#421777} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8cf11a02e256b61c86a23e9c6126ff3183a628c5 Cr-Commit-Position: refs/heads/master@{#421777} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
