Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439953005:
Support NSFlagsChanged in ui::EventFromNative. (Closed)
|
Created:
4 years, 2 months ago by alshabalin Modified:
4 years, 1 month ago CC:
chromium-reviews, mac-reviews_chromium.org, tdresser+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport NSFlagsChanged in ui::EventFromNative.
On Windows and Linux pressing modifier only keys produce key pressed and
key released events but on Mac this wasn't the case. This change adds
support for NSFlagsChanged to equalize.
BUG=659854
Committed: https://crrev.com/e9e6f93ed2bbd520915f9545ec97b2a33211d2ae
Cr-Commit-Position: refs/heads/master@{#428607}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Unify NSFlagsChanged handling between content and ui/events. #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize the platform. BUG= ========== to ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG= ==========
alshabalin@yandex-team.ru changed reviewers: + sadrul@chromium.org
PTAL
sadrul@chromium.org changed reviewers: + tapted@chromium.org
+tapted@
Is there an overarching bug that this CL is fixing? (can you set a BUG=?) We shouldn't be doing this just to get rid of the NOTIMPLMENTED(). However, I can believe that we _do_ need it, I'm just not sure of the context in which you're encountering it. And it's likely that there is more plumbing required. For example, do we need to add a handler to -[BridgedContentView flagsChanged:]? A bug for context would help determine what else might be required. Thanks! https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:29: case VKEY_CAPITAL: There's no need to use the Windows keycodes that KeyboardCodeFromNSEvent gives - here we can switch on kVK_CapsLock, kVK_[Right]Shift, kVK_[Right]Control, kVK_[Right]Option, kVK_[Right]Command Just pass [native_event keyCode] you might need to #include Carbon https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:80: return EventTypeForFlagsChanged(native_event.modifierFlags, [native_event modifierFlags] - we only use dot notation for structs https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... File ui/events/cocoa/events_mac_unittest.mm (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:502: TEST_F(EventsMacTest, NSFlagsChanged) { test should have a comment above describing its purpose https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:504: NSEventModifierFlags modifierFlags; this isn't between @implementation/@end, so we should use hacker_style (i.e. modifier_flags - same for those below) https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:509: // CapsLock. optional: Perhaps include a const char* member, and put this comment in that then... https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:529: for (size_t i = 0; i < arraysize(test_cases); ++i) { ..then here I think you can do `for (const auto& test_case : test_cases) {` and you don't need the line below https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:531: SCOPED_TRACE(::testing::Message() << "While checking case #" << i); ... and here output the const char* https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:532: NSEvent* nativeEvent = cocoa_test_event_utils::KeyEventWithModifierOnly( native_event, or ns_event https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:534: auto event = EventFromNative(nativeEvent); this isn't a good use of `auto` per https://google.github.io/styleguide/cppguide.html#auto - use std::unique_ptr<Event> event https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... File ui/events/test/cocoa_test_event_utils.h (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... ui/events/test/cocoa_test_event_utils.h:79: // NSShiftKeyMask == true means Shift is pressed and |key_code| == kVK_Shift nit: `|modifiers| & NSShiftKeyMask == true` -> `(|modifiers| & NSShiftKeyMask) != 0` https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... ui/events/test/cocoa_test_event_utils.h:80: // with |modifiers| & NSShiftKeyMask == false means Shift is released. `|modifiers| & NSShiftKeyMask == false` -> `(|modifiers| & NSShiftKeyMask) == 0`
On 2016/10/25 06:55:10, tapted wrote: > Is there an overarching bug that this CL is fixing? (can you set a BUG=?) > > We shouldn't be doing this just to get rid of the NOTIMPLMENTED(). However, I > can believe that we _do_ need it, I'm just not sure of the context in which > you're encountering it. And it's likely that there is more plumbing required. > For example, do we need to add a handler to -[BridgedContentView flagsChanged:]? > A bug for context would help determine what else might be required. Thanks! In our fork of Chromium we have a feature that's activated by pressing Shift key while hovering mouse over web contents. I don't know if chrome has anything like it (is it possible for extensions to react to modifier-only key presses?). And you're probably right about needing -[BridgedContentView flagsChanged:].
On 2016/10/26 09:14:26, alshabalin wrote: > On 2016/10/25 06:55:10, tapted wrote: > > Is there an overarching bug that this CL is fixing? (can you set a BUG=?) > > > > We shouldn't be doing this just to get rid of the NOTIMPLMENTED(). However, I > > can believe that we _do_ need it, I'm just not sure of the context in which > > you're encountering it. And it's likely that there is more plumbing required. > > For example, do we need to add a handler to -[BridgedContentView > flagsChanged:]? > > A bug for context would help determine what else might be required. Thanks! > > In our fork of Chromium we have a feature that's activated by pressing Shift key > while > hovering mouse over web contents. I don't know if chrome has anything like it > (is it > possible for extensions to react to modifier-only key presses?). And you're > probably right > about needing -[BridgedContentView flagsChanged:]. Oh, and RenderWidgetHostViewMac supports NSFlagsChanged, so web contents do get modifer only key presses, it's just when you receive this event back in the browser (via WebContentsDelegate interface) you can't convert it to ui::Event.
On 2016/10/26 09:28:10, alshabalin wrote: > On 2016/10/26 09:14:26, alshabalin wrote: > > On 2016/10/25 06:55:10, tapted wrote: > > > Is there an overarching bug that this CL is fixing? (can you set a BUG=?) > > > > > > We shouldn't be doing this just to get rid of the NOTIMPLMENTED(). However, > I > > > can believe that we _do_ need it, I'm just not sure of the context in which > > > you're encountering it. And it's likely that there is more plumbing > required. > > > For example, do we need to add a handler to -[BridgedContentView > > flagsChanged:]? > > > A bug for context would help determine what else might be required. Thanks! > > > > In our fork of Chromium we have a feature that's activated by pressing Shift > key > > while > > hovering mouse over web contents. I don't know if chrome has anything like it > > (is it > > possible for extensions to react to modifier-only key presses?). And you're > > probably right > > about needing -[BridgedContentView flagsChanged:]. I can't think of anything in the current Chrome Mac UI, but pressing `Alt` on Windows does give focus to the Hotdog/App menu. Platform parity is important, and eventually we will want to deliver ui::Events to WebContents on Mac rather than NSEvents. We can reduce the added code a bit (see below). I added a BUG= for you. It doesn't need to say much, it just helps to have a way to possibly follow-up later with more related patches, or have a discussion separate from the codereview. You can add BridgedContentView handling in a separate CL, but it should have its own tests (perhaps a cross-platform one in view_unittest.cc or accelerator_unittest.cc (since that's probably how Alt is routed)). > Oh, and RenderWidgetHostViewMac supports NSFlagsChanged, so web contents > do get modifer only key presses, it's just when you receive this event back in > the > browser (via WebContentsDelegate interface) you can't convert it to ui::Event. Hm - so it looks like there's additional complexity here which has already been solved in content::. We don't want to have to solve all those problems again, so let's share the code that content is using rather than write our own EventTypeForFlagsChanged in web_input_event_builders_mac.mm there is IsKeyUpEvent() -- I think we can move it to ui/events/cocoa/cocoa_event_utils.h https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:80: return EventTypeForFlagsChanged(native_event.modifierFlags, And here just have case NSKeyDown: case NSKeyUp: case NSFlagsChanged: return IsKeyUpEvent(native_event) ? ET_KEY_RELEASED : ET_KEY_PRESSED;
Description was changed from ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG= ========== to ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG=659854 ==========
alshabalin@yandex-team.ru changed reviewers: + aelias@chromium.org
+ aelias for content/browser/renderer_host/input https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.mm File ui/events/cocoa/events_mac.mm (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.... ui/events/cocoa/events_mac.mm:80: return EventTypeForFlagsChanged(native_event.modifierFlags, On 2016/10/27 00:33:05, tapted wrote: > And here just have > > case NSKeyDown: > case NSKeyUp: > case NSFlagsChanged: > return IsKeyUpEvent(native_event) ? ET_KEY_RELEASED : ET_KEY_PRESSED; Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... File ui/events/cocoa/events_mac_unittest.mm (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:502: TEST_F(EventsMacTest, NSFlagsChanged) { On 2016/10/25 06:55:10, tapted wrote: > test should have a comment above describing its purpose Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:504: NSEventModifierFlags modifierFlags; On 2016/10/25 06:55:10, tapted wrote: > this isn't between @implementation/@end, so we should use hacker_style (i.e. > modifier_flags - same for those below) Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:509: // CapsLock. On 2016/10/25 06:55:10, tapted wrote: > optional: Perhaps include a const char* member, and put this comment in that > then... Good idea, thanks! Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:532: NSEvent* nativeEvent = cocoa_test_event_utils::KeyEventWithModifierOnly( On 2016/10/25 06:55:10, tapted wrote: > native_event, or ns_event Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac_... ui/events/cocoa/events_mac_unittest.mm:534: auto event = EventFromNative(nativeEvent); On 2016/10/25 06:55:10, tapted wrote: > this isn't a good use of `auto` per > https://google.github.io/styleguide/cppguide.html#auto - use > std::unique_ptr<Event> event Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... File ui/events/test/cocoa_test_event_utils.h (right): https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... ui/events/test/cocoa_test_event_utils.h:79: // NSShiftKeyMask == true means Shift is pressed and |key_code| == kVK_Shift On 2016/10/25 06:55:10, tapted wrote: > nit: `|modifiers| & NSShiftKeyMask == true` -> `(|modifiers| & NSShiftKeyMask) > != 0` Done. https://codereview.chromium.org/2439953005/diff/1/ui/events/test/cocoa_test_e... ui/events/test/cocoa_test_event_utils.h:80: // with |modifiers| & NSShiftKeyMask == false means Shift is released. On 2016/10/25 06:55:10, tapted wrote: > `|modifiers| & NSShiftKeyMask == false` -> `(|modifiers| & NSShiftKeyMask) == 0` Done.
On 2016/10/27 00:33:05, tapted wrote: > On 2016/10/26 09:28:10, alshabalin wrote: > > On 2016/10/26 09:14:26, alshabalin wrote: > > > On 2016/10/25 06:55:10, tapted wrote: > > > > Is there an overarching bug that this CL is fixing? (can you set a BUG=?) > > > > > > > > We shouldn't be doing this just to get rid of the NOTIMPLMENTED(). > However, > > I > > > > can believe that we _do_ need it, I'm just not sure of the context in > which > > > > you're encountering it. And it's likely that there is more plumbing > > required. > > > > For example, do we need to add a handler to -[BridgedContentView > > > flagsChanged:]? > > > > A bug for context would help determine what else might be required. > Thanks! > > > > > > In our fork of Chromium we have a feature that's activated by pressing Shift > > key > > > while > > > hovering mouse over web contents. I don't know if chrome has anything like > it > > > (is it > > > possible for extensions to react to modifier-only key presses?). And you're > > > probably right > > > about needing -[BridgedContentView flagsChanged:]. > > I can't think of anything in the current Chrome Mac UI, but pressing `Alt` on > Windows does give focus to the Hotdog/App menu. Platform parity is important, > and eventually we will want to deliver ui::Events to WebContents on Mac rather > than NSEvents. We can reduce the added code a bit (see below). On Linux pressing Alt doesn't seem to focus App menu. Can it be platform specific accessibility accelerator? > I added a BUG= for you. It doesn't need to say much, it just helps to have a way > to possibly follow-up later with more related patches, or have a discussion > separate from the codereview. Thanks! > You can add BridgedContentView handling in a separate CL, but it should have its > own tests (perhaps a cross-platform one in view_unittest.cc or > accelerator_unittest.cc (since that's probably how Alt is routed)). I'd better do that in a separate CL when I get access to my Windows box to see what's going on. > > Oh, and RenderWidgetHostViewMac supports NSFlagsChanged, so web contents > > do get modifer only key presses, it's just when you receive this event back in > > the > > browser (via WebContentsDelegate interface) you can't convert it to ui::Event. > > Hm - so it looks like there's additional complexity here which has already been > solved in content::. We don't want to have to solve all those problems again, so > let's share the code that content is using rather than write our own > EventTypeForFlagsChanged > > in web_input_event_builders_mac.mm there is IsKeyUpEvent() -- I think we can > move it to ui/events/cocoa/cocoa_event_utils.h Done, thanks! > https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.mm > File ui/events/cocoa/events_mac.mm (right): > > https://codereview.chromium.org/2439953005/diff/1/ui/events/cocoa/events_mac.... > ui/events/cocoa/events_mac.mm:80: return > EventTypeForFlagsChanged(native_event.modifierFlags, > And here just have > > case NSKeyDown: > case NSKeyUp: > case NSFlagsChanged: > return IsKeyUpEvent(native_event) ? ET_KEY_RELEASED : ET_KEY_PRESSED;
lgtm
content/browser lgtm
The CQ bit was checked by alshabalin@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
stamp lgtm
The CQ bit was checked by alshabalin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG=659854 ========== to ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG=659854 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG=659854 ========== to ========== Support NSFlagsChanged in ui::EventFromNative. On Windows and Linux pressing modifier only keys produce key pressed and key released events but on Mac this wasn't the case. This change adds support for NSFlagsChanged to equalize. BUG=659854 Committed: https://crrev.com/e9e6f93ed2bbd520915f9545ec97b2a33211d2ae Cr-Commit-Position: refs/heads/master@{#428607} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e9e6f93ed2bbd520915f9545ec97b2a33211d2ae Cr-Commit-Position: refs/heads/master@{#428607} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
