Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820153003:
[MD Bookmarks] Add keyboard navigation to sidebar. (Closed)
|
Created:
3 years, 8 months ago by calamity Modified:
3 years, 7 months ago Reviewers:
tsergeant CC:
chromium-reviews, dcheng, michaelpg+watch-md-ui_chromium.org, jlklein+watch-closure_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD Bookmarks] Add keyboard navigation to sidebar.
This CL adds keyboard navigation to the bookmark manager sidebar by
making only a single focus target for the sidebar which is changed by
keypresses.
BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2820153003
Cr-Commit-Position: refs/heads/master@{#468913}
Committed: https://chromium.googlesource.com/chromium/src/+/84f04dbf558f6c3391ef4115e45d7c0ddefc0f00
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix_breakage #Patch Set 3 : select_on_move #
Total comments: 13
Patch Set 4 : rebase, address comments #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : address comments #Patch Set 7 : fix sidebar.html #Patch Set 8 : rebase #Patch Set 9 : fix test #
Total comments: 4
Patch Set 10 : address comments #Dependent Patchsets: Messages
Total messages: 50 (38 generated)
Description was changed from ========== [MD Bookmarks] Add keyboard navigation to sidebar. This CL adds keyboard navigation to the bookmark manager sidebar by making only a single focus target for the sidebar which is changed by keypresses. BUG=692844 ========== to ========== [MD Bookmarks] Add keyboard navigation to sidebar. This CL adds keyboard navigation to the bookmark manager sidebar by making only a single focus target for the sidebar which is changed by keypresses. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
And here's the meat. It's a doozy. I have one or two things left to do on it (mousedown needs to change the focus), but it should be enough to give you a good idea of the approach, and evaluate the testing strategy (which is gross). Let me know if you have any large-scale initial thoughts =D
Firstly, there's two places (the first two comments) where the code seems to be wrong. The result is that the CL doesn't do anything when it's patched locally. It scares me that the tests are totally okay about that. Secondly, an important piece that seems to be missing is what happens when an item is focused. At the moment focusing an item doesn't seem to do anything, so it's hard to know if this code is actually doing the right thing. I think we discussed doing what the old BMM does and having focus change immediately select that folder. Is that still what you intend to do? If so, the distinction between FolderNode.focused and FolderNode.isSelectedItem_ confuses me. Are there any cases where they should be different? It seems like this CL goes to a lot of effort to maintain a new variable that's a duplicate of an old one. As it is, with these two things it's hard for me to speak too specifically about the approach. I'll look again once they're addressed. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/app.js (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/app.js:114: this.focusedFolderNode_ = true; What's going on here? My guess is this was supposed to be this.focusedFolderNode_.focusable = true; ? https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.html:86: draggable="[[!isRootFolder_(depth)]]" tabindex="getTabIndex_(focusable)" needs $="[[]]" https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:97: if (this.keyboardEventMatchesKeys(e, 'up')) { Seems like overkill to use IronA11yKeysBehavior just for this: just do e.key == 'ArrowUp' instead. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:104: Is left/right going to be handled here too? Maybe add a TODO for it. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:130: if (!newFocusFolderNode || newFocusFolderNode.itemId != ROOT_NODE_ID) { This can be simplified a little bit: if (!newFocusFolderNode) return false; if (newFocusFolderNode.itemId != ROOT_NODE_ID) { this.fire('folder-node-focus-changed', newFocusFolderNode); newFocusFolderNode.getFocusTarget().focus(); } return true; https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:148: * @return {HTMLElement|null} Returns null Returns null...when?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Okay, fixed the things. Selecting on focus feels a bit weird and clicking doesn't focus the item, but a second click does. Feels a bit weird, but I'm not sure if showing the focus ring every folder selection is appropriate either. It also feels weird that sidebar focus will look different to main list focus (ring vs highlight) but maybe we can ask Alan about that after it lands. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/app.js (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/app.js:114: this.focusedFolderNode_ = true; On 2017/04/19 01:42:16, tsergeant wrote: > What's going on here? My guess is this was supposed to be > > this.focusedFolderNode_.focusable = true; > > ? Done. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.html:86: draggable="[[!isRootFolder_(depth)]]" tabindex="getTabIndex_(focusable)" On 2017/04/19 01:42:16, tsergeant wrote: > needs $="[[]]" Done. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:97: if (this.keyboardEventMatchesKeys(e, 'up')) { On 2017/04/19 01:42:16, tsergeant wrote: > Seems like overkill to use IronA11yKeysBehavior just for this: just do > > e.key == 'ArrowUp' > > instead. Done. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:104: On 2017/04/19 01:42:16, tsergeant wrote: > Is left/right going to be handled here too? Maybe add a TODO for it. Yeah. It's in a follow-up. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:130: if (!newFocusFolderNode || newFocusFolderNode.itemId != ROOT_NODE_ID) { On 2017/04/19 01:42:16, tsergeant wrote: > This can be simplified a little bit: > > if (!newFocusFolderNode) > return false; > > if (newFocusFolderNode.itemId != ROOT_NODE_ID) { > this.fire('folder-node-focus-changed', newFocusFolderNode); > newFocusFolderNode.getFocusTarget().focus(); > } > > return true; Done. https://codereview.chromium.org/2820153003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_bookmarks/folder_node.js:148: * @return {HTMLElement|null} Returns null On 2017/04/19 01:42:16, tsergeant wrote: > Returns null...when? Done.
As discussed, I'm pretty happy with this version https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:89: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> Should this be tabindex="-1"? https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:151: Array.from(this.root.querySelectorAll('bookmarks-folder-node')); Pull this out into a helper getter, since it's used in a couple of places. https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:154: if (this == current) { I find it confusing that this is packaged into this function. Essentially, the expected flow of the algorithm is that `this == current` is true during the first event handler, but never again after that. On top of that, because this function handles two disjoint cases, the 'current' variable can be one of two fairly different things. I think the algorithm would be easier to read and understand if the `this==current` branch was pulled out into a separate function that is called from changeKeyboardSelection_() directly. Then, this function would only handle one case (`current` is an active child), which means we can also rename `current` to something clearer: I would suggest `activeChild`. https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/folder_node_test.js (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/folder_node_test.js:131: test('keyboard selection', function() { I think it would make a lot of sense to use an interactive UI 'integration' test instead of this. So much stuff is done manually here, and it still isn't able to test the actual keydown handler, or that the correct element gets focused. I think it would be nicer to test that pressing the down key focuses the right element directly. https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/folder_node_test.js:156: assertFalse(changeKeyboardFocus(1, '2', '2')); I think this is testing slightly the wrong thing? Here you're setting activeElement to the folder-node with ID 2, but in reality it should be the #container child of that node. It doesn't cause any problems at the moment, but it conceivably could if you follow my suggestions from above.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by calamity@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 #4 (id:160001) has been deleted
The CQ bit was checked by calamity@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by calamity@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/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:89: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> On 2017/04/19 07:32:17, tsergeant wrote: > Should this be tabindex="-1"? I think this is a little better. -1 is still focusable by mouse, which causes the focus to silently change when you click another item's button. Pressing up/down after that then goes up/down relative to that item. Clicking with tabindex="" focuses the list and makes up/down move that instead of selecting a new item. https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:151: Array.from(this.root.querySelectorAll('bookmarks-folder-node')); On 2017/04/19 07:32:18, tsergeant wrote: > Pull this out into a helper getter, since it's used in a couple of places. Done. https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:154: if (this == current) { On 2017/04/19 07:32:18, tsergeant wrote: > I find it confusing that this is packaged into this function. Essentially, the > expected flow of the algorithm is that `this == current` is true during the > first event handler, but never again after that. On top of that, because this > function handles two disjoint cases, the 'current' variable can be one of two > fairly different things. > > I think the algorithm would be easier to read and understand if the > `this==current` branch was pulled out into a separate function that is called > from changeKeyboardSelection_() directly. > > Then, this function would only handle one case (`current` is an active child), > which means we can also rename `current` to something clearer: I would suggest > `activeChild`. Hmm, ok. I've changed things here a little bit. WDYT? https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/folder_node_test.js (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/folder_node_test.js:131: test('keyboard selection', function() { On 2017/04/19 07:32:18, tsergeant wrote: > I think it would make a lot of sense to use an interactive UI 'integration' test > instead of this. > > So much stuff is done manually here, and it still isn't able to test the actual > keydown handler, or that the correct element gets focused. I think it would be > nicer to test that pressing the down key focuses the right element directly. Yeah, ok. https://codereview.chromium.org/2820153003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/folder_node_test.js:156: assertFalse(changeKeyboardFocus(1, '2', '2')); On 2017/04/19 07:32:18, tsergeant wrote: > I think this is testing slightly the wrong thing? > > Here you're setting activeElement to the folder-node with ID 2, but in reality > it should be the #container child of that node. It doesn't cause any problems at > the moment, but it conceivably could if you follow my suggestions from above. Yeah, you're right. Fixed.
Cool, I think new focus test is significantly better. Also, Rietveld has no idea what's going on with sidebar.html =S https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:89: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> On 2017/04/28 06:15:47, calamity wrote: > On 2017/04/19 07:32:17, tsergeant wrote: > > Should this be tabindex="-1"? > > I think this is a little better. -1 is still focusable by mouse, which causes > the focus to silently change when you click another item's button. Pressing > up/down after that then goes up/down relative to that item. > > Clicking with tabindex="" focuses the list and makes up/down move that instead > of selecting a new item. I'm not sure what you mean by "focuses the list"? When I click the toggle button with a different node selected, the focus gets dropped and goes back to <body> https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:154: if (this == current) { On 2017/04/28 06:15:47, calamity wrote: > On 2017/04/19 07:32:18, tsergeant wrote: > > I find it confusing that this is packaged into this function. Essentially, the > > expected flow of the algorithm is that `this == current` is true during the > > first event handler, but never again after that. On top of that, because this > > function handles two disjoint cases, the 'current' variable can be one of two > > fairly different things. > > > > I think the algorithm would be easier to read and understand if the > > `this==current` branch was pulled out into a separate function that is called > > from changeKeyboardSelection_() directly. > > > > Then, this function would only handle one case (`current` is an active child), > > which means we can also rename `current` to something clearer: I would suggest > > `activeChild`. > > Hmm, ok. I've changed things here a little bit. WDYT? Yeah, I think this is a bit better. It's still a complex algorithm, so we probably can't make it perfect anyway. https://codereview.chromium.org/2820153003/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:155: * @private Is this still intended to be @private? https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:85: focusAndKeydown('1', 'ArrowDown'); Two related questions: - Why do you focus the folder node each time before you do the keydown? Shouldn't in (most cases?) focus already be there? - If you do need to manually focus, can we have at least one case that checks focus is in the right place after the key handler? https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:88: store.data.selectedFolder = '2'; With the recent changes to TestStore enough, are you able to remove all of these manual changes to the store?
The CQ bit was checked by calamity@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_ozone_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 calamity@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_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 calamity@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/2820153003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:89: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> On 2017/05/01 01:35:57, tsergeant wrote: > On 2017/04/28 06:15:47, calamity wrote: > > On 2017/04/19 07:32:17, tsergeant wrote: > > > Should this be tabindex="-1"? > > > > I think this is a little better. -1 is still focusable by mouse, which causes > > the focus to silently change when you click another item's button. Pressing > > up/down after that then goes up/down relative to that item. > > > > Clicking with tabindex="" focuses the list and makes up/down move that instead > > of selecting a new item. > > I'm not sure what you mean by "focuses the list"? When I click the toggle button > with a different node selected, the focus gets dropped and goes back to <body> Discussed. https://codereview.chromium.org/2820153003/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2820153003/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.js:155: * @private On 2017/05/01 01:35:57, tsergeant wrote: > Is this still intended to be @private? Changed the method name. https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:85: focusAndKeydown('1', 'ArrowDown'); On 2017/05/01 01:35:57, tsergeant wrote: > Two related questions: > > - Why do you focus the folder node each time before you do the keydown? > Shouldn't in (most cases?) focus already be there? This was just because the notifyObservers() would mess up things. TestStore change has made this obsolete. > - If you do need to manually focus, can we have at least one case that checks > focus is in the right place after the key handler? This is probably a good idea anyway so done. https://codereview.chromium.org/2820153003/diff/220001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:88: store.data.selectedFolder = '2'; On 2017/05/01 01:35:57, tsergeant wrote: > With the recent changes to TestStore enough, are you able to remove all of these > manual changes to the store? Yep, that was the dream, and now the dream is real.
https://codereview.chromium.org/2820153003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:83: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> With the change to paper-icon-button-light, tabindex doesn't seem to take effect like it used to. You can now tab through to each of the buttons. Blergh. Any ideas how we can fix that? Maybe we'll need to preventDefault on mousedown or similar. https://codereview.chromium.org/2820153003/diff/300001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2820153003/diff/300001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:6: * @fileoverview Tests for MD History which are run as interactive ui tests. Not "MD History" anymore...
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 calamity@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/2820153003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2820153003/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/folder_node.html:83: is-open$="[[!isClosed_]]" on-tap="toggleFolder_" tabindex=""> On 2017/05/03 03:56:38, tsergeant wrote: > With the change to paper-icon-button-light, tabindex doesn't seem to take effect > like it used to. You can now tab through to each of the buttons. > > Blergh. Any ideas how we can fix that? Maybe we'll need to preventDefault on > mousedown or similar. Made mousedown preventDefault. https://codereview.chromium.org/2820153003/diff/300001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2820153003/diff/300001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:6: * @fileoverview Tests for MD History which are run as interactive ui tests. On 2017/05/03 03:56:38, tsergeant wrote: > Not "MD History" anymore... Done.
lgtm
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 calamity@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": 320001, "attempt_start_ts": 1493794951567430,
"parent_rev": "c8be5e3666ee8a9afcd174f28d1ff49101fe5636", "commit_rev":
"84f04dbf558f6c3391ef4115e45d7c0ddefc0f00"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Add keyboard navigation to sidebar. This CL adds keyboard navigation to the bookmark manager sidebar by making only a single focus target for the sidebar which is changed by keypresses. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add keyboard navigation to sidebar. This CL adds keyboard navigation to the bookmark manager sidebar by making only a single focus target for the sidebar which is changed by keypresses. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2820153003 Cr-Commit-Position: refs/heads/master@{#468913} Committed: https://chromium.googlesource.com/chromium/src/+/84f04dbf558f6c3391ef4115e45d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:320001) as https://chromium.googlesource.com/chromium/src/+/84f04dbf558f6c3391ef4115e45d... |
