Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issue 2885353002: MD Bookmarks: Prevent keyboard shortcuts when the toolbar/dialogs are focused (Closed)

Created:
3 years, 7 months ago by tsergeant
Modified:
3 years, 6 months ago
Reviewers:
calamity
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Bookmarks: Improve keyboard shortcut targeting Keyboard shortcuts are no longer triggered by a global event listener looking at all events on the page. Instead, we process keyboard shortcuts as part of the existing input processing for the tree and the list, with an additional listener to pick up any key presses while nothing is focused. This has two benefits: 1. Pressing a keyboard shortcut with a sidebar folder focused will process shortcut for that node, rather than for the list selection. 2. Keyboard shortcuts will no longer activate when dialogs/buttons/input fields are active BUG=722300 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2885353002 Cr-Commit-Position: refs/heads/master@{#477468} Committed: https://chromium.googlesource.com/chromium/src/+/6c3a6dffe1ba6e9f3594f43a47c31e885cf5ffbd

Patch Set 1 #

Total comments: 6

Patch Set 2 : Complete rewrite #

Total comments: 2

Patch Set 3 : Add a test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -22 lines) Patch
M chrome/browser/resources/md_bookmarks/command_manager.js View 1 2 2 chunks +25 lines, -14 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/compiled_resources2.gyp View 1 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/folder_node.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/folder_node.js View 1 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.js View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/test/data/webui/md_bookmarks/command_manager_test.js View 1 5 chunks +10 lines, -8 lines 0 comments Download
M chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js View 1 2 3 chunks +46 lines, -0 lines 0 comments Download
M chrome/test/data/webui/md_bookmarks/toolbar_test.js View 1 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (21 generated)
tsergeant
PTAL. I'm worried that in the future we might need something more sophisticated than this, ...
3 years, 7 months ago (2017-05-17 07:17:16 UTC) #5
calamity
https://codereview.chromium.org/2885353002/diff/1/chrome/browser/resources/md_bookmarks/app.html File chrome/browser/resources/md_bookmarks/app.html (right): https://codereview.chromium.org/2885353002/diff/1/chrome/browser/resources/md_bookmarks/app.html#newcode77 chrome/browser/resources/md_bookmarks/app.html:77: <bookmarks-command-manager key-target="[[$$('#main-container')]]"> I had no idea you could do ...
3 years, 7 months ago (2017-05-19 04:09:27 UTC) #8
tsergeant
Okay, here's a complete rewrite of the existing CL. It achieves much the same thing ...
3 years, 6 months ago (2017-06-01 03:17:05 UTC) #15
calamity
lg. https://codereview.chromium.org/2885353002/diff/40001/chrome/browser/resources/md_bookmarks/list.js File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2885353002/diff/40001/chrome/browser/resources/md_bookmarks/list.js#newcode181 chrome/browser/resources/md_bookmarks/list.js:181: if (e.path[0] instanceof HTMLButtonElement && e.key == 'Enter') ...
3 years, 6 months ago (2017-06-06 07:15:11 UTC) #18
tsergeant
https://codereview.chromium.org/2885353002/diff/40001/chrome/browser/resources/md_bookmarks/list.js File chrome/browser/resources/md_bookmarks/list.js (right): https://codereview.chromium.org/2885353002/diff/40001/chrome/browser/resources/md_bookmarks/list.js#newcode181 chrome/browser/resources/md_bookmarks/list.js:181: if (e.path[0] instanceof HTMLButtonElement && e.key == 'Enter') { ...
3 years, 6 months ago (2017-06-06 08:10:48 UTC) #21
calamity
lgtm
3 years, 6 months ago (2017-06-06 08:18:47 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2885353002/60001
3 years, 6 months ago (2017-06-06 23:48:06 UTC) #26
commit-bot: I haz the power
3 years, 6 months ago (2017-06-06 23:53:19 UTC) #29
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6c3a6dffe1ba6e9f3594f43a47c3...

Powered by Google App Engine
This is Rietveld 408576698