Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2033433006:
MacViews: Modify insertText handlers to correctly handle space key and simplify menu event dispatch. (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tdresser+watch_chromium.org, tfarina, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: 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
Committed: https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f
Cr-Commit-Position: refs/heads/master@{#400918}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 15
Patch Set 3 : Address review comments. #Patch Set 4 : Migrate key event logic from MenuKeyEventHandler to MenuController. #Patch Set 5 : Rebase #Patch Set 6 : Split changes in ui_controls_mac.mm #Patch Set 7 : Changes to comments. #
Total comments: 4
Patch Set 8 : Address review comments. #
Total comments: 2
Patch Set 9 : Fix trybot failures. Format code. #Patch Set 10 : Rebase and Remove patchset dependency #Patch Set 11 : Fix failing interactive_ui_tests. #
Messages
Total messages: 37 (22 generated)
Description was changed from ========== MacViews: space key events. ========== to ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 BUG=607429,617110 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 BUG=607429,617110 ========== to ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 BUG=607429,617110 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Will add tests subsequently, if you think the approach is correct. https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (left): https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1082: ui::VKEY_UNKNOWN) == Can IME's be active in case of an active menu, as this code seems to imply? https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:635: // Generate a synthetic ui::KeyEvent for toolkit-views. Since keyUp wasn't implemented till now, Views::OnKeyReleased wasn't probably being invoked up till now. We should check that this does not cause any undesired behavior as far as MacViews is concerned. https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:209: ui::KeyboardCodeFromNSEvent(event), ui::EF_NONE); Modified this. Earlier we were just casting the character to ui::KeyboardCode, which is incorrect.
Description was changed from ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 BUG=607429,617110 ========== to ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 ==========
Description was changed from ========== 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:, crbug.com/617110 is also fixed. Also, this CL removes all menu event handling from insertText:replacementRange: since it 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 ========== to ========== 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 ==========
This approach looks pretty good, but we should do the ui_controls_mac change first. https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (left): https://codereview.chromium.org/2033433006/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1082: ui::VKEY_UNKNOWN) == On 2016/06/06 02:04:42, karandeepb wrote: > Can IME's be active in case of an active menu, as this code seems to imply? That does kinda seem like something that should be supported, but maybe it's not possible to do that in OSX right now. https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (left): https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:19: // immediately. This lets us run the post-event task right So, sadly we will need to land the change that converts this from sendEvent to postEvent separately -- there may be some subtle changes (e.g. maybe things become flaky on some bots), and some other Cocoa folks should look at it too. https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:25: // observer/notification. Unlike windows, I cannot post non-events Should we keep some of these notes? https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:26: // In order to notify the caller correctly after all events has been has -> have https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:374: DCHECK(event); nit: blank line before https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:636: hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event); Should allow the menu to intercept keyUp as well, otherwise the up/downs won't be balanced https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:665: // We only handle the case where no of characters is 1. Cases not handled (not nit: 'We only' -> 'Only'. Also this needs to say why it's safe to ignore the other cases. https://codereview.chromium.org/2033433006/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2033433006/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1026: const ui::KeyEvent& key_event) { Is there a way of having MenuKeyEventHandler::OnKeyEvent call this? I think it would be good to merge the logic for these in case some fixes are missing or get lost in future
Patchset #4 (id:140001) has been deleted
Description was changed from ========== 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. Finally, ui_controls_mac.mm is modified to use [NSApp postEvent: atStart:] instead of [NSApp sendEvent:]. This is required since events sent using [NSApp sendEvent:] are not added to the event queue, hence the correct event is not reported by [NSApp currentEvent]. 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 ========== to ========== 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. This CL is dependent on crrev.com/2035393002. BUG=607429, 617110 ==========
PTAL Trent. Thanks. https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (left): https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:19: // immediately. This lets us run the post-event task right On 2016/06/06 04:44:05, tapted wrote: > So, sadly we will need to land the change that converts this from sendEvent to > postEvent separately -- there may be some subtle changes (e.g. maybe things > become flaky on some bots), and some other Cocoa folks should look at it too. Done. See crrev.com/2035393002. https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:25: // observer/notification. Unlike windows, I cannot post non-events On 2016/06/06 04:44:05, tapted wrote: > Should we keep some of these notes? These notes are explaining why [NSApp sendEvent] is being used. But since we are removing the last usage of sendEvent from this file, these should probably be removed. https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_mac.mm (right): https://codereview.chromium.org/2033433006/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_mac.mm:26: // In order to notify the caller correctly after all events has been On 2016/06/06 04:44:05, tapted wrote: > has -> have Done. https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:374: DCHECK(event); On 2016/06/06 04:44:05, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:636: hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event); On 2016/06/06 04:44:05, tapted wrote: > Should allow the menu to intercept keyUp as well, otherwise the up/downs won't > be balanced Done. https://codereview.chromium.org/2033433006/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:665: // We only handle the case where no of characters is 1. Cases not handled (not On 2016/06/06 04:44:05, tapted wrote: > nit: 'We only' -> 'Only'. Also this needs to say why it's safe to ignore the > other cases. Done. https://codereview.chromium.org/2033433006/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2033433006/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1026: const ui::KeyEvent& key_event) { On 2016/06/06 04:44:05, tapted wrote: > Is there a way of having MenuKeyEventHandler::OnKeyEvent call this? I think it > would be good to merge the logic for these in case some fixes are missing or get > lost in future Done.
Hm - if this is fixing http://crbug.com/607429 I think we need a regression test for that, unless there's already coverage somewhere. https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:79: const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN; Since this file is bigger, there should be a comment for this - but I guess we could just move it down to inside OnWillDispatchKeyEvent(..) as well. (we probably also want COMMAND instead of ALT on Mac, but we should see if that's actually a problem before adding this complexity). https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1050: if (!MenuController::GetActiveInstance()) Convention in the rest of this file is to omit the MenuController:: prefixes. Here and on MenuController::EXIT_NONE below. (we should fix the ones that I put in above for MenuController::EXIT_ALL/DESTROYED too)
PTAL Trent. Have added a test which fails on the current master for MacViews. http://crbug.com/607429 is not fully fixed, since space key behavior is not completely in line with Cocoa. On Views, click is registered on space key release, while on Cocoa, it is done on key press itself. https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:79: const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN; On 2016/06/07 01:23:28, tapted wrote: > Since this file is bigger, there should be a comment for this - but I guess we > could just move it down to inside OnWillDispatchKeyEvent(..) as well. (we > probably also want COMMAND instead of ALT on Mac, but we should see if that's > actually a problem before adding this complexity). All the character events we are passing currently have no modifier flags set. I think this is fine for the moment. We can modify it later if this causes any problems. https://codereview.chromium.org/2033433006/diff/220001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1050: if (!MenuController::GetActiveInstance()) On 2016/06/07 01:23:28, tapted wrote: > Convention in the rest of this file is to omit the MenuController:: prefixes. > Here and on MenuController::EXIT_NONE below. (we should fix the ones that I put > in above for MenuController::EXIT_ALL/DESTROYED too) Done.
looks like aura platforms aren't happy with the test, looks good otherwise :) https://codereview.chromium.org/2033433006/diff/230001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2033433006/diff/230001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:587: // Todo(karandeepb): On Mac, button should get clicked on a Space key press (and nit: button -> a button
Patchset #9 (id:250001) has been deleted
Patchset #11 (id:310001) has been deleted
Patchset #10 (id:290001) has been deleted
Patchset #9 (id:270001) has been deleted
PTAL Trent. Didn't investigate much but ui::EventGenerator doesn't seem to work for key events on Windows and Linux. Also, other places in the codebase also directly invoke the event like I have done. https://codereview.chromium.org/2033433006/diff/230001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2033433006/diff/230001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:587: // Todo(karandeepb): On Mac, button should get clicked on a Space key press (and On 2016/06/07 04:59:35, tapted wrote: > nit: button -> a button Done.
lgtm, although I'm assuming there will be views_unittests regressions without https://codereview.chromium.org/2035393002, so let's wait for that. We might want to look into http://crbug.com/49270 some more too I've done a variety of flakiness fixes for things using ClickOnView in the past - it might be easy to get BrowserKeyEventsTest.CommandKeyEvents to work with sendEvent atStart
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@ for ui/views/controls review.
LGTM
Patchset #11 (id:370001) has been deleted
Description was changed from ========== 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. This CL is dependent on crrev.com/2035393002. BUG=607429, 617110 ========== to ========== 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 ==========
PTAL Trent. I have removed the dependency on the patchset https://codereview.chromium.org/2035393002#ps1 and added a workaround which fixes the interactive_ui_tests that fail as a result of removing the dependency.
On 2016/06/21 07:31:31, karandeepb wrote: > PTAL Trent. I have removed the dependency on the patchset > https://codereview.chromium.org/2035393002#ps1 and added a workaround which > fixes the interactive_ui_tests that fail as a result of removing the dependency. lgtm - IMO this is nicer than [NSApp currentEvent] regardless - less reliance on globals tends to be a good thing.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2033433006/#ps390001 (title: "Fix failing interactive_ui_tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033433006/390001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:390001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f Cr-Commit-Position: refs/heads/master@{#400918} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5c38e137d2b39cc17c828ddfb783e1842e7dd76f Cr-Commit-Position: refs/heads/master@{#400918} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
