New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 607429 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocking:
issue 603386
issue 636676



Sign in to add a comment

MacViews: Space Key doesn't choose/click the currently focused item.

Project Member Reported by karandeepb@chromium.org, Apr 28 2016

Issue description

Version: 52.0.2719.0
OS: Mac
What steps will reproduce the problem?
(1) Enable chrome://flags/#mac-views-dialogs
(2) Open bookmark bubble by clicking on Star icon to the right of address bar.
(3) Shift focus to Edit button.
(4) Press Space key.

What is the expected output?
The Edit bookmark dialog should open.

What do you see instead?
Space Key press is ignored.

See https://support.apple.com/en-au/HT204434

 
Components: Internals>Views
Owner: karandeepb@chromium.org
So it turns out, currently we are not getting any Space Key events on MacViews. BridgedContentView::onKeyDown calls interpretKeyEvents. For a space key press, this generates an insertText action message, when inputContext returns nil. This is not routed through doCommandBySelector, so we don't generate a synthetic ui::KeyEvent for this. 

A simple solution may be to call BridgedContentView::handleKeyEvent in insertText if the string to be inserted has a length of 1. 

But probably we need to think more about  crbug.com/616326 , to decouple key event handling for menus, textfield etc.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c38e137d2b39cc17c828ddfb783e1842e7dd76f

commit 5c38e137d2b39cc17c828ddfb783e1842e7dd76f
Author: karandeepb <karandeepb@chromium.org>
Date: Tue Jun 21 07:52:39 2016

MacViews: Modify insertText handlers to correctly handle space key and simplify menu event dispatch.

Currently, Space key events are not being passed down the view hierarchy, since
no ui::KeyEvent is being generated for them. This CL modifies insertText: to
generate a synthetic ui::KeyEvent for character events. This allows us to
correctly handle the space (and possibly other) keys. Since we no longer call
insertText:replacementRange: from insertText:,  crbug.com/617110  is also fixed.

Also, this CL removes all menu event handling from insertText:replacementRange:
since it is only invoked in case of a valid inputContext. Also, this CL adds
keyUp: to BridgedContentView, so that key-up events are passed to the Views
hierarchy.

Also checked that this causes no views_unittests regressions and that
enabled interactive_ui_tests in:
-menu_controller_interactive_uitest.cc
-menu_item_view_interactive_uitest.cc
-menu_model_adapter_test.cc
-menu_view_drag_and_drop_test.cc
still work correctly.

BUG= 607429 ,  617110 

Review-Url: https://codereview.chromium.org/2033433006
Cr-Commit-Position: refs/heads/master@{#400918}

[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/cocoa/bridged_content_view.h
[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/controls/button/custom_button_unittest.cc
[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f/ui/views/controls/menu/menu_key_event_handler.cc

Blocking: 636676

Comment 6 by tapted@chromium.org, Dec 12 2016

Blocking: 603386
Labels: Phase3
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dfd313b6e9b08551787c6938a04ab1e2814c0f23

commit dfd313b6e9b08551787c6938a04ab1e2814c0f23
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Jan 05 10:55:29 2017

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
Cr-Commit-Position: refs/heads/master@{#441622}

[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/controls/button/custom_button.cc
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/controls/button/custom_button.h
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/controls/button/custom_button_unittest.cc
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/style/platform_style.cc
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/style/platform_style.h
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/style/platform_style_mac.mm
[modify] https://crrev.com/dfd313b6e9b08551787c6938a04ab1e2814c0f23/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b412f671d94f869489023137ba2705785883614f

commit b412f671d94f869489023137ba2705785883614f
Author: karandeepb <karandeepb@chromium.org>
Date: Mon Jan 16 02:34:26 2017

MacViews: Correctly handle character events when there's an active TextInputClient.

Currently in insertTextInternal:, we do not generate a synthetic character event
for the View hierarchy when there's an active TextInputClient. Since a Space key
press generates an insertText: (or insertText:replacementRange:) action message,
this causes views::Combobox to not receive the space key press. This is because
views::Combobox uses the views::PrefixSelector TextInputClient when focused. As
a result, a space key press on a combobox does not currently open the combobox
dropdown.

To solve, generate key events for all character events in insertTextInternal:.
The key event is generated before calling TextInputClient::Insert[Char/Text]
which is similar to other platforms. However, the key event is not passed to the
View hierarchy in case there's active composition text, since it should be
consumed by the IME in that case.

Also, a new variable hasUnhandledKeyDownEvent_ is introduced on the
BridgedContentView so that keyDownEvent_ remains valid for the duration of
keyDown:. This is needed because a keyDown: can infact lead to multiple
insertText: action messages.

Also, combobox unittests are modified to use an EventGenerator to test key event
behavior to provide better coverage. (With an EventGenerator,
ComboboxTest.NotifyOnClickWithSpaceKey fails on the master).

BUG= 619545 ,  607429 
TEST=With chrome://flags/#secondary-ui-md on Mac, open the Bookmark bubble.
Enable Full Keyboard Access (Ctrl+F7). Focus the Folder combobox. Ensure
pressing space opens the combobox dropdown.

Review-Url: https://codereview.chromium.org/2624093002
Cr-Commit-Position: refs/heads/master@{#443832}

[modify] https://crrev.com/b412f671d94f869489023137ba2705785883614f/ui/views/cocoa/bridged_content_view.h
[modify] https://crrev.com/b412f671d94f869489023137ba2705785883614f/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/b412f671d94f869489023137ba2705785883614f/ui/views/controls/combobox/combobox_unittest.cc

Cc: tapted@chromium.org shrike@chromium.org
Status: Fixed (was: Started)
Status: Combobox, Buttons and Links all respond correctly to Space. Seems to me TreeView and Tabbed pane shouldn't (and they don't currently). 
Marking as fixed. Also, cc'ing shrike@ and tapted@ in case they know of any different behavior.

Sign in to add a comment