Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803543003:
WebUI: fix cr-action-menu keyboard off-by-one issue (Closed)
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 8 months ago Reviewers:
dpapad CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: fix cr-action-menu keyboard off-by-one issue
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2803543003
Cr-Commit-Position: refs/heads/master@{#462732}
Committed: https://chromium.googlesource.com/chromium/src/+/aee0c7172b135b405d7b0b8f8a92077e02052ff6
Patch Set 1 #Patch Set 2 : add test #
Total comments: 6
Patch Set 3 : updates based on comments #Patch Set 4 : merge #
Depends on Patchset: Messages
Total messages: 24 (19 generated)
Description was changed from ========== MD Settings: fix cr-action-menu keyboard off-by-one issue BUG=None ========== to ========== MD Settings: fix cr-action-menu keyboard off-by-one issue BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cr-action-menu is reused outside MD Settings, so you can prefix your CL with "WebUI" instead of "MD Settigs". LGTM https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:97: menu.showAt(document.querySelector('#dots')); assertEquals(menu, document.activeElement); https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:100: assertEquals(items[2], menu.root.activeElement); Nit (optional): Make it more explicit that we expect the last item to be focused. assertEquals(items[items.length - 1], menu.root.activeElement); https://codereview.chromium.org/2803543003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2803543003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:127: // Avoid off-by-one when nothing is focused and up is pressed. Nit (optional): Perhaps just rephrase to // Handle case where nothing is focused and up is pressed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MD Settings: fix cr-action-menu keyboard off-by-one issue BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: fix cr-action-menu keyboard off-by-one issue BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:97: menu.showAt(document.querySelector('#dots')); On 2017/04/05 21:49:13, dpapad wrote: > assertEquals(menu, document.activeElement); Done. https://codereview.chromium.org/2803543003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:100: assertEquals(items[2], menu.root.activeElement); On 2017/04/05 21:49:13, dpapad wrote: > Nit (optional): Make it more explicit that we expect the last item to be > focused. > > assertEquals(items[items.length - 1], menu.root.activeElement); Done. https://codereview.chromium.org/2803543003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2803543003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:127: // Avoid off-by-one when nothing is focused and up is pressed. On 2017/04/05 21:49:13, dpapad wrote: > Nit (optional): Perhaps just rephrase to > // Handle case where nothing is focused and up is pressed. Done.
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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scottchen@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...
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 scottchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2803543003/#ps60001 (title: "merge")
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": 1491525529403310,
"parent_rev": "15f5b51d19bd375b45ca8f9f8377b0522f261395", "commit_rev":
"aee0c7172b135b405d7b0b8f8a92077e02052ff6"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: fix cr-action-menu keyboard off-by-one issue BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: fix cr-action-menu keyboard off-by-one issue BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2803543003 Cr-Commit-Position: refs/heads/master@{#462732} Committed: https://chromium.googlesource.com/chromium/src/+/aee0c7172b135b405d7b0b8f8a92... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/aee0c7172b135b405d7b0b8f8a92... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
