Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374483002:
[tracing] Add keyboard navigation to the tab view (Closed)
Description[tracing] Add keyboard navigation to the tab view
BUG=catapult:#2750
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/dae0bc0c962f7c2c3219ba5642df4f91b642c200
Patch Set 1 #
Total comments: 10
Patch Set 2 : update for comments #
Total comments: 2
Patch Set 3 : add switch #Messages
Total messages: 19 (10 generated)
hjd@chromium.org changed reviewers: + petrcermak@chromium.org
hjd@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks!
Looks good overall. Just a couple of comments. Could you please add a "BUG=catapult:#2750" line to the patch description (it refers to this bug: https://github.com/catapult-project/catapult/issues/2750)? Thanks, Petr https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:121: focusable_: { I don't think there's any value in adding a private 'focusable_' attribute here. After all, you don't actually use it anywhere. You just set it in updateFocusability_. Please remove it. https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:220: tab.firstElementChild.click(); To prepare for the structure of the tab changing in the future, let's use querySelector instead: tab.querySelector('input').click(); https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:220: tab.firstElementChild.click(); Could you try doing .checked = true? It would be better than simulating a click, in my opinion. If that doesn't work, then let's keep it as is ;-) https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:231: var focusableNow = this.shouldBeFocusable_(); as explained in one of my previous comments, I think you should simplify this: if (this.shouldBeFocusable_()) { ... } else { ... } https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:232: if (this.get('focusable_') === focusableNow) BTW, you could just do `this.focusable_` here and `this.focusable_ = focusableNow` below (however, I wouldn't do that).
Patch title/description nit: I prefer NOT appending "s" to the verb in the title. Looking at the most recent commits, they don't use it either. Please replace s/Adds/Add/ in the patch title and the first line of the description. Thanks, Petr
Description was changed from ========== [Tracing] Adds keyboard navigation to the tab view ========== to ========== [tracing] Adds keyboard navigation to the tab view ==========
Description was changed from ========== [tracing] Adds keyboard navigation to the tab view ========== to ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:#2750 ==========
Description was changed from ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:#2750 ========== to ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:2750 ==========
Description was changed from ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:2750 ========== to ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:#2750 ==========
Description was changed from ========== [tracing] Adds keyboard navigation to the tab view BUG=catapult:#2750 ========== to ========== [tracing] Add keyboard navigation to the tab view BUG=catapult:#2750 ==========
On 2016/09/27 13:42:45, petrcermak wrote: > Patch title/description nit: I prefer NOT appending "s" to the verb in the > title. Looking at the most recent commits, they don't use it either. Please > replace s/Adds/Add/ in the patch title and the first line of the description. > > Thanks, > Petr Done.
Thanks! https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:121: focusable_: { On 2016/09/27 13:33:06, petrcermak wrote: > I don't think there's any value in adding a private 'focusable_' attribute here. > After all, you don't actually use it anywhere. You just set it in > updateFocusability_. Please remove it. Done. https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:220: tab.firstElementChild.click(); On 2016/09/27 13:33:06, petrcermak wrote: > To prepare for the structure of the tab changing in the future, let's use > querySelector instead: > > tab.querySelector('input').click(); Done. https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:220: tab.firstElementChild.click(); On 2016/09/27 13:33:06, petrcermak wrote: > Could you try doing .checked = true? It would be better than simulating a click, > in my opinion. If that doesn't work, then let's keep it as is ;-) I tried it but it didn't seem to work :( I'll leave it as is. https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:231: var focusableNow = this.shouldBeFocusable_(); On 2016/09/27 13:33:06, petrcermak wrote: > as explained in one of my previous comments, I think you should simplify this: > > if (this.shouldBeFocusable_()) { > ... > } else { > ... > } Done. https://codereview.chromium.org/2374483002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:232: if (this.get('focusable_') === focusableNow) On 2016/09/27 13:33:06, petrcermak wrote: > BTW, you could just do `this.focusable_` here and `this.focusable_ = > focusableNow` below (however, I wouldn't do that). Acknowledged.
LGTM with one final comment. After this and the following two patches land: * https://codereview.chromium.org/2373913003/ * https://codereview.chromium.org/2375143002/ the new heap dump UI should work with the keyboard flawlessly :-) Thanks, Petr https://codereview.chromium.org/2374483002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2374483002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:211: if (nextIndex === undefined) this should be a `default` case in the `switch` statement above instead.
Thanks! https://codereview.chromium.org/2374483002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2374483002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:211: if (nextIndex === undefined) On 2016/09/28 17:26:30, petrcermak wrote: > this should be a `default` case in the `switch` statement above instead. Done.
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2374483002/#ps40001 (title: "add switch")
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.
Description was changed from ========== [tracing] Add keyboard navigation to the tab view BUG=catapult:#2750 ========== to ========== [tracing] Add keyboard navigation to the tab view BUG=catapult:#2750 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
