Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2850753002:
Prevent opening context menu with all items hidden. (Closed)
DescriptionPrevent opening context menu with all items hidden.
TEST=none
TEST=manually tested as noted in the bug
BUG=716322
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2850753002
Cr-Commit-Position: refs/heads/master@{#468531}
Committed: https://chromium.googlesource.com/chromium/src/+/bce3863fc28dc7a2b05eb37a1055b2d43f8002a8
Patch Set 1 #Patch Set 2 : Use existing logic to determine whether the item is saparator or not. #
Total comments: 2
Messages
Total messages: 21 (14 generated)
Description was changed from ========== Prevent opening context menu with all items hidden. TEST=none TEST=manually tested as noted in the bug BUG=716322 ========== to ========== Prevent opening context menu with all items hidden. TEST=none TEST=manually tested as noted in the bug BUG=716322 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@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 yamaguchi@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.
yamaguchi@chromium.org changed reviewers: + yoshiki@chromium.org
Will you take a look? It seems the previous change around the line https://chromiumcodereview.appspot.com/22154002 has did something to prevent showing empty context menu (contains only separators), but it still shows completely invisible context menu. It takes keyboard focus, and it can and can invoke "create new directory" command unwillingly.
https://codereview.chromium.org/2850753002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/menu.js (right): https://codereview.chromium.org/2850753002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/menu.js:191: hasVisibleItems: function() { nit: I think renaming this to "hasVisibleMenuItem" is better for readability. But it;s up to you.
lgtm
yamaguchi@chromium.org changed reviewers: + michaelpg@chromium.org
+michaelpg Michael, will your review the change because this is on a webui component? https://codereview.chromium.org/2850753002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/menu.js (right): https://codereview.chromium.org/2850753002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/menu.js:191: hasVisibleItems: function() { On 2017/05/01 06:36:53, yoshiki wrote: > nit: I think renaming this to "hasVisibleMenuItem" is better for readability. > But it;s up to you. I guess it's been just "item" because the name of this class already contains "menu". I'll keep the name as it is now.
lgtm
The CQ bit was checked by yamaguchi@chromium.org
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": 20001, "attempt_start_ts": 1493686380598460,
"parent_rev": "d0206f11864ca3b7fd59e5f04a334c8c448349ed", "commit_rev":
"bce3863fc28dc7a2b05eb37a1055b2d43f8002a8"}
Message was sent while issue was closed.
Description was changed from ========== Prevent opening context menu with all items hidden. TEST=none TEST=manually tested as noted in the bug BUG=716322 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Prevent opening context menu with all items hidden. TEST=none TEST=manually tested as noted in the bug BUG=716322 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2850753002 Cr-Commit-Position: refs/heads/master@{#468531} Committed: https://chromium.googlesource.com/chromium/src/+/bce3863fc28dc7a2b05eb37a1055... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bce3863fc28dc7a2b05eb37a1055... |
