Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505943002:
MacViews: Fix accelerator handling while Omnibox is in focus. (Closed)
DescriptionMacViews: Fix accelerator handling while Omnibox is in focus.
Currently in both Cocoa and MacViewsBrowser when Omnibox is in focus the
accelerators are only handled in -[NativeWidgetMacNSWindow
performKeyEquivalent:], which internally uses ChromeCommandDispatcherDelegate,
which consults global_keyboard_shortcuts_cocoa_mac.mm's tables.
In crbug.com/620315 we're trying to migrate the
global_keyboard_shortcuts_cocoa_mac.mm's tables over to accelerator_table.cc.
BUG=665823
Committed: https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296
Cr-Commit-Position: refs/heads/master@{#434436}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove performKeyEquivalent:, update keyDown:, add tests. #
Total comments: 25
Patch Set 3 : Fix review issues. #
Total comments: 8
Patch Set 4 : Fix review issues. #
Total comments: 5
Patch Set 5 : Make EventGenerator::Target's comments more verbose. #Patch Set 6 : Fixed TextfieldTest.*, disable ViewTest.ActivateAcceleratorOnMac on non-MacViews. #
Total comments: 19
Patch Set 7 : Fix tapted's review issues. #
Total comments: 2
Messages
Total messages: 52 (23 generated)
mblsha@yandex-team.ru changed reviewers: + tapted@chromium.org
There are two tests that fail for https://codereview.chromium.org/2074643003/ when I'm testing locally: 1. GlobalKeyboardShortcutsTest.SwitchTabsMac (browser_tests) 2. GlobalKeyboardShortcuts.ShortcutsToWindowCommand (unit_tests) 1. Passes with this CL. 2. Fails because it expects to find an accelerator for IDC_SELECT_TAB_0 in GetWindowKeyboardShortcutTable(). Guess the test needs to be split in cocoa / views variants as well. Should I write more tests for this CL?
On 2016/11/16 13:19:57, themblsha wrote: > There are two tests that fail for https://codereview.chromium.org/2074643003/ > when I'm testing locally: > 1. GlobalKeyboardShortcutsTest.SwitchTabsMac (browser_tests) > 2. GlobalKeyboardShortcuts.ShortcutsToWindowCommand (unit_tests) > > 1. Passes with this CL. > 2. Fails because it expects to find an accelerator for IDC_SELECT_TAB_0 in > GetWindowKeyboardShortcutTable(). Guess the test needs to be split in cocoa / > views variants as well. perhaps - depends on the failure. I'd need to see a suggested fix. (But also it only fails with mac_views_browser=1 so, to be frank/honest, you'll get through the CQ fine and you can follow up with a fix later if you need to. > > Should I write more tests for this CL? Yeah, this really does need a separate test. There are some disabled (#if 0) tests in view_unittest.cc for accelerators which actually pass on Mac (but not Linux/Windows) - https://codereview.chromium.org/2509733005/ . Unfortunately, these don't really test coverage for this. And I don't think ui::test::EventGenerator will help much, since it simulates event dispatch by sending directly to keyDown. Sending to the app with EventGenerator::set_targeting_application may help, but that might need something in an interactive_ui_test rather than a unit_test (which is OK). It might also be possible to refactor set_targeting_application into something like set_target({APPLICATION, WINDOW, WIDGET}), where we add a new path for WINDOW which does [window sendEvent:event] rather than [NSApp sendEvent:] or EmulateSendEvent(). https://codereview.chromium.org/2505943002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:763: - (BOOL)performKeyEquivalent:(NSEvent*)theEvent { I think there's more we have to do here. Have a look at -performKeyEquivalent: in render_widget_host_view_mac.mm . We probably want the firstResponder check, and then I really like the idea of a shared method like -keyEvent:wasKeyEquivalent: which both keyDown and performKeyEquivalent call. We shouldn't need separate performKeyEquivalentEvent_ and keyDownEvent_ members. I'm also not sure if we want to skip the bubbling in doCommandBySelector -- i.e. I think we want that NSBeep() -- what are the cases that beep when we don't want them to? The private method override `- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event` in render_widget_host_view_mac.mm looks really useful as well. Maybe it can even simplify our performKeyEquivalent: further.
> perhaps - depends on the failure. I'd need to see a suggested fix. (But also it > only fails with mac_views_browser=1 so, to be frank/honest, you'll get through > the CQ fine and you can follow up with a fix later if you need to. Heh, then I could successfully land my original CL in its approved form :-D https://codereview.chromium.org/2505943002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:763: - (BOOL)performKeyEquivalent:(NSEvent*)theEvent { On 2016/11/17 08:20:14, tapted wrote: > I think there's more we have to do here. Have a look at -performKeyEquivalent: > in render_widget_host_view_mac.mm . We probably want the firstResponder check, > and then I really like the idea of a shared method like > -keyEvent:wasKeyEquivalent: which both keyDown and performKeyEquivalent call. We > shouldn't need separate performKeyEquivalentEvent_ and keyDownEvent_ members. Good catch! > I'm also not sure if we want to skip the bubbling in doCommandBySelector -- i.e. > I think we want that NSBeep() -- what are the cases that beep when we don't want > them to? https://codereview.chromium.org/2074643003/#msg73 Accelerators that result on noop: command will beep, see my older comment for a list. If we handle both performKeyEquivalent: and keyDown: it could result in double-beeps, which would be subpar. > The private method override `- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event` in > render_widget_host_view_mac.mm looks really useful as well. Maybe it can even > simplify our performKeyEquivalent: further. Whoa! That's the ticket! Now your original suggestion is more feasible. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2216: TEST_F(ViewTest, ActivateAcceleratorOnMac) { Here's the test for all the code paths.
nice - this looks pretty good. nits mostly https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... File ui/events/test/event_generator.h (right): https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:141: // Events could be dispatched using different methods. I'd add a sentence like "The choice is a tradeoff between test robustness and coverage of OS internals that affect event dispatch." https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:144: // Dispatch through the application. "Least robust." https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:148: // Default. Emulates default Window dispatch. "Most robust." https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.h:10: #include "base/optional.h" nit: remove https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:780: if (keyDownEvent_) { comment above: // If |keyDownEvent_| wasn't cleared during -interpretKeyEvents:, it wasn't // handled. Give Widget accelerators a chance to handle it. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1305: ui::KeyEvent event(keyDownEvent_); Can this just return here? I.e. return; // Handle in -keyDown:. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1317: if (keyDownEvent_) { This needs a comment to say why it can't be done in keyDown Something like // For events that AppKit sends via doCommandBySelector:, first attempt to // handle as a Widget accelerator. Forward along the responder chain only if the // Widget doesn't handle it. See how you feel about a helper method like - (BOOL)handleUnhandledKeyDownAsKeyEvent { if (!keyDownEvent_) return NO; ui::KeyEvent event(keyDownEvent_); [self handleKeyEvent:&event]; keyDownEvent_ = nil; return event.handled(); } Then this would look like if (![self handleUnhandledKeyDownAsKeyEvent]) [[self nextResponder] doCommandBySelector:selector]; and we'd have - (void)keyDown:(NSEvent*)theEvent { keyDownEvent_ = theEvent; // Convert the event into an action message, according to OSX key mappings. [self interpretKeyEvents:@[ theEvent ]]; [self handleUnhandledKeyDownAsKeyEvent]; DCHECK(!keyDownEvent_); } https://codereview.chromium.org/2505943002/diff/20001/ui/views/test/event_gen... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/test/event_gen... ui/views/test/event_generator_delegate_mac.mm:358: case Target::APPLICATION: this should be indented - git cl format? https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2109: class TestViewWidget { nit: comment like // A Widget with a TestView in the view hierarchy. Used for accelerator tests. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2214: #if defined(OS_MACOSX) Does this new test need to be OS_MACOSX? (ideally it should be cross-platform, but I agree that getting the existing tests running on non-mac is a bit orthogonal) https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2215: // Test that BridgedContentView correctly handle Accelerator key events. nit: handle -> handles also add .. key events when subject to OS event dispatch.
https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... File ui/events/test/event_generator.h (right): https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:141: // Events could be dispatched using different methods. On 2016/11/18 07:28:04, tapted wrote: > I'd add a sentence like > > "The choice is a tradeoff between test robustness and coverage of OS internals > that affect event dispatch." Done. https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:144: // Dispatch through the application. On 2016/11/18 07:28:04, tapted wrote: > "Least robust." Done. https://codereview.chromium.org/2505943002/diff/20001/ui/events/test/event_ge... ui/events/test/event_generator.h:148: // Default. Emulates default Window dispatch. On 2016/11/18 07:28:04, tapted wrote: > "Most robust." Done. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.h:10: #include "base/optional.h" On 2016/11/18 07:28:04, tapted wrote: > nit: remove Done. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:780: if (keyDownEvent_) { On 2016/11/18 07:28:04, tapted wrote: > comment above: > > // If |keyDownEvent_| wasn't cleared during -interpretKeyEvents:, it wasn't > // handled. Give Widget accelerators a chance to handle it. Done. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1305: ui::KeyEvent event(keyDownEvent_); On 2016/11/18 07:28:04, tapted wrote: > Can this just return here? I.e. > > return; // Handle in -keyDown:. Yep, should work. https://codereview.chromium.org/2505943002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1317: if (keyDownEvent_) { On 2016/11/18 07:28:04, tapted wrote: > This needs a comment to say why it can't be done in keyDown > > Something like > > // For events that AppKit sends via doCommandBySelector:, first attempt to > // handle as a Widget accelerator. Forward along the responder chain only if the > // Widget doesn't handle it. > > > See how you feel about a helper method like > > - (BOOL)handleUnhandledKeyDownAsKeyEvent { > if (!keyDownEvent_) > return NO; > > ui::KeyEvent event(keyDownEvent_); > [self handleKeyEvent:&event]; > keyDownEvent_ = nil; > return event.handled(); > } > > Then this would look like > > if (![self handleUnhandledKeyDownAsKeyEvent]) > [[self nextResponder] doCommandBySelector:selector]; > > and we'd have > > - (void)keyDown:(NSEvent*)theEvent { > keyDownEvent_ = theEvent; > // Convert the event into an action message, according to OSX key mappings. > [self interpretKeyEvents:@[ theEvent ]]; > [self handleUnhandledKeyDownAsKeyEvent]; > DCHECK(!keyDownEvent_); > } Nice, removes quite a bit of duplicate code. https://codereview.chromium.org/2505943002/diff/20001/ui/views/test/event_gen... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/test/event_gen... ui/views/test/event_generator_delegate_mac.mm:358: case Target::APPLICATION: On 2016/11/18 07:28:04, tapted wrote: > this should be indented - git cl format? Ah, I'm probably using an outdated clang-format in my Sublime Text installation. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2109: class TestViewWidget { On 2016/11/18 07:28:04, tapted wrote: > nit: comment like > > // A Widget with a TestView in the view hierarchy. Used for accelerator tests. Done. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2214: #if defined(OS_MACOSX) On 2016/11/18 07:28:04, tapted wrote: > Does this new test need to be OS_MACOSX? (ideally it should be cross-platform, > but I agree that getting the existing tests running on non-mac is a bit > orthogonal) I'm using EF_COMMAND_DOWN, I think it's not very portable by itself. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2215: // Test that BridgedContentView correctly handle Accelerator key events. On 2016/11/18 07:28:04, tapted wrote: > nit: handle -> handles > > also add > > .. key events when subject to OS event dispatch. Done.
basically lgtm with the following, but you'll OWNERS (probably sadrul, from ui/events/OWNERS) https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2214: #if defined(OS_MACOSX) On 2016/11/18 13:31:50, themblsha wrote: > On 2016/11/18 07:28:04, tapted wrote: > > Does this new test need to be OS_MACOSX? (ideally it should be cross-platform, > > but I agree that getting the existing tests running on non-mac is a bit > > orthogonal) > > I'm using EF_COMMAND_DOWN, I think it's not very portable by itself. Only the first test expectation does, and it would make sense to do something like #if defined(OS_MACOSX) // Only use EF_COMMAND_DOWN on Mac. const ui::EventFlags kPlatformModifier = ui::EF_COMMAND_DOWN; #else const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN; #endif We should at least try it out on aura platforms - I ran some jobs at crrev.com/2515203002 but tip of tree is currently broken. Cross platform tests are really great, since they not only give other platforms test coverage, but they also help rationalise about changed behaviour by demonstrating that it is behaviour that already exists on other platforms. https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:277: // Allows accelerators to be handled at different points in AppKit key event // dispatch. Checks for an unhandled event passed in to -keyDown: and passes it // to the Widget for processing. Returns YES if the Widget handles it. - (BOOL)handleUnhandledKeyDownAsKeyEvent https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:766: - (BOOL)handleUnhandledKeyDownAsKeyEvent { This should be moved to after handleKeyEvent: (line 457) https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1317: if (keyDownEvent_ && [NSStringFromSelector(selector) hasPrefix:@"insert"]) { nit: no curlies https://codereview.chromium.org/2505943002/diff/40001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/40001/ui/views/view_unittest.... ui/views/view_unittest.cc:2215: #if defined(OS_MACOSX) if we can't get rid of this #if, we should put a comment above it and link to a bug to make this cross-platform
mblsha@yandex-team.ru changed reviewers: + sadrul@chromium.org
sadrul: Please review event_generator, event_monitor_unittest and view_unittest. https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:2214: #if defined(OS_MACOSX) On 2016/11/21 06:41:02, tapted wrote: > On 2016/11/18 13:31:50, themblsha wrote: > > On 2016/11/18 07:28:04, tapted wrote: > > > Does this new test need to be OS_MACOSX? (ideally it should be > cross-platform, > > > but I agree that getting the existing tests running on non-mac is a bit > > > orthogonal) > > > > I'm using EF_COMMAND_DOWN, I think it's not very portable by itself. > > Only the first test expectation does, and it would make sense to do something > like > > #if defined(OS_MACOSX) > // Only use EF_COMMAND_DOWN on Mac. > const ui::EventFlags kPlatformModifier = ui::EF_COMMAND_DOWN; > #else > const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN; > #endif > > We should at least try it out on aura platforms - I ran some jobs at > crrev.com/2515203002 but tip of tree is currently broken. > > Cross platform tests are really great, since they not only give other platforms > test coverage, but they also help rationalise about changed behaviour by > demonstrating that it is behaviour that already exists on other platforms. Done. https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:277: On 2016/11/21 06:41:02, tapted wrote: > // Allows accelerators to be handled at different points in AppKit key event > // dispatch. Checks for an unhandled event passed in to -keyDown: and passes it > // to the Widget for processing. Returns YES if the Widget handles it. > - (BOOL)handleUnhandledKeyDownAsKeyEvent Done. But this forward-declaration interface block is really not needed anymore, you think it's worth having only to have all the code comments in a single place? https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:766: - (BOOL)handleUnhandledKeyDownAsKeyEvent { On 2016/11/21 06:41:02, tapted wrote: > This should be moved to after handleKeyEvent: (line 457) Done. https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1317: if (keyDownEvent_ && [NSStringFromSelector(selector) hasPrefix:@"insert"]) { On 2016/11/21 06:41:02, tapted wrote: > nit: no curlies Done.
https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... File ui/events/test/event_generator.h (right): https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... ui/events/test/event_generator.h:147: WIDGET, The difference between WINDOW and WIDGET isn't really clear. Mind clarifying that in the comment here? Can you also mention that it is used only on mac? https://codereview.chromium.org/2505943002/diff/60001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/60001/ui/views/view_unittest.... ui/views/view_unittest.cc:2268: TEST_F(ViewTest, ActivateAccelerator) { Any reason these tests can't be turned on on other platforms too?
https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... File ui/events/test/event_generator.h (right): https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... ui/events/test/event_generator.h:147: WIDGET, On 2016/11/21 16:30:53, sadrul wrote: > The difference between WINDOW and WIDGET isn't really clear. Mind clarifying > that in the comment here? > > Can you also mention that it is used only on mac? Made the comments more verbose. "Currently only supported on Mac." is mentioned on top of the enum and applies to all the cases. Is it not prominent enough? https://codereview.chromium.org/2505943002/diff/60001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/60001/ui/views/view_unittest.... ui/views/view_unittest.cc:2268: TEST_F(ViewTest, ActivateAccelerator) { On 2016/11/21 16:30:53, sadrul wrote: > Any reason these tests can't be turned on on other platforms too? Currently they fail for some reason on non-MacViews builds: https://codereview.chromium.org/2509733005/, and they were disabled before this CL so this is not a regression.
lgtm
https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... File ui/events/test/event_generator.h (right): https://codereview.chromium.org/2505943002/diff/60001/ui/events/test/event_ge... ui/events/test/event_generator.h:147: WIDGET, On 2016/11/21 17:02:20, themblsha wrote: > On 2016/11/21 16:30:53, sadrul wrote: > > The difference between WINDOW and WIDGET isn't really clear. Mind clarifying > > that in the comment here? > > > > Can you also mention that it is used only on mac? > > Made the comments more verbose. "Currently only supported on Mac." is mentioned > on top of the enum and applies to all the cases. Is it not prominent enough? Whoops, I missed that comment :)
still lgtm - thanks! https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:277: On 2016/11/21 12:44:20, themblsha wrote: > On 2016/11/21 06:41:02, tapted wrote: > > // Allows accelerators to be handled at different points in AppKit key event > > // dispatch. Checks for an unhandled event passed in to -keyDown: and passes > it > > // to the Widget for processing. Returns YES if the Widget handles it. > > - (BOOL)handleUnhandledKeyDownAsKeyEvent > > Done. But this forward-declaration interface block is really not needed anymore, > you think it's worth having only to have all the code comments in a single > place? Method/prototype declarations on private categories (i.e. `@interface Foo ()` with nothing in the parens) have a neat property, where it requires that all declared methods be defined in the current translation unit. So, it's a good way to describe the `private:` section the way we do for C++ classes - I think it's easier to scan through an @interface for the documentation rather @implementation. Also, I think it's important to call out the methods we are adding, rather than those we are overriding - ObjectiveC makes it easy for the distinction to get lost. (also Objective C compilers didn't always figure out that a private method existed until it was parsed, i.e. similar to regular C -- you weren't always able to call a private method that was only defined "later" in the file)
On 2016/11/21 22:00:16, tapted wrote: > Method/prototype declarations on private categories (i.e. `@interface Foo ()` > with nothing in the parens) have a neat property, where it requires that all > declared methods be defined in the current translation unit. So, it's a good way > to describe the `private:` section the way we do for C++ classes - I think it's > easier to scan through an @interface for the documentation rather > @implementation. Also, I think it's important to call out the methods we are > adding, rather than those we are overriding - ObjectiveC makes it easy for the > distinction to get lost. > > (also Objective C compilers didn't always figure out that a private method > existed until it was parsed, i.e. similar to regular C -- you weren't always > able to call a private method that was only defined "later" in the file) Thanks for explanation :-)
The CQ bit was checked by mblsha@yandex-team.ru
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Had to fix TextfieldTest*, please take a look whether my changes are kosher. https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:533: keyDownEvent_ = nil; Fixex TextfieldTest.TextInputType_InsertionTest (and several other tests), as otherwise we'd get duplicate text entry. https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:552: DCHECK(!keyDownEvent_); This didn't cause any problems with the tests I've tried so placed a DCHECK just in case. https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:96: const bool control = event.IsCommandDown(); Fixes TextfieldTest.CutCopyPaste. "Ensure [Ctrl]+[Shift]+[Insert] is a no-op" triggered Paste because it looked at Control modifier on Mac and actually sent Cmd+Shift+Insert. https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1277: #endif Fixes TextfieldTest.KeysWithModifiersTest. MockInputMethod dispatches Cmd+2 KeyDown event directly to InsertChar(). https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (left): https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1258: EXPECT_FALSE(textfield_->key_received()); The comment is outdated, as we try to DispatchKeyEvent using the InputMethod, and key now is actually received. https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest... ui/views/view_unittest.cc:2216: // run. Figure out if still valuable and either nuke or fix. crbug.com/667757. This test fails on non-MacViews, so I've created a crbug for that.
The CQ bit was checked by mblsha@yandex-team.ru 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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Since new files are changing, once the issues are resolved, you should get an lg from someone in ui/views/controls/textfield/OWNERS (and sadrul should look again if you end up changing ui/events/base_event_utils.cc as I suggest below) https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:533: keyDownEvent_ = nil; On 2016/11/22 14:33:48, themblsha wrote: > Fixex TextfieldTest.TextInputType_InsertionTest (and several other tests), as > otherwise we'd get duplicate text entry. Acknowledged. The comment could be more terse, too, since it's probably needed below. E.g. keyDownEvent_ = nil; // Handled. https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:552: DCHECK(!keyDownEvent_); On 2016/11/22 14:33:48, themblsha wrote: > This didn't cause any problems with the tests I've tried so placed a DCHECK just > in case. It's easy to hit this DCHECK - just focus something that isn't a textfield and press a key. E.g. focus a button after turning on Full Keyboard Access in OSX System Preferences. (pressing a key on a dialog with no textfields will probably also trigger this) https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:96: const bool control = event.IsCommandDown(); On 2016/11/22 14:33:48, themblsha wrote: > Fixes TextfieldTest.CutCopyPaste. "Ensure [Ctrl]+[Shift]+[Insert] is a no-op" > triggered Paste because it looked at Control modifier on Mac and actually sent > Cmd+Shift+Insert. This doesn't look right -- E.g. -[BridgedContentView copy:] uses EF_CONTROL_DOWN, (but also an edit command so it won't end up here). I'd suggest something like const bool control = event.IsControlDown() || event.IsCommandDown(); (i.e. no #ifdef guard - do that on all platforms - IsCommandDown should only come up on mac anyway) It might also be possible that the test doesn't make sense on Mac, or should really be dispatching the event differently (e.g. EventGenerator::WINDOW), since in "real" code (outside of tests) this is handled elsewhere on Mac. https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1277: #endif On 2016/11/22 14:33:48, themblsha wrote: > Fixes TextfieldTest.KeysWithModifiersTest. MockInputMethod dispatches Cmd+2 > KeyDown event directly to InsertChar(). This test already behaves correctly on CrOS. I think the right fix for this is to change ui/events/base_event_utils.cc and add a `kSystemKeyModifierMask` for Mac that is just EF_COMMAND_DOWN -- (Mac *shouldn't* include EF_ALT_DOWN for the same reasons as described in the comment for IsSystemKeyModifier - i.e. Alt is used to input extended characters on Mac). https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (left): https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1257: // on to the next responder, usually ending up at the window, which will beep. this is nice :) https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest... ui/views/view_unittest.cc:2215: // TODO: these tests were initially commented out when getting aura to This comment about aura doesn't apply to this test, since the test has never run places other than mac. The TODO format isn't right (I thought there was a presubmit for this?) Something like // TODO(themblsha): Bring this up on non-Mac platforms. It currently fails because <reason>. See http://crbug.com/667757. #if defined(OS_MACOSX) then the old comment should move back down where it was (with a separate #if) (although, it is likely that it's failing for the same reason as the old tests - it would be nice to figure out what that is..) https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest... ui/views/view_unittest.cc:2225: const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN; this case can be omitted if it's not being brought up on !mac.
mblsha@yandex-team.ru changed reviewers: + pkasting@chromium.org
+pkasting. Peter, could you please look at ui/events/base_event_utils.cc, and textfield*.cc? https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:533: keyDownEvent_ = nil; On 2016/11/23 02:09:30, tapted wrote: > On 2016/11/22 14:33:48, themblsha wrote: > > Fixex TextfieldTest.TextInputType_InsertionTest (and several other tests), as > > otherwise we'd get duplicate text entry. > > Acknowledged. The comment could be more terse, too, since it's probably needed > below. E.g. > > keyDownEvent_ = nil; // Handled. Done. https://codereview.chromium.org/2505943002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:552: DCHECK(!keyDownEvent_); On 2016/11/23 02:09:30, tapted wrote: > On 2016/11/22 14:33:48, themblsha wrote: > > This didn't cause any problems with the tests I've tried so placed a DCHECK > just > > in case. > > It's easy to hit this DCHECK - just focus something that isn't a textfield and > press a key. E.g. focus a button after turning on Full Keyboard Access in OSX > System Preferences. > > (pressing a key on a dialog with no textfields will probably also trigger this) Ah. Duplicated "keyDownEvent_ = nil; // Handled." here as well. https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:96: const bool control = event.IsCommandDown(); On 2016/11/23 02:09:30, tapted wrote: > On 2016/11/22 14:33:48, themblsha wrote: > > Fixes TextfieldTest.CutCopyPaste. "Ensure [Ctrl]+[Shift]+[Insert] is a no-op" > > triggered Paste because it looked at Control modifier on Mac and actually sent > > Cmd+Shift+Insert. > > This doesn't look right -- E.g. -[BridgedContentView copy:] uses > EF_CONTROL_DOWN, (but also an edit command so it won't end up here). > > I'd suggest something like > > const bool control = event.IsControlDown() || event.IsCommandDown(); > > (i.e. no #ifdef guard - do that on all platforms - IsCommandDown should only > come up on mac anyway) > > > It might also be possible that the test doesn't make sense on Mac, or should > really be dispatching the event differently (e.g. EventGenerator::WINDOW), since > in "real" code (outside of tests) this is handled elsewhere on Mac. Just EventGenerator::WINDOW is not enough, but in conjunction with this fix and Textfield::GetAcceleratorForCommandId() it works fine. https://codereview.chromium.org/2505943002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1277: #endif On 2016/11/23 02:09:30, tapted wrote: > On 2016/11/22 14:33:48, themblsha wrote: > > Fixes TextfieldTest.KeysWithModifiersTest. MockInputMethod dispatches Cmd+2 > > KeyDown event directly to InsertChar(). > > This test already behaves correctly on CrOS. I think the right fix for this is > to change ui/events/base_event_utils.cc and add a `kSystemKeyModifierMask` for > Mac that is just EF_COMMAND_DOWN -- (Mac *shouldn't* include EF_ALT_DOWN for > the same reasons as described in the comment for IsSystemKeyModifier - i.e. Alt > is used to input extended characters on Mac). Whoa, this really fixes it. Thanks! https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest... ui/views/view_unittest.cc:2215: // TODO: these tests were initially commented out when getting aura to On 2016/11/23 02:09:31, tapted wrote: > This comment about aura doesn't apply to this test, since the test has never run > places other than mac. The TODO format isn't right (I thought there was a > presubmit for this?) > > Something like > > // TODO(themblsha): Bring this up on non-Mac platforms. It currently fails > because <reason>. See http://crbug.com/667757. > #if defined(OS_MACOSX) > > then the old comment should move back down where it was (with a separate #if) > > > (although, it is likely that it's failing for the same reason as the old tests - > it would be nice to figure out what that is..) Yeah, TestView::AcceleratorPressed() is not invoked for some reason, we probably need to set up the views a little bit differently. https://codereview.chromium.org/2505943002/diff/100001/ui/views/view_unittest... ui/views/view_unittest.cc:2225: const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN; On 2016/11/23 02:09:31, tapted wrote: > this case can be omitted if it's not being brought up on !mac. I thought it could live so it could be used in the future when the test is fixed on all platforms, but now I've remembered that you have a no-unused-code policy. Removed.
The CQ bit was checked by mblsha@yandex-team.ru 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: 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_...)
LGTM https://codereview.chromium.org/2505943002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2505943002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:101: const bool control = event.IsControlDown() || event.IsCommandDown(); This doesn't need to check the specific platform modifier? Returning true for "control" on Mac is OK?
lgtm again - thanks! https://codereview.chromium.org/2505943002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2505943002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:101: const bool control = event.IsControlDown() || event.IsCommandDown(); On 2016/11/23 17:35:22, Peter Kasting wrote: > This doesn't need to check the specific platform modifier? Returning true for > "control" on Mac is OK? I think this is right, and it's the simplest thing that works and is correct. GetCommandForKeyEvent shouldn't be called on Mac from OnKeyPressed: An edit command should get scheduled instead since that will obey key remappings, but the KeyEvent used to trigger that command is given a control modifier (not command) so that other View subclasses overriding OnKeyEvent don't have to worry about Mac specifics. However, GetCommandForKeyEvent probably _will_ be invoked by Textfield::AcceleratorPressed(..), which says it's invoked by BrowserView. I think this is after bouncing off the renderer, so it's harder to make guarantees there.
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2505943002/#ps120001 (title: "Fix tapted's review issues.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mblsha@yandex-team.ru
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
Exceeded global retry quota
The CQ bit was checked by tapted@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tapted@chromium.org
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": 1480046836677280,
"parent_rev": "b662cf7548a7a4a5c622fffb407276a2d90e38a7", "commit_rev":
"c6129bdb7bed57342dca89f75e3a0ff28892530c"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix accelerator handling while Omnibox is in focus. Currently in both Cocoa and MacViewsBrowser when Omnibox is in focus the accelerators are only handled in -[NativeWidgetMacNSWindow performKeyEquivalent:], which internally uses ChromeCommandDispatcherDelegate, which consults global_keyboard_shortcuts_cocoa_mac.mm's tables. In crbug.com/620315 we're trying to migrate the global_keyboard_shortcuts_cocoa_mac.mm's tables over to accelerator_table.cc. BUG=665823 ========== to ========== MacViews: Fix accelerator handling while Omnibox is in focus. Currently in both Cocoa and MacViewsBrowser when Omnibox is in focus the accelerators are only handled in -[NativeWidgetMacNSWindow performKeyEquivalent:], which internally uses ChromeCommandDispatcherDelegate, which consults global_keyboard_shortcuts_cocoa_mac.mm's tables. In crbug.com/620315 we're trying to migrate the global_keyboard_shortcuts_cocoa_mac.mm's tables over to accelerator_table.cc. BUG=665823 Committed: https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296 Cr-Commit-Position: refs/heads/master@{#434436} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e405f4e5df76138d888527165ea13446a1e08296 Cr-Commit-Position: refs/heads/master@{#434436} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
