Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947013002:
TabBox will now select hidden tabs through arrow keys.
DescriptionTabBox will now select hidden tabs through arrow keys.
BUG=734917
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 5
Patch Set 2 : Updates for calamity. #Messages
Total messages: 16 (10 generated)
Description was changed from ========== TabBox will now select hidden tabs through arrow keys. BUG=734917 ========== to ========== TabBox will now select hidden tabs through arrow keys. BUG=734917 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by skym@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...
skym@chromium.org changed reviewers: + calamity@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we write a test for this somewhere? https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/tabs.js:177: // it's marked as hidden. We not want to infinitely loop here. I think this comment is too specific. // Select next visible tab. Does nothing if no other tabs are visible. https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/tabs.js:178: while (tabbox.querySelector('tabs').children[next].hidden && How about var next; do { next = (next + delta + count) % count; } while (!this.children[next].offsetParent && index != next);
The CQ bit was checked by skym@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...
Can you point me to an example of a test that you think would be best for testing this scenario? The two webui tests I can find seem to do things quite differently. skym@skym:~/chromium/linux/src$ find ui/webui/* | grep test ui/webui/resources/js/cr/ui/menu_test.html ui/webui/resources/js/webui_resource_test.js https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/tabs.js:177: // it's marked as hidden. We not want to infinitely loop here. On 2017/06/21 07:22:31, calamity wrote: > I think this comment is too specific. > > // Select next visible tab. Does nothing if no other tabs are visible. Done. https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/tabs.js:178: while (tabbox.querySelector('tabs').children[next].hidden && On 2017/06/21 07:22:31, calamity wrote: > How about > > var next; > do { > next = (next + delta + count) % count; > } while (!this.children[next].offsetParent && index != next); > Switched to do/while and this.children. I don't see why you'd want to check offsetParent instead of hidden, it is so much less readable.
On 2017/06/21 17:00:36, skym wrote: > Can you point me to an example of a test that you think would be best for > testing this scenario? The two webui tests I can find seem to do things quite > differently. > > skym@skym:~/chromium/linux/src$ find ui/webui/* | grep test > ui/webui/resources/js/cr/ui/menu_test.html > ui/webui/resources/js/webui_resource_test.js > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > File ui/webui/resources/js/cr/ui/tabs.js (right): > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > ui/webui/resources/js/cr/ui/tabs.js:177: // it's marked as hidden. We not want > to infinitely loop here. > On 2017/06/21 07:22:31, calamity wrote: > > I think this comment is too specific. > > > > // Select next visible tab. Does nothing if no other tabs are visible. > > Done. > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > ui/webui/resources/js/cr/ui/tabs.js:178: while > (tabbox.querySelector('tabs').children[next].hidden && > On 2017/06/21 07:22:31, calamity wrote: > > How about > > > > var next; > > do { > > next = (next + delta + count) % count; > > } while (!this.children[next].offsetParent && index != next); > > > > Switched to do/while and this.children. I don't see why you'd want to check > offsetParent instead of hidden, it is so much less readable. Hmm, doesn't look like webui_resource_test.js is an actual unit test. Am i correct to assume that doing something similar menu_test.html is probably the best approach for this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/06/21 17:06:29, skym wrote: > On 2017/06/21 17:00:36, skym wrote: > > Can you point me to an example of a test that you think would be best for > > testing this scenario? The two webui tests I can find seem to do things quite > > differently. > > > > skym@skym:~/chromium/linux/src$ find ui/webui/* | grep test > > ui/webui/resources/js/cr/ui/menu_test.html > > ui/webui/resources/js/webui_resource_test.js > > > > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > > File ui/webui/resources/js/cr/ui/tabs.js (right): > > > > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > > ui/webui/resources/js/cr/ui/tabs.js:177: // it's marked as hidden. We not want > > to infinitely loop here. > > On 2017/06/21 07:22:31, calamity wrote: > > > I think this comment is too specific. > > > > > > // Select next visible tab. Does nothing if no other tabs are visible. > > > > Done. > > > > > https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... > > ui/webui/resources/js/cr/ui/tabs.js:178: while > > (tabbox.querySelector('tabs').children[next].hidden && > > On 2017/06/21 07:22:31, calamity wrote: > > > How about > > > > > > var next; > > > do { > > > next = (next + delta + count) % count; > > > } while (!this.children[next].offsetParent && index != next); > > > > > > > Switched to do/while and this.children. I don't see why you'd want to check > > offsetParent instead of hidden, it is so much less readable. > > Hmm, doesn't look like webui_resource_test.js is an actual unit test. Am i > correct to assume that doing something similar menu_test.html is probably the > best approach for this? Sorry that the task of setting up the test harness falls to you. Have a look at load_time_data_browsertest.js. You'll need to make a tabs.html at ui/webui/resources/html/cr/ui/ which has the dependencies of tabs.js listed. cr_action_menu_test.js will show you how to deal with actions and HTML elements in a test.
https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/2947013002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/tabs.js:178: while (tabbox.querySelector('tabs').children[next].hidden && On 2017/06/21 17:00:36, skym wrote: > On 2017/06/21 07:22:31, calamity wrote: > > How about > > > > var next; > > do { > > next = (next + delta + count) % count; > > } while (!this.children[next].offsetParent && index != next); > > > > Switched to do/while and this.children. I don't see why you'd want to check > offsetParent instead of hidden, it is so much less readable. offsetParent deals with display: none in general (the behavior of hidden is actually defined in local stylesheets). There's a tradeoff between readability and consistency here. |
