Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS: Should never encounter KeyData when transitMode is rawKeyData. #87339

Closed
knopp opened this issue Jul 30, 2021 · 18 comments · Fixed by flutter/engine#27817
Closed

macOS: Should never encounter KeyData when transitMode is rawKeyData. #87339

knopp opened this issue Jul 30, 2021 · 18 comments · Fixed by flutter/engine#27817
Labels
a: text input Entering text in a text field or keyboard related problems platform-mac Building on or for macOS specifically

Comments

@knopp
Copy link
Member

knopp commented Jul 30, 2021

Happens after the keyboard event refactor landed.

[ERROR:flutter/lib/ui/ui_dart_state.cc(209)] Unhandled Exception: 'package:flutter/src/services/hardware_keyboard.dart':
Failed assertion: line 787 pos 16: 'false': Should never encounter KeyData when transitMode is rawKeyData.

It happens in the following scenario: Register macOS global hot key, on hot key create new flutter window/engine and show it. However I assume this would also happen in different multi window scenarios (and is not necessarily tied to a hot key).

What happens is that cocoa immediately sends NSEventTypeFlagsChanged to the new window with event.modifierFlags == 256, which basically means no modifiers.

However the code in FlutterChannelKeyResponder does this:

    case NSEventTypeFlagsChanged:
      if (event.modifierFlags < _previouslyPressedFlags) {
        type = @"keyup";
      } else if (event.modifierFlags > _previouslyPressedFlags) {
        type = @"keydown";
      } else {
        // ignore duplicate modifiers; This can happen in situations like switching
        // between application windows when MacOS only sends the up event to new window.
        return;
      }

_previouslyPressedFlags is 0, so this always generates key down event even though there aren't any modifiers present. Which in turns sets _transitMode in HardwareKeyboard to KeyDataTransitMode.rawKeyData, because this is sent before any actual ui keyboard event. After this the keyboard no longer works in window resulting in message above.

Potential solution would be either initializing _previouslyPressedFlags to 256, or check if event.modifierFlags > 256 before generating key down event.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

/cc @dkwingsmt

@knopp knopp added platform-mac Building on or for macOS specifically a: text input Entering text in a text field or keyboard related problems labels Jul 30, 2021
@dkwingsmt
Copy link
Contributor

Hi, this issue might have been fixed by flutter/engine#27774 (which has been merged but not rolled in yet). Can you checkout the latest engine and see if the problem persists?

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

Hi, this issue might have been fixed by flutter/engine#27774 (which has been merged but not rolled in yet). Can you checkout the latest engine and see if the problem persists?

I tried it with flutter/engine#27774 in and there is no difference. It have something to with CMD key being held down when new window is opened. Cocoa sends NSEventTypeFlagsChanged with empty flags (0x100) to new window, which makes FlutterChannelKeyResponder send a key down event while FlutterEmbedderKeyResponder simply ignores the event. This makes the framework switch to KeyDataTransitMode.rawKeyData.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jul 30, 2021

Ok. In that case, is it possible to resolve by changing the embedder responder to send an empty event, instead of changing the channel responder to not sending the raw event?

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

Ok. In that case, can you switch to a solution that changes the embedder handler to send and empty event, instead of changing the channel handler to not sending the raw event?

I'm not sure I understand why. Why should NSEventTypeFlagsChanged with modifierFlags 0x100 generate a key down event?

@dkwingsmt
Copy link
Contributor

All empty ui.KeyDatas are simply ignored by the framework, but are sent just to avoid crashes as described in this ticket. Their event type doesn't matter. The reason I think changing the embedder responder is better is because in that way we can preserve the channel responder's behavior and keep it easy to describe.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

@dkwingsmt , I modified [FlutterEmbedderKeyResponder handleFlagEvent:callback:to send empty event if (lastTargetPressed == shouldBePressed), which resolves the problem, but now I'm seeing this in log:

flutter: ══╡ EXCEPTION CAUGHT BY SERVICES LIBRARY ╞══════════════════════════════════════════════════════════
flutter: The following assertion was thrown during a platform message callback:
flutter: Attempted to send a key down event when no keys are in keysPressed. This state can occur if the key
flutter: event being sent doesn't properly set its modifier flags. This was the event:
flutter: RawKeyDownEvent#c8e10(logicalKey: LogicalKeyboardKey#00106(keyId: "0x200000106", keyLabel: "Meta
flutter: Left", debugName: "Meta Left"), physicalKey: PhysicalKeyboardKey#700e3(usbHidUsage: "0x000700e3",
flutter: debugName: "Meta Left")) and its data: RawKeyEventDataMacOs#e646c(characters,
flutter: charactersIgnoringModifiers, keyCode: 55, modifiers: 256)
flutter: 'package:flutter/src/services/raw_keyboard.dart':
flutter: Failed assertion: line 668 pos 7: 'event is! RawKeyDownEvent || _keysPressed.isNotEmpty'
flutter: 
flutter: Either the assertion indicates an error in the framework itself, or we should provide substantially
flutter: more information in this error message to help you determine and fix the underlying cause.
flutter: In either case, please report this assertion by filing a bug on GitHub:
flutter:   https://github.com/flutter/flutter/issues/new?template=2_bug.md
flutter: 
flutter: When the exception was thrown, this was the stack:
flutter: #2      RawKeyboard.handleRawKeyEvent (package:flutter/src/services/raw_keyboard.dart:668:7)
flutter: #3      KeyEventManager.handleRawKeyMessage (package:flutter/src/services/hardware_keyboard.dart:818:33)
flutter: #4      BasicMessageChannel.setMessageHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:73:49)
flutter: #5      BasicMessageChannel.setMessageHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:72:47)
flutter: #6      _DefaultBinaryMessenger.setMessageHandler.<anonymous closure> (package:flutter/src/services/binding.dart:379:35)
flutter: #7      _DefaultBinaryMessenger.setMessageHandler.<anonymous closure> (package:flutter/src/services/binding.dart:376:46)
flutter: #8      _invoke2.<anonymous closure> (dart:ui/hooks.dart:203:15)
flutter: #12     _invoke2 (dart:ui/hooks.dart:202:10)
flutter: #13     _ChannelCallbackRecord.invoke (dart:ui/channel_buffers.dart:42:5)
flutter: #14     _Channel.push (dart:ui/channel_buffers.dart:132:31)
flutter: #15     ChannelBuffers.push (dart:ui/channel_buffers.dart:329:17)
flutter: #16     PlatformDispatcher._dispatchPlatformMessage (dart:ui/platform_dispatcher.dart:536:22)
flutter: #17     _dispatchPlatformMessage (dart:ui/hooks.dart:90:31)
flutter: (elided 5 frames from class _AssertionError and dart:async)

What happens is that this is key-up event of CMD (meta key) delivered to newly opened window, which FlutterChannelKeyResponder confuses with key down. This definitely seems like a bug in FlutterChannelKeyResponder. This particular stacktrace is probably not new, it was just overlooked because unlike the original issue this one doesn't prevent further input.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

In other words, if I'm not mistaken something like this should not exist:

RawKeyDownEvent#c8e10(logicalKey: LogicalKeyboardKey#00106(keyId: "0x200000106", keyLabel: "Meta
Left", debugName: "Meta Left"), physicalKey: PhysicalKeyboardKey#700e3(usbHidUsage: "0x000700e3",
debugName: "Meta Left")) and its data: RawKeyEventDataMacOs#e646c(characters,
charactersIgnoringModifiers, keyCode: 55, modifiers: 256)

In macOS modifiers 256 means no modifiers. Meta Left KeyDown event should have meta in the modifiers. FlutterChannelKeyResponder should not produce event like this.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jul 30, 2021

Thanks for investigation! That's interesting. Now, let's submit the embedder responder PR separately, and talk about the channel responder fix.

It seems like we're receiving more modifier flags than we care, which sounds like a dangerous move since we are directly comparing modifier flags with event.modifierFlags < _previouslyPressedFlags. Maybe we can mask the incoming modifierFlags in the channel responder so that only flags mentioned in raw_keyboard_macos remain. Can you try if it works?

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

I'm not entirely sure why no modifier in event.modifierFlags has the 0x100 bit set. It doesn't seem to be documented anywhere.

So something like this seems to works fine:

      NSEventModifierFlags modifierFlags = event.modifierFlags & ~0x100;
      if (modifierFlags < _previouslyPressedFlags) {
        type = @"keyup";
      } else if (modifierFlags > _previouslyPressedFlags) {
        type = @"keydown";
      } else {
        // ignore duplicate modifiers; This can happen in situations like switching
        // between application windows when MacOS only sends the up event to new window.
        return;
      }

I assume masking this with flags in raw_keyboard_macos would work same (none of those is 0x100), but it's bit too late for me to try that.

There is also NSEventModifierFlagDeviceIndependentFlagsMask which might be relevant here (but is missing modifierLeftControl, etc). Not sure we need all those modifiers for comparison. If we do perhaps just ~0x100 is less error prone.

@dkwingsmt
Copy link
Contributor

I was considering NSEventModifierFlagDeviceIndependentFlagsMask too but it will not work, since apparently many flags relied by raw_keyboard_macos is out of it.

~0x100 should be fine too. So would you like to submit the PRs or do you want me to do it?

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

I'll update the original PR. Just a sec.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

@dkwingsmt, the PR is here.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

FWIW, with this in place I have no need for the empty event in FlutterEmbedderKeyResponder. With channel responder not sending the "made up" keydown event there's no problem with transit mode anymore.

@dkwingsmt
Copy link
Contributor

I'd still appreciate if you can add that change in (otherwise I will do it anyway). We're changing the protocol so that every key message (anything received by the system) must send some key data, even an empty one. Otherwise it might cause future problems.

@knopp
Copy link
Member Author

knopp commented Jul 30, 2021

Sure, I can do it tomorrow.

@knopp
Copy link
Member Author

knopp commented Jul 31, 2021

Sure, I can do it tomorrow.

@dkwingsmt, I gave it a shot (I added [self sendEmptyEvent]; to handleFlagEvent:callback: if lastTargetPressed == shouldBePressed, but it broke the SynthesizeMissedModifierEvents test in a way where I wasn't sure I'm doing the right thing. So it's probably best if you take a look at it :)

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: text input Entering text in a text field or keyboard related problems platform-mac Building on or for macOS specifically
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants