Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1437523005:
Custom buttons should only handle accelerators when focused. (Closed)
DescriptionCustom buttons should only handle accelerators when focused.
BUG=541415
Committed: https://crrev.com/297ae873b471a46929ea39697b121c0b411434ee
Cr-Commit-Position: refs/heads/master@{#360130}
Patch Set 1 #
Total comments: 4
Patch Set 2 : msw comments #
Total comments: 4
Patch Set 3 : msw comments #
Total comments: 3
Patch Set 4 : sky comments #Patch Set 5 : Rebase & style #
Total comments: 2
Patch Set 6 : Add comment #
Messages
Total messages: 23 (4 generated)
meacer@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:254: if (!HasFocus()) This isn't quite right... you want to check if !GetWidget()->IsActive() The accelerator key should work even if another control is focused. (ie. Edit Bookmark; focus the 'Name' textfield; hit [Enter] to "Save") Maybe also bail or continue if GetWidget() is null for tests, etc. https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button_unittest.cc:208: // Shouldn't handle accelerators when not focused. Update the test for the behavior I explained in my other comment. It should be an interactive_ui_test, since it depends on activation. (or maybe use a TestWidget subclass with an |active| flag/accessors)
https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:254: if (!HasFocus()) On 2015/11/10 22:48:34, msw wrote: > This isn't quite right... you want to check if !GetWidget()->IsActive() > The accelerator key should work even if another control is focused. > (ie. Edit Bookmark; focus the 'Name' textfield; hit [Enter] to "Save") > Maybe also bail or continue if GetWidget() is null for tests, etc. Done. https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1437523005/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button_unittest.cc:208: // Shouldn't handle accelerators when not focused. On 2015/11/10 22:48:34, msw wrote: > Update the test for the behavior I explained in my other comment. > It should be an interactive_ui_test, since it depends on activation. > (or maybe use a TestWidget subclass with an |active| flag/accessors) Added TestWidget class here.
lgtm with a nit https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:102: scoped_ptr<TestWidget> widget_; Hmm, I worry slightly about making this a TestWidget, in case any other button code tested here relies on the IsActive() state of its Widget, which is now clamped to |false| by default. But nothing in a unit test should actually rely on widget activation state (without a helper like this), so this is fine. https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:222: button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE)); nit: EXPECT_FALSE(widget()->IsActive()) just before this.
meacer@chromium.org changed reviewers: + sky@chromium.org
+sky as owner https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:102: scoped_ptr<TestWidget> widget_; On 2015/11/11 00:55:43, msw wrote: > Hmm, I worry slightly about making this a TestWidget, in case any other button > code tested here relies on the IsActive() state of its Widget, which is now > clamped to |false| by default. But nothing in a unit test should actually rely > on widget activation state (without a helper like this), so this is fine. Acknowledged. https://codereview.chromium.org/1437523005/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:222: button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE)); On 2015/11/11 00:55:43, msw wrote: > nit: EXPECT_FALSE(widget()->IsActive()) just before this. Done.
https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:254: if (GetWidget() && !GetWidget()->IsActive()) Actually, how is the button getting the accelerator if the widget isn't active?
https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:254: if (GetWidget() && !GetWidget()->IsActive()) On 2015/11/11 18:54:01, sky wrote: > Actually, how is the button getting the accelerator if the widget isn't active? One of the failing tests has this scenario: ConstrainedWindowViewTest::ClosesOnEscape tries to close the dialog, but the widget is not active when the keypress is sent. Not sure if this is desired behavior. Should I go around and fix the tests by explicitly activating the widgets?
On 2015/11/12 00:37:32, Mustafa Emre Acer wrote: > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > File ui/views/controls/button/custom_button.cc (right): > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > ui/views/controls/button/custom_button.cc:254: if (GetWidget() && > !GetWidget()->IsActive()) > On 2015/11/11 18:54:01, sky wrote: > > Actually, how is the button getting the accelerator if the widget isn't > active? > > One of the failing tests has this scenario: > ConstrainedWindowViewTest::ClosesOnEscape tries to close the dialog, but the > widget is not active when the keypress is sent. > > Not sure if this is desired behavior. Should I go around and fix the tests by > explicitly activating the widgets? I think Scott is wondering how the underlying production code tries to have an accelerator be handled within an inactive widget. As for the failing test, perhaps make the default activation true, or only use TestWidget in your specific test fixture.
On 2015/11/12 00:45:44, msw wrote: > On 2015/11/12 00:37:32, Mustafa Emre Acer wrote: > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > File ui/views/controls/button/custom_button.cc (right): > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > ui/views/controls/button/custom_button.cc:254: if (GetWidget() && > > !GetWidget()->IsActive()) > > On 2015/11/11 18:54:01, sky wrote: > > > Actually, how is the button getting the accelerator if the widget isn't > > active? > > > > One of the failing tests has this scenario: > > ConstrainedWindowViewTest::ClosesOnEscape tries to close the dialog, but the > > widget is not active when the keypress is sent. > > > > Not sure if this is desired behavior. Should I go around and fix the tests by > > explicitly activating the widgets? > > I think Scott is wondering how the underlying production code tries to have an > accelerator be handled within an inactive widget. > As for the failing test, perhaps make the default activation true, or only use > TestWidget in your specific test fixture. Thanks for pointing this out. Looks like this fix isn't correct because the newly created login window isn't set as active in the first place. That means this fix breaks handling of all accelerators (e.g. ESC doesn't close the dialog anymore), since IsActive() for the dialog is always false. Not being very familiar with the UI code, I don't know if we expect the newly created window to be activated too. I dug into focusing and activating code a bit, and it looks like only top level windows can be activated, which the newly created modal login dialog is not. Should it be activated? As for accelerator handling, CustomButton defers CanHandleAccelerators to View which doesn't check for activity status. That seems to be low level code which I assume works correctly, or should it check for the widget being active?
On 2015/11/12 21:11:40, Mustafa Emre Acer wrote: > On 2015/11/12 00:45:44, msw wrote: > > On 2015/11/12 00:37:32, Mustafa Emre Acer wrote: > > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > > File ui/views/controls/button/custom_button.cc (right): > > > > > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > > ui/views/controls/button/custom_button.cc:254: if (GetWidget() && > > > !GetWidget()->IsActive()) > > > On 2015/11/11 18:54:01, sky wrote: > > > > Actually, how is the button getting the accelerator if the widget isn't > > > active? > > > > > > One of the failing tests has this scenario: > > > ConstrainedWindowViewTest::ClosesOnEscape tries to close the dialog, but the > > > widget is not active when the keypress is sent. > > > > > > Not sure if this is desired behavior. Should I go around and fix the tests > by > > > explicitly activating the widgets? > > > > I think Scott is wondering how the underlying production code tries to have an > > accelerator be handled within an inactive widget. > > As for the failing test, perhaps make the default activation true, or only use > > TestWidget in your specific test fixture. > > Thanks for pointing this out. Looks like this fix isn't correct because the > newly created login window isn't set as active in the first place. That means > this fix breaks handling of all accelerators (e.g. ESC doesn't close the dialog > anymore), since IsActive() for the dialog is always false. > > Not being very familiar with the UI code, I don't know if we expect the newly > created window to be activated too. I dug into focusing and activating code a > bit, and it looks like only top level windows can be activated, which the newly > created modal login dialog is not. Should it be activated? > > As for accelerator handling, CustomButton defers CanHandleAccelerators to View > which doesn't check for activity status. That seems to be low level code which I > assume works correctly, or should it check for the widget being active? Scott may have a better suggestion and would know if there's something more fundamentally wrong with the state of affairs. My naive suggestion is to test if focus is on a descendant of the button's widget, but that seems like a hacky workaround. It'd be nice to also test if the top-level window of the button (ie. the dialog's parent?) is active, but I'm not sure that's entirely feasible... I'll ask Scott to weigh in.
I was thinking we should block this at the accelerator dispatching level. By which I don't call accelerators on inactive widgets. But that could likely break things. So, LGTM https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:56: bool IsActive() const override { return active_; } nit: generally we prefix overrides with what class they are overriding. See custom_button.h for an example of what I mean.
On 2015/11/12 23:42:57, sky wrote: > I was thinking we should block this at the accelerator dispatching level. By > which I don't call accelerators on inactive widgets. But that could likely break > things. So, LGTM > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > File ui/views/controls/button/custom_button_unittest.cc (right): > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > ui/views/controls/button/custom_button_unittest.cc:56: bool IsActive() const > override { return active_; } > nit: generally we prefix overrides with what class they are overriding. See > custom_button.h for an example of what I mean. Sorry, maybe I wasn't clear: the CL isn't correct because it disables all accelerators on the login dialog and other modal dialogs. This is because the login dialog isn't marked as inactive even when it's displayed and in focus, so esc, enter keys etc. don't work at all.
On 2015/11/12 23:51:47, Mustafa Emre Acer wrote: > On 2015/11/12 23:42:57, sky wrote: > > I was thinking we should block this at the accelerator dispatching level. By > > which I don't call accelerators on inactive widgets. But that could likely > break > > things. So, LGTM > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > File ui/views/controls/button/custom_button_unittest.cc (right): > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > ui/views/controls/button/custom_button_unittest.cc:56: bool IsActive() const > > override { return active_; } > > nit: generally we prefix overrides with what class they are overriding. See > > custom_button.h for an example of what I mean. > > Sorry, maybe I wasn't clear: the CL isn't correct because it disables all > accelerators on the login dialog and other modal dialogs. This is because the > login dialog isn't marked as inactive even when it's displayed and in focus, so > esc, enter keys etc. don't work at all. Ugh! That's because it's a child widget, which are never 'active'. I think you'll need to check the top level widget too, eg: I think you want something like if ((InChildWidget() && !FocusInChildWidget()) || (!InChildWidget() && !GetWidget()->IsActive()) return; InChildWidget is GetWidget() && GetWidget()->GetTopLevelWidget() != GetWidget() for FocusInChildWidget you have to go to focusmanager and see if the focused view is contained in the rootview of the widget. Or make active status work correctly. The problem is from the systems perspective the top level widget is the active one, and making that inactive likely has problems.
On 2015/11/13 01:07:45, sky wrote: > On 2015/11/12 23:51:47, Mustafa Emre Acer wrote: > > On 2015/11/12 23:42:57, sky wrote: > > > I was thinking we should block this at the accelerator dispatching level. By > > > which I don't call accelerators on inactive widgets. But that could likely > > break > > > things. So, LGTM > > > > > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > > File ui/views/controls/button/custom_button_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1437523005/diff/40001/ui/views/controls/butto... > > > ui/views/controls/button/custom_button_unittest.cc:56: bool IsActive() const > > > override { return active_; } > > > nit: generally we prefix overrides with what class they are overriding. See > > > custom_button.h for an example of what I mean. > > > > Sorry, maybe I wasn't clear: the CL isn't correct because it disables all > > accelerators on the login dialog and other modal dialogs. This is because the > > login dialog isn't marked as inactive even when it's displayed and in focus, > so > > esc, enter keys etc. don't work at all. > > Ugh! That's because it's a child widget, which are never 'active'. I think > you'll need to check the top level widget too, eg: > > I think you want something like > > if ((InChildWidget() && !FocusInChildWidget()) || (!InChildWidget() && > !GetWidget()->IsActive()) > return; > > InChildWidget is GetWidget() && GetWidget()->GetTopLevelWidget() != GetWidget() > for FocusInChildWidget you have to go to focusmanager and see if the focused > view is contained in the rootview of the widget. I went with this approach. Can you please take another look? > > Or make active status work correctly. The problem is from the systems > perspective the top level widget is the active one, and making that inactive > likely has problems.
LGTM https://codereview.chromium.org/1437523005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:254: if ((IsChildWidget() && !FocusInChildWidget()) || This is subtle enough that it is worth a comment.
Thanks. https://codereview.chromium.org/1437523005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1437523005/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:254: if ((IsChildWidget() && !FocusInChildWidget()) || On 2015/11/17 14:22:39, sky wrote: > This is subtle enough that it is worth a comment. Done.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1437523005/#ps100001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1437523005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437523005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/297ae873b471a46929ea39697b121c0b411434ee Cr-Commit-Position: refs/heads/master@{#360130} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
