Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738893003:
Use keys 1 through 3 to switch between and click on focusable elements. (Closed)
|
Created:
3 years, 9 months ago by elichtenberg Modified:
3 years, 9 months ago 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, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse keys 1 through 3 to switch between and click on focusable elements.
BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2738893003
Cr-Commit-Position: refs/heads/master@{#456161}
Committed: https://chromium.googlesource.com/chromium/src/+/e2a3dd0d63dd994aaad58f8818f0ed075b453834
Patch Set 1 #
Total comments: 4
Patch Set 2 : Refactored tree walking code. #
Total comments: 14
Patch Set 3 : Formatting changes based on CL comments. #
Total comments: 2
Patch Set 4 : Removed logging in switch_access_event_handler.cc #
Dependent Patchsets: Messages
Total messages: 31 (21 generated)
Description was changed from ========== Use keys 1 through 3 to switch between and click on focusable elements. BUG=593885 ========== to ========== Use keys 1 through 3 to switch between and click on focusable elements. BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by elichtenberg@google.com to run a CQ dry run
elichtenberg@google.com changed reviewers: + dmazzoni@chromium.org, dtseng@chromium.org
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/2738893003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/switch_access/switch_access.js:33: if (event.key === "1") { Use a switch instead of several if statements Maybe a second switch if debuggingEnabled is true https://codereview.chromium.org/2738893003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/switch_access/switch_access.js:189: if (this.isInteresting_(child)) { Right now you have calls to isInteresting throughout the code and you have "Interesting" in most of the helper function names. I think it'd be cleaner if you had a simple getNextNode() and getDescendant() and so on. Then to get the next interesting node, all you need to do is: let next = getNextNode(next); while (next && !isInteresting(next)) next = getNextNode(next); Then you'd just have isInteresting in one or two places rather than several places.
https://codereview.chromium.org/2738893003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/switch_access/switch_access.js:33: if (event.key === "1") { On 2017/03/09 00:00:01, dmazzoni wrote: > Use a switch instead of several if statements > > Maybe a second switch if debuggingEnabled is true > Rather than having two separate switch statements based on whether debuggingEnabled is true, I just have a second switch statement for the debug keys. It prevents a lot of duplicate code, but the downside is that when debugging is enabled, keys 1, 2, and 3 will still check all of the cases of the second switch statement. Is this okay? https://codereview.chromium.org/2738893003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/switch_access/switch_access.js:189: if (this.isInteresting_(child)) { On 2017/03/09 00:00:01, dmazzoni wrote: > Right now you have calls to isInteresting throughout the code and you have > "Interesting" in most of the helper function names. > > I think it'd be cleaner if you had a simple getNextNode() and getDescendant() > and so on. > > Then to get the next interesting node, all you need to do is: > > let next = getNextNode(next); > while (next && !isInteresting(next)) > next = getNextNode(next); > > Then you'd just have isInteresting in one or two places rather than several > places. Done.
The CQ bit was checked by elichtenberg@google.com 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/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (left): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:26: document.addEventListener("keyup", function(event) { nit: single quotes for string literals i.e. 'keyup' https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:56: this.printDetails_(); No need to have a special variant if you have a custom logger. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:47: if (debuggingEnabled) { Consider using a custom logger that only logs if the debug flag is set e.g. Switch.log = function(...) {} https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:219: if (!descendant) { |node| is called by sharing, so no need to use another binding for it. If (!node.lastChild) return null; while (node.lastChild) node = node.lastChild; return node; https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:221: } No curlies for single lined if bodies https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:236: if (node.state && node.state.focusable) { return node.state && node.state.focusable; node.state should always be defined too, so node.state.focusable would suffice. Also, note that focusable nodes can have focusable ancestors.
The CQ bit was checked by elichtenberg@google.com 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/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (left): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:26: document.addEventListener("keyup", function(event) { On 2017/03/09 20:57:40, David Tseng wrote: > nit: single quotes for string literals i.e. 'keyup' Done. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:56: this.printDetails_(); On 2017/03/09 20:57:40, David Tseng wrote: > No need to have a special variant if you have a custom logger. Talked offline about this. Keeping debug functions for now, but might remove them once switch access is developed further. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:47: if (debuggingEnabled) { On 2017/03/09 20:57:40, David Tseng wrote: > Consider using a custom logger that only logs if the debug flag is set e.g. > Switch.log = function(...) {} Will do in separate CL. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:219: if (!descendant) { On 2017/03/09 20:57:40, David Tseng wrote: > |node| is called by sharing, so no need to use another binding for it. > > If (!node.lastChild) > return null; > > while (node.lastChild) > node = node.lastChild; > > return node; Done. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:221: } On 2017/03/09 20:57:40, David Tseng wrote: > No curlies for single lined if bodies Done. https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:236: if (node.state && node.state.focusable) { On 2017/03/09 20:57:40, David Tseng wrote: > return node.state && node.state.focusable; > > node.state should always be defined too, so > node.state.focusable would suffice. > > Also, note that focusable nodes can have focusable ancestors. Done. According to externs, node.state can be null, so doing: node.state && node.state.focusable
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:236: if (node.state && node.state.focusable) { > > Also, note that focusable nodes can have focusable ancestors. To clarify, from observing many pages, it is usually more useful to consider leaf focusable nodes as "interesting". i.e. a node is interesting only if it is focusable and has no focusable descendants. Just FYI. For example, <div tabindex=0> <div tabindex=0> <div tabindex=0> <button></button>... will mean you're visiting all of the containing divs. https://codereview.chromium.org/2738893003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc (left): https://codereview.chromium.org/2738893003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:35: LOG(ERROR) << "Dispatching key " << key_code - ui::VKEY_0 Remove debug logging or VLOG it; this will show up in release builds.
https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/switch_access/switch_access.js (right): https://codereview.chromium.org/2738893003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/switch_access/switch_access.js:236: if (node.state && node.state.focusable) { On 2017/03/10 18:09:22, David Tseng wrote: > > > > Also, note that focusable nodes can have focusable ancestors. > > > To clarify, from observing many pages, it is usually more useful to consider > leaf focusable nodes as "interesting". i.e. a node is interesting only if it is > focusable and has no focusable descendants. Just FYI. > For example, > <div tabindex=0> > <div tabindex=0> > <div tabindex=0> > <button></button>... > > will mean you're visiting all of the containing divs. Thanks for clarifying. That makes sense. I'll account for this once I handle other things that might make a node "interesting" (e.g., links and buttons that currently aren't considered focusable). https://codereview.chromium.org/2738893003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc (left): https://codereview.chromium.org/2738893003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:35: LOG(ERROR) << "Dispatching key " << key_code - ui::VKEY_0 On 2017/03/10 18:09:22, David Tseng wrote: > Remove debug logging or VLOG it; this will show up in release builds. Done.
The CQ bit was checked by elichtenberg@google.com 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...
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 elichtenberg@google.com
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": 60001, "attempt_start_ts": 1489179901617460,
"parent_rev": "ac5c34a5e762fd095bf129de24512100be09872a", "commit_rev":
"e2a3dd0d63dd994aaad58f8818f0ed075b453834"}
Message was sent while issue was closed.
Description was changed from ========== Use keys 1 through 3 to switch between and click on focusable elements. BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Use keys 1 through 3 to switch between and click on focusable elements. BUG=593885 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2738893003 Cr-Commit-Position: refs/heads/master@{#456161} Committed: https://chromium.googlesource.com/chromium/src/+/e2a3dd0d63dd994aaad58f8818f0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e2a3dd0d63dd994aaad58f8818f0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
