Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624093002:
MacViews: Correctly handle character events when there's an active TextInputClient. (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: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/b412f671d94f869489023137ba2705785883614f
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix tests #
Total comments: 11
Patch Set 3 : Rebase #Patch Set 4 : Some corner cases. #Patch Set 5 : Another corner case! #
Total comments: 2
Patch Set 6 : Flip isKeyDownEventHandled_ -> hasUnhandledKeyDownEvent_ #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (22 generated)
Description was changed from ========== MacViews: Correctly handle character events when there's an active TextInputClient. ========== to ========== 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. 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. ==========
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...
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. https://codereview.chromium.org/2624093002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (left): https://codereview.chromium.org/2624093002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:539: // requires two 16 bit characters. Currently Views menu only supports Also, modified this comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/2624093002/diff/20001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:211: CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS); Did not look into it, but tests fail with the EventGenerator on using TYPE_POPUP window on non-Mac platforms.
lgtm with an updated dcheck and other stuff below unless you see something different https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:520: ui::KeyEvent charEvent = GetCharacterEventFromNSEvent(keyDownEvent_); I only discovered it by chance, but there's a super-subtle bug introduced by having this here that results in the DCHECK on line 207 getting hit, e.g., when pressing Alt+q. That inserts "œ" which has [text length] == 1, but [[event characters] length] == 2. I think we still want mnemonic selection for alt+q (although perhaps MenuController should be a textInputClient / PrefixSelector like combobox..). On native mac menus, it jumps to 'o', which is presumably [event characters][0], so I think we just need to update the DCHECK on line 207 (e.g. to check >0) with a comment about Alt+q, unless you see something else. https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:523: } part of me is surprised we don't accidentally handle events twice this way, but I guess `DispatchKeyEvent` never actually makes it to InsertChar. https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:211: CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS); On 2017/01/11 08:16:20, karandeepb wrote: > Did not look into it, but tests fail with the EventGenerator on using TYPE_POPUP > window on non-Mac platforms. I think popups have can_activate = false by default during Widget::Init https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:222: event_generator_.reset( event_generator_ = base::MakeUnique<..EventGenerator>(..GetNativeWindow()); ( https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/d5FFq... )
Description was changed from ========== 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. 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. ========== to ========== 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. 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). Also, a new variable isKeyDownEventHandled is introduced so that keyDownEvent_ remains valid for the duration of keyDown:. This is needed because a keyDown: can infact lead to multiple insertText: action messages. 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. ==========
Patchset #5 (id:80001) has been deleted
Description was changed from ========== 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. 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). Also, a new variable isKeyDownEventHandled is introduced so that keyDownEvent_ remains valid for the duration of keyDown:. This is needed because a keyDown: can infact lead to multiple insertText: action messages. 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. ========== to ========== 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 isKeyDownEventHandled_ 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. ==========
PTAL Trent. https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:520: ui::KeyEvent charEvent = GetCharacterEventFromNSEvent(keyDownEvent_); >>I only discovered it by chance, but there's a super-subtle bug introduced by >>having this here that results in the DCHECK on line 207 getting hit, e.g., when >>pressing Alt+q. That inserts "œ" which has [text length] == 1, but [[event >>characters] length] == 2. >>I think we still want mnemonic selection for alt+q (although perhaps >>MenuController should be a textInputClient / PrefixSelector like combobox..). On >>native mac menus, it jumps to 'o', which is presumably [event characters][0], so >>I think we just need to update the DCHECK on line 207 (e.g. to check >0) with a >>comment about Alt+q, unless you see something else. Alt+q causes [[event character] length] == 1 in my testing, however [[event characters] length] = 2 and [text length] =1 is possible as you said (See code comments). I discovered some more subtle behavior. Hopefully the comments in code should be self explanatory. Decided to not use keyDownEvent_ = nil to mark the key event as handled, since it's useful to have the correct key down event for the duration of keyDown:, especially when multiple insertText: action messages can be generated for a single key down. Let me know if any of this does not make sense to you. https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:523: } On 2017/01/11 14:54:20, tapted wrote: > part of me is surprised we don't accidentally handle events twice this way, but > I guess `DispatchKeyEvent` never actually makes it to InsertChar. Reading the code, I think the flow on Mac is- InputMethodMac::DispatchKeyEvent -> InputMethodBase::DispatchKeyEventPostIME -> BridgedNativeWidget::DispatchKeyEventPostIME -> Widget::OnKeyEvent. which is quite different from other platforms where Input method interacts with the TextInputClient. Unrelated, seeing the code in BridgedNativeWidget::DispatchKeyEventPostIME, seems there's something wrong. We call Widget::OnKeyEvent and also FocusManager::OnKeyEvent. Widget::OnKeyEvent also calls FocusManager::OnKeyEvent. May also be related to crbug.com/679522. https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:211: CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS); On 2017/01/11 14:54:20, tapted wrote: > On 2017/01/11 08:16:20, karandeepb wrote: > > Did not look into it, but tests fail with the EventGenerator on using > TYPE_POPUP > > window on non-Mac platforms. > > I think popups have can_activate = false by default during Widget::Init Acknowledged. https://codereview.chromium.org/2624093002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox_unittest.cc:222: event_generator_.reset( On 2017/01/11 14:54:20, tapted wrote: > event_generator_ = base::MakeUnique<..EventGenerator>(..GetNativeWindow()); ( > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/d5FFq... > ) Done.
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.
lgtm with the fix below (hasPendingKeyDownEvent_) - take care flipping the logic :) https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:520: ui::KeyEvent charEvent = GetCharacterEventFromNSEvent(keyDownEvent_); On 2017/01/13 03:13:38, karandeepb wrote: > >>I only discovered it by chance, but there's a super-subtle bug introduced by > >>having this here that results in the DCHECK on line 207 getting hit, e.g., > when > >>pressing Alt+q. That inserts "œ" which has [text length] == 1, but [[event > >>characters] length] == 2. > >>I think we still want mnemonic selection for alt+q (although perhaps > >>MenuController should be a textInputClient / PrefixSelector like combobox..). > On > >>native mac menus, it jumps to 'o', which is presumably [event characters][0], > so > >>I think we just need to update the DCHECK on line 207 (e.g. to check >0) with > a > >>comment about Alt+q, unless you see something else. > > Alt+q causes [[event character] length] == 1 in my testing, however [[event > characters] length] = 2 and [text length] =1 is possible as you said (See code > comments). I discovered some more subtle behavior. Hopefully the comments in > code should be self explanatory. Decided to not use keyDownEvent_ = nil to mark > the key event as handled, since it's useful to have the correct key down event > for the duration of keyDown:, especially when multiple insertText: action > messages can be generated for a single key down. > > Let me know if any of this does not make sense to you. > Acknowledged. https://codereview.chromium.org/2624093002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:523: } On 2017/01/13 03:13:38, karandeepb wrote: > On 2017/01/11 14:54:20, tapted wrote: > > part of me is surprised we don't accidentally handle events twice this way, > but > > I guess `DispatchKeyEvent` never actually makes it to InsertChar. > > Reading the code, I think the flow on Mac is- > InputMethodMac::DispatchKeyEvent -> InputMethodBase::DispatchKeyEventPostIME -> > BridgedNativeWidget::DispatchKeyEventPostIME -> Widget::OnKeyEvent. > > which is quite different from other platforms where Input method interacts with > the TextInputClient. > > Unrelated, seeing the code in BridgedNativeWidget::DispatchKeyEventPostIME, > seems there's something wrong. We call Widget::OnKeyEvent and also > FocusManager::OnKeyEvent. Widget::OnKeyEvent also calls > FocusManager::OnKeyEvent. May also be related to crbug.com/679522. nothing seems too out of place yet.. but yes - something to investigate :/ https://codereview.chromium.org/2624093002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2624093002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.h:44: BOOL isKeyDownEventHandled_; This will be initialized to NO, so if something calls handleUnhandledKeyDownAsKeyEvent before the first call to keyDown, then I think we may try to make a ui::KeyEvent from nil. Perhaps BOOL hasPendingKeyDownEvent_; (and flip a bunch of logic).
Description was changed from ========== 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 isKeyDownEventHandled_ 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. ========== to ========== 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. ==========
https://codereview.chromium.org/2624093002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2624093002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.h:44: BOOL isKeyDownEventHandled_; On 2017/01/13 20:02:44, tapted wrote: > This will be initialized to NO, so if something calls > handleUnhandledKeyDownAsKeyEvent before the first call to keyDown, then I think > we may try to make a ui::KeyEvent from nil. > > Perhaps > > BOOL hasPendingKeyDownEvent_; > > (and flip a bunch of logic). Makes sense. Though I think hasUnhandledKeyDownEvent_ is better and more consistent with other code.
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/2624093002/#ps120001 (title: "Flip isKeyDownEventHandled_ -> hasUnhandledKeyDownEvent_")
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": 120001, "attempt_start_ts": 1484532166994940,
"parent_rev": "3f42814819c6318c4b6f9148f3fb9aa245f08257", "commit_rev":
"b412f671d94f869489023137ba2705785883614f"}
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/b412f671d94f869489023137ba27... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b412f671d94f869489023137ba27... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
