Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552013003:
Ensure spoken feedback keyboard hooks prevent re-injection of key up events (Closed)
|
Created:
4 years ago by David Tseng Modified:
4 years ago Reviewers:
dmazzoni CC:
chromium-reviews, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure spoken feedback keyboard hooks prevent re-injection of key up events
Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js.
If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate.
BUG=668771
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/fa04240e9ca5c26709789a3ebfb5ed522a1d4355
Cr-Commit-Position: refs/heads/master@{#436653}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Ensure spoken feedback keyboard hooks prevent re-injection of key up events #Messages
Total messages: 14 (8 generated)
Description was changed from ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG= ========== to ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG=668771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org
lgtm but I think the logic can be simpler https://codereview.chromium.org/2552013003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/keyboard_handler.js (right): https://codereview.chromium.org/2552013003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/keyboard_handler.js:69: for (var i = 0; i < this.preventedKeys_.length; i++) { If you make preventedKeys_ a Set(), I think you can replace all of this code with just this: if (this.preventedKeys_.has(evt.keyCode)) { this.preventedKeys_.delete(evt.keyCode); evt.preventDefault(); evt.stopPropagation(); }
https://codereview.chromium.org/2552013003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/keyboard_handler.js (right): https://codereview.chromium.org/2552013003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/keyboard_handler.js:69: for (var i = 0; i < this.preventedKeys_.length; i++) { On 2016/12/06 07:25:22, dmazzoni wrote: > If you make preventedKeys_ a Set(), I think you can replace all of this code > with just this: > > if (this.preventedKeys_.has(evt.keyCode)) { > this.preventedKeys_.delete(evt.keyCode); > evt.preventDefault(); > evt.stopPropagation(); > } Sure. Was storing the entire event before. Changed to use a set.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2552013003/#ps20001 (title: "Ensure spoken feedback keyboard hooks prevent re-injection of key up events")
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": 1481044118101590,
"parent_rev": "2498faecfab1dc1c147d17c85047336a2bf1aba3", "commit_rev":
"18420861e0b3289ea45305fba4881ad0ef6437cc"}
Message was sent while issue was closed.
Description was changed from ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG=668771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG=668771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG=668771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Ensure spoken feedback keyboard hooks prevent re-injection of key up events Due to the way we handle keyboard events, we need to explicitly handle prevention of both key down and key up events in js. If we do not, accelerators that happen to trigger via release e.g. the high contrast shortcut, will activate. BUG=668771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/fa04240e9ca5c26709789a3ebfb5ed522a1d4355 Cr-Commit-Position: refs/heads/master@{#436653} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fa04240e9ca5c26709789a3ebfb5ed522a1d4355 Cr-Commit-Position: refs/heads/master@{#436653} |
