Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857893002:
[MD Bookmarks] Allow left/right keys to close/open folders in sidebar. (Closed)
Description[MD Bookmarks] Allow left/right keys to close/open folders in sidebar.
This CL makes the left and right arrows close and open folders in the
sidebar respectively. A closed folder will transfer selection to the
parent when left is pressed and an open folder will transfer selection
to its first child.
BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2857893002
Cr-Commit-Position: refs/heads/master@{#471199}
Committed: https://chromium.googlesource.com/chromium/src/+/872e2db0bbc390dc17223d85a48581549b8a0836
Patch Set 1 : #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : fix rtl test #
Messages
Total messages: 46 (39 generated)
Description was changed from ========== [MD Bookmarks] Allow left/right keys to close/open folders in sidebar. This CL makes the left and right arrows close and open folders in the sidebar respectively. A closed folder will transfer selection to the parent when left is pressed and an open folder will transfer selection to its first child. BUG=692844 ========== to ========== [MD Bookmarks] Allow left/right keys to close/open folders in sidebar. This CL makes the left and right arrows close and open folders in the sidebar respectively. A closed folder will transfer selection to the parent when left is pressed and an open folder will transfer selection to its first child. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by calamity@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by calamity@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.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by calamity@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_tsan_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 calamity@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
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.
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:67: :host-context([dir='rtl']) #arrow { This appears to work, but it should probably apply to #arrow-icon instead. Also, you could apply all these styles to `#arrow iron-icon` if you want to avoid adding a new ID. Up to you. https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:92: } else if (e.key == 'ArrowRight') { These shortcuts need to flip in RTL. https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:135: function keydown(id, key) { There's already a keydown function above (line 41). Can it be reused here? https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:143: // The selected folder is focus enabled on attach. Is there a particular reason to repeat these two assertions from the previous test? Just delete them otherwise
The CQ bit was checked by calamity@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by calamity@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: 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_...)
https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.html:67: :host-context([dir='rtl']) #arrow { On 2017/05/05 01:31:07, tsergeant wrote: > This appears to work, but it should probably apply to #arrow-icon instead. > > Also, you could apply all these styles to `#arrow iron-icon` if you want to > avoid adding a new ID. Up to you. Done. https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/folder_node.js (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/folder_node.js:92: } else if (e.key == 'ArrowRight') { On 2017/05/05 01:31:07, tsergeant wrote: > These shortcuts need to flip in RTL. Done. https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:135: function keydown(id, key) { On 2017/05/05 01:31:07, tsergeant wrote: > There's already a keydown function above (line 41). Can it be reused here? Done. Forgot to clean this up after rebase. https://codereview.chromium.org/2857893002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:143: // The selected folder is focus enabled on attach. On 2017/05/05 01:31:07, tsergeant wrote: > Is there a particular reason to repeat these two assertions from the previous > test? Just delete them otherwise Done.
lgtm https://codereview.chromium.org/2857893002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2857893002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:170: document.body.style.direction = 'rtl'; Nit: oh boy we'd better reset this after the test is done.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
https://codereview.chromium.org/2857893002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right): https://codereview.chromium.org/2857893002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:170: document.body.style.direction = 'rtl'; On 2017/05/09 04:43:42, tsergeant wrote: > Nit: oh boy we'd better reset this after the test is done. 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: This issue passed the CQ dry run.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2857893002/#ps120001 (title: "fix rtl test")
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": 120001, "attempt_start_ts": 1494557878520940,
"parent_rev": "46a4de44f2d04ac212c55fbc08fe17fd2da63845", "commit_rev":
"f010228590f676a1264aa0330f1258a7d9a67bb1"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494557878520940,
"parent_rev": "25aed983a45bceabfc3d82393a74facd2e86a1cf", "commit_rev":
"872e2db0bbc390dc17223d85a48581549b8a0836"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Allow left/right keys to close/open folders in sidebar. This CL makes the left and right arrows close and open folders in the sidebar respectively. A closed folder will transfer selection to the parent when left is pressed and an open folder will transfer selection to its first child. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Allow left/right keys to close/open folders in sidebar. This CL makes the left and right arrows close and open folders in the sidebar respectively. A closed folder will transfer selection to the parent when left is pressed and an open folder will transfer selection to its first child. BUG=692844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2857893002 Cr-Commit-Position: refs/heads/master@{#471199} Committed: https://chromium.googlesource.com/chromium/src/+/872e2db0bbc390dc17223d85a485... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/872e2db0bbc390dc17223d85a485... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
