Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607923002:
MacViews: Handle Space and Return keys correctly for Buttons. (Closed)
|
Created:
3 years, 11 months ago by karandeepb Modified:
3 years, 11 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Handle Space and Return keys correctly for Buttons.
Currently on MacViews, the Space key clicks the button on key-release and the
Return key clicks the button on key-press if the button has focus. However the
Cocoa behavior is that the Space key should click the button on key-press when a
button is focused. Also the Return key should perform the default action
associated with a dialog, irrespective of the view focused.
This CL introduces two new constants to PlatformStyle to account for the special
behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl.
CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and
CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of
these platform specific constants. Existing unit tests are modified to be in
line with the new behavior and one new test is also added.
BUG=607429, 607430
TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble.
Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space
clicks the button on key-press and Return performs the default action(Done)
instead of opening the Edit Bookmark Dialog.
Committed: https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23
Cr-Commit-Position: refs/heads/master@{#441622}
Patch Set 1 #Patch Set 2 : -- #Patch Set 3 : -- #Patch Set 4 : -- #
Total comments: 25
Patch Set 5 : Address review. #
Total comments: 10
Patch Set 6 : Address review. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 44 (36 generated)
Description was changed from ========== -- -- -- BUG= ========== to ========== -- -- -- BUG= ==========
The CQ bit was checked by karandeepb@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.
Description was changed from ========== -- -- -- BUG= ========== to ========== MacViews: Handle Space and Return correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG= 607429, 607430 ==========
Description was changed from ========== MacViews: Handle Space and Return correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG= 607429, 607430 ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG= 607429, 607430 ==========
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG= 607429, 607430 ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ==========
Patchset #4 (id:60001) has been deleted
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kButtonActionOnReturn. CustomButton::OnKeyPressed and CustomButton::OnKeyReleased are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kClickControlOnReturn. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessingare are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ==========
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kClickControlOnReturn. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessingare are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kClickControlOnReturn. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Also looked at CustomButton::OnKeyPressed overrides. Most of them were in ui/app_list, which I don't think we care about anymore. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:311: (event.key_code() == ui::VKEY_RETURN); I am not entirely sure about the reason behind this piece of code. This was added in https://codereview.chromium.org/1645013004. If this button is focused, it would have already consumed OnKeyEvent for space and enter, hence we wouldn't reach here. The only use-case I think this handles is when OnKey[Pressed/Released] are overriden by subclasses to not handle Space/Enter. On Mac, we want to handle Return key as an accelerator even if a button has focus. Also the comments for View::SkipDefaultKeyEventProcessing says: In that case(when true is returned), OnKeyPressed will subsequently be invoked for that event. This seems wrong to me. I think OnKeyPressed is invoked before, then SkipDefaultKeyEventProcessing is used by FocusManager::OnKeyEvent.
The CQ bit was checked by karandeepb@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.
https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:311: (event.key_code() == ui::VKEY_RETURN); On 2017/01/04 03:55:40, karandeepb wrote: > I am not entirely sure about the reason behind this piece of code. This was > added in https://codereview.chromium.org/1645013004. If this button is focused, > it would have already consumed OnKeyEvent for space and enter, hence we wouldn't > reach here. I think it would reach here if CustomButton::OnKeyPressed() returned false. I guess that's rare. perhaps a test was failing in that CL. > The only use-case I think this handles is when > OnKey[Pressed/Released] are overriden by subclasses to not handle Space/Enter. > On Mac, we want to handle Return key as an accelerator even if a button has > focus. On windows both space and return do something - I think we need something different here. > Also the comments for View::SkipDefaultKeyEventProcessing says: In that > case(when true is returned), OnKeyPressed will subsequently be invoked for that > event. This seems wrong to me. I think OnKeyPressed is invoked before, then > SkipDefaultKeyEventProcessing is used by FocusManager::OnKeyEvent. It says "Invoked [before handling] by the focus manager for tab traversal, accelerators and other focus related actions." - I think that's still correct. That is, yes: View::OnKeyPressed is invoked before, but FocusManager::OnKeyPressed [not an override] is invoked after. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:42: else if (event.key_code() == ui::VKEY_RETURN && nit: "no else after return" - https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:256: views::InkDropState::ACTION_PENDING) { views:: not needed, same below https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:261: break; nit: no break https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:266: break; nit: no break https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.h:32: // An enum describing the action to perform on a key press. since this deals with press and release, the comment doesn't sound right. Maybe // An enum describing how a given key code should affect the button state. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.h:34: ACTION_PRESS, // The button is pressed on OnKeyPressed and clicked on Perhaps CLICK_ON_PRESS / CLICK_ON_RELEASE / CLICK_NONE? (and I guess `ClickAction` would make it more consistent with the above too) https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:653: EXPECT_TRUE(button()->OnKeyPressed(space_press)); why this change? (won't this bypass accelerator handling?) https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... ui/views/style/platform_style.h:53: static const bool kClickControlOnReturn; I read "Control" and thought "Control+Return" - i.e. a key combination.. But do we need two constants? They seem to imply each other. e.g. can we have // Whether buttons react only to the Space key (when pressed). Otherwise buttons // react to Return when pressed, and Space when released. static const bool kButtonsClickOnSpaceOnly = is_mac; https://codereview.chromium.org/2607923002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:159: LabelButton* cancel_button = client_view->cancel_button(); can we keep the old tests as well? (on non-mac at least) - did they start failing (why?)
Patchset #5 (id:100001) has been deleted
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kButtonActionOnSpace and kClickControlOnReturn. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ==========
The CQ bit was checked by karandeepb@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...
PTAL Trent. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:311: (event.key_code() == ui::VKEY_RETURN); >> I think it would reach here if CustomButton::OnKeyPressed() returned false. I >> guess that's rare. perhaps a test was failing in that CL. Yeah that was my guess as well. This I guess can happen when OnKeyPressed is overriden by a subclass. >> On windows both space and return do something - I think we need something >> different here. But GetButtonActionForKeyEvent(event) would not return ACTION_NONE on windows for space and enter. >>It says "Invoked [before handling] by the focus manager for tab traversal, >>accelerators and other focus related actions." - I think that's still correct. >>That is, yes: View::OnKeyPressed is invoked before, but >>FocusManager::OnKeyPressed [not an override] is invoked after. You mean FocusManager::OnKeyEvent? There's no FocusManager::OnKeyPressed and the comment in view.h refers to OnKeyPressed, which was confusing to me. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:42: else if (event.key_code() == ui::VKEY_RETURN && On 2017/01/04 04:54:31, tapted wrote: > nit: "no else after return" - > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:256: views::InkDropState::ACTION_PENDING) { On 2017/01/04 04:54:31, tapted wrote: > views:: not needed, same below Done. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:261: break; On 2017/01/04 04:54:31, tapted wrote: > nit: no break Done. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:266: break; On 2017/01/04 04:54:31, tapted wrote: > nit: no break Done. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.h:32: // An enum describing the action to perform on a key press. On 2017/01/04 04:54:31, tapted wrote: > since this deals with press and release, the comment doesn't sound right. Maybe > > // An enum describing how a given key code should affect the button state. Changed to "An enum describing the events on which a button should be clicked for a given key event." to make it consistent with the comment for NotifyAction. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.h:34: ACTION_PRESS, // The button is pressed on OnKeyPressed and clicked on On 2017/01/04 04:54:31, tapted wrote: > Perhaps CLICK_ON_PRESS / CLICK_ON_RELEASE / CLICK_NONE? > > (and I guess `ClickAction` would make it more consistent with the above too) Yeah this seems better. https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:653: EXPECT_TRUE(button()->OnKeyPressed(space_press)); On 2017/01/04 04:54:31, tapted wrote: > why this change? (won't this bypass accelerator handling?) This seems more appropriate since we are unit-testing the CustomButton class and we are interested in the return value of OnKey[Pressed/Released]. It will bypass accelerator handling but then we haven't set up any custom accelerators. Testing accelerator handling at the dialog delegate level seems more appropriate. https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... ui/views/style/platform_style.h:53: static const bool kClickControlOnReturn; On 2017/01/04 04:54:31, tapted wrote: > I read "Control" and thought "Control+Return" - i.e. a key combination.. But do > we need two constants? They seem to imply each other. e.g. can we have Is kReturnClicksFocusedControl better? > // Whether buttons react only to the Space key (when pressed). Otherwise buttons > // react to Return when pressed, and Space when released. > static const bool kButtonsClickOnSpaceOnly = is_mac; Two constants seem better to me. We are describing two separate behaviors here- whether to click a button on space-press/space-release, and whether we should click a control on pressing the Return key (This will end up being used for other views as well, for eg. see https://codereview.chromium.org/2609943004/). https://codereview.chromium.org/2607923002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:159: LabelButton* cancel_button = client_view->cancel_button(); On 2017/01/04 04:54:31, tapted wrote: > can we keep the old tests as well? (on non-mac at least) - did they start > failing (why?) Done. I didn't think these tests were appropriate here. These test button behavior on OnKeyPressed for a return event, which is better tested at the CustomButton level.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:311: (event.key_code() == ui::VKEY_RETURN); On 2017/01/04 06:14:45, karandeepb wrote: > >> I think it would reach here if CustomButton::OnKeyPressed() returned false. I > >> guess that's rare. perhaps a test was failing in that CL. > Yeah that was my guess as well. This I guess can happen when OnKeyPressed is > overriden by a subclass. > > > >> On windows both space and return do something - I think we need something > >> different here. > But GetButtonActionForKeyEvent(event) would not return ACTION_NONE on windows > for space and enter. Ah. Yup. > > >>It says "Invoked [before handling] by the focus manager for tab traversal, > >>accelerators and other focus related actions." - I think that's still correct. > >>That is, yes: View::OnKeyPressed is invoked before, but > >>FocusManager::OnKeyPressed [not an override] is invoked after. > You mean FocusManager::OnKeyEvent? There's no FocusManager::OnKeyPressed and the > comment in view.h refers to OnKeyPressed, which was confusing to me. Ah, right. yeah that last sentence can probably be ignored. Perhaps fire off a CL to sky@ that removes it? (and yes: I meant FocusManager::OnKeyEvent) https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.h:32: // An enum describing the action to perform on a key press. On 2017/01/04 06:14:45, karandeepb wrote: > On 2017/01/04 04:54:31, tapted wrote: > > since this deals with press and release, the comment doesn't sound right. > Maybe > > > > // An enum describing how a given key code should affect the button state. > > Changed to "An enum describing the events on which a button should be clicked > for a given key event." to make it consistent with the comment for NotifyAction. sg https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2607923002/diff/80001/ui/views/style/platform... ui/views/style/platform_style.h:53: static const bool kClickControlOnReturn; On 2017/01/04 06:14:45, karandeepb wrote: > On 2017/01/04 04:54:31, tapted wrote: > > I read "Control" and thought "Control+Return" - i.e. a key combination.. But > do > > we need two constants? They seem to imply each other. e.g. can we have > Is kReturnClicksFocusedControl better? > > > // Whether buttons react only to the Space key (when pressed). Otherwise > buttons > > // react to Return when pressed, and Space when released. > > static const bool kButtonsClickOnSpaceOnly = is_mac; > > Two constants seem better to me. We are describing two separate behaviors here- > whether to click a button on space-press/space-release, and whether we should > click a control on pressing the Return key (This will end up being used for > other views as well, for eg. see https://codereview.chromium.org/2609943004/). Hm. I see your point. I was thinking of it in terms of a platform having one "primary" key for activating, which always activates on key press (mac=space, win=return). Then, if the primary key isn't space, space activates on release. This is more "one-dimensional". There's a downside to having them separate -- it implies there exists a platform configuration that doesn't change them as a pair. But I guess you can say that about lots of things here. This is OK, but I think we can improve the documentation (see later comment) https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:35: CLICK_ON_PRESS, I toyed with buttons a bit manually, and realised this could be confused with a mouse press, which never causes a "click". So perhaps CLICK_ON_KEY_PRESS / CLICK_ON_KEY_RELEASE and ClickAction -> KeyClickAction? https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:677: // with a dialog. with a dialog, even if a button has focus. https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:49: // Click action to perform on a button when the Space key is pressed. This comment doesn't read quite right, since "pressed" implies only one side of the action. Maybe something like // Whether the space key clicks a button on key press or key release. https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:52: // Whether Return key clicks the focused control. It might be good to be explicit here too. E.g. "Whether the Return key clicks the focused control (on key press). Otherwise, Return does nothing unless it is handled by an accelerator." (actually on Mac, it should NSBeep() if not handled by an accelerator, but that's orthogonal) https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style_mac.mm:44: // control is focused. With extended comment in platform_style.h, this comment can probably be dropped.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by karandeepb@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...
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2607923002/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:311: (event.key_code() == ui::VKEY_RETURN); > Ah, right. yeah that last sentence can probably be ignored. Perhaps fire off a > CL to sky@ that removes it? > > (and yes: I meant FocusManager::OnKeyEvent) Will do! https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:35: CLICK_ON_PRESS, On 2017/01/05 00:34:41, tapted wrote: > I toyed with buttons a bit manually, and realised this could be confused with a > mouse press, which never causes a "click". So perhaps > > CLICK_ON_KEY_PRESS / CLICK_ON_KEY_RELEASE > > and ClickAction -> KeyClickAction? Done. https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:677: // with a dialog. On 2017/01/05 00:34:41, tapted wrote: > with a dialog, even if a button has focus. Done. https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:49: // Click action to perform on a button when the Space key is pressed. On 2017/01/05 00:34:41, tapted wrote: > This comment doesn't read quite right, since "pressed" implies only one side of > the action. > > Maybe something like > > // Whether the space key clicks a button on key press or key release. Done. https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style.h:52: // Whether Return key clicks the focused control. On 2017/01/05 00:34:41, tapted wrote: > It might be good to be explicit here too. E.g. > > "Whether the Return key clicks the focused control (on key press). Otherwise, > Return does nothing unless it is handled by an accelerator." > > (actually on Mac, it should NSBeep() if not handled by an accelerator, but > that's orthogonal) Done. https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/2607923002/diff/120001/ui/views/style/platfor... ui/views/style/platform_style_mac.mm:44: // control is focused. On 2017/01/05 00:34:41, tapted wrote: > With extended comment in platform_style.h, this comment can probably be dropped. I'll let this remain since this explains that Return is bound to the default dialog action, which is something not captured in the comment in platform_style.h.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2607923002/#ps240001 (title: "Address review.")
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": 240001, "attempt_start_ts": 1483613498405240,
"parent_rev": "56107d3019db54a6b0c15e4154c2df0a8f0722e2", "commit_rev":
"b2cd473c86a4ce575d91505729699cf65f713276"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. Review-Url: https://codereview.chromium.org/2607923002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. Review-Url: https://codereview.chromium.org/2607923002 ========== to ========== MacViews: Handle Space and Return keys correctly for Buttons. Currently on MacViews, the Space key clicks the button on key-release and the Return key clicks the button on key-press if the button has focus. However the Cocoa behavior is that the Space key should click the button on key-press when a button is focused. Also the Return key should perform the default action associated with a dialog, irrespective of the view focused. This CL introduces two new constants to PlatformStyle to account for the special behavior on Mac, namely kClickActionOnSpace and kReturnClicksFocusedControl. CustomButton::OnKeyPressed, CustomButton::OnKeyReleased and CustomButton::SkipDefaultKeyEventProcessing are modified to respect the value of these platform specific constants. Existing unit tests are modified to be in line with the new behavior and one new test is also added. BUG=607429, 607430 TEST=On Mac, enable chrome://flags/#secondary-ui-md. Open the Bookmark Bubble. Enable Full Keyboard Access(Ctrl+F7). Focus the Edit button. Verify Space clicks the button on key-press and Return performs the default action(Done) instead of opening the Edit Bookmark Dialog. Committed: https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23 Cr-Commit-Position: refs/heads/master@{#441622} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23 Cr-Commit-Position: refs/heads/master@{#441622} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
