Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1671323002:
[UI Views] Handle accelerators being pressed for buttons inside a menu (Closed)
Description[UI Views] Handle accelerators being pressed for buttons inside a menu
CustomButton::AcceleratorPressed checks that the button's widget is active,
but this won't be true for a button hosted inside a menu. Add support for
in-menu buttons being triggered by the keyboard, and add a test.
BUG=584771
Committed: https://crrev.com/0cb6b42e4445363a5e89de6e8cf1ba15c46a0642
Cr-Commit-Position: refs/heads/master@{#374245}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : Latest master #
Messages
Total messages: 25 (11 generated)
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
Scott, mind taking a look?
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/1671323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/1671323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:77: // TODO(devlin): Shouldn't this be one key down event? This will be fixed with https://codereview.chromium.org/1661673004/. Thanks for pointing this! I will update this test if it lands first. https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button.cc:284: Activate(); why not "return Activate();"? https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button.h:81: bool AcceleratorPressed(const ui::Accelerator& accelerator) override; nit: should be above GetAccessibleState in view.h order.
https://codereview.chromium.org/1671323002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc (right): https://codereview.chromium.org/1671323002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc:77: // TODO(devlin): Shouldn't this be one key down event? On 2016/02/06 00:58:08, varkha wrote: > This will be fixed with https://codereview.chromium.org/1661673004/. Thanks for > pointing this! I will update this test if it lands first. Oh, nice! I was going to look into that next. Glad you're on top of it! https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button.cc:284: Activate(); On 2016/02/06 00:58:08, varkha wrote: > why not "return Activate();"? The return value of Activate() is designed for mouse events, and is really odd (e.g., it returns false if it processes the event and true if it doesn't). CustomButton::AcceleratorPressed() pretty much just eats the event as long as it's reasonable, so I think we can take that approach. But if we'd prefer to do some more checking here, I'm open to it. https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/1671323002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button.h:81: bool AcceleratorPressed(const ui::Accelerator& accelerator) override; On 2016/02/06 00:58:08, varkha wrote: > nit: should be above GetAccessibleState in view.h order. Huh. I didn't know we had a style rule for matching inherited methods to the order they are declared in the super class (only for matching declaration and definition order). I'm pretty sure most of our classes are quite wrong for that style. :) (But done - super easy change and no reason not to)
Patchset #1 (id:1) has been deleted
Will https://codereview.chromium.org/1544803004/ fix this.
On 2016/02/08 18:09:30, sky wrote: > Will https://codereview.chromium.org/1544803004/ fix this. No - but close. https://codereview.chromium.org/1544803004/ addresses one concern, which is that the button doesn't really ever have "focus" in the menu, but doesn't address the fact that CustomButton::AcceleratorPressed() forwards to NotifyClick(), rather than MenuButton::Activate(). This change makes it go to the right place (and is analogous with the rest of MenuButton - e.g. mouse events to go to Activate() rather than NotifyClick()).
On 2016/02/08 18:23:36, Devlin wrote: > On 2016/02/08 18:09:30, sky wrote: > > Will https://codereview.chromium.org/1544803004/ fix this. > > No - but close. https://codereview.chromium.org/1544803004/ addresses one > concern, which is that the button doesn't really ever have "focus" in the menu, > but doesn't address the fact that CustomButton::AcceleratorPressed() forwards to > NotifyClick(), rather than MenuButton::Activate(). This change makes it go to > the right place (and is analogous with the rest of MenuButton - e.g. mouse > events to go to Activate() rather than NotifyClick()). On a separate-but-related note, I've always wondered why MenuButtons have a custom listener, and have looked through the codebase and not really found any place we need it. Is there a reason to not just provide a button listener and clean up a bit of this weirdness? (Note that I'd still want this to go in in the short term, since that would be a much bigger change and I don't know when I'd get to it.)
LGTM - Not sure on MenuButtonListener. If you can nuke it and use ButtonListener, go for it.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1671323002/#ps60001 (title: "Latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671323002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [UI Views] Handle accelerators being pressed for buttons inside a menu CustomButton::AcceleratorPressed checks that the button's widget is active, but this won't be true for a button hosted inside a menu. Add support for in-menu buttons being triggered by the keyboard, and add a test. BUG=584771 ========== to ========== [UI Views] Handle accelerators being pressed for buttons inside a menu CustomButton::AcceleratorPressed checks that the button's widget is active, but this won't be true for a button hosted inside a menu. Add support for in-menu buttons being triggered by the keyboard, and add a test. BUG=584771 Committed: https://crrev.com/0cb6b42e4445363a5e89de6e8cf1ba15c46a0642 Cr-Commit-Position: refs/heads/master@{#374245} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0cb6b42e4445363a5e89de6e8cf1ba15c46a0642 Cr-Commit-Position: refs/heads/master@{#374245} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
