Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[macOS] Forward key events to NSTextInputContext when there's composing text #32051

Merged
merged 2 commits into from
Mar 16, 2022
Merged

[macOS] Forward key events to NSTextInputContext when there's composing text #32051

merged 2 commits into from
Mar 16, 2022

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Mar 15, 2022 •
edited
Loading

Partially fixes flutter/flutter#85328, still does not work in the macOS accent menu

Quick demo:

Screen.Recording.2022-03-15.at.3.47.43.PM.mov

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cbracken
cbracken approved these changes Mar 15, 2022
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 15, 2022
dkwingsmt
dkwingsmt suggested changes Mar 15, 2022
View reviewed changes
shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm
[tester recordEmbedderCallsTo:embedderCallbacks];
[tester recordChannelCallsTo:channelCallbacks];
// TextInputPlugin does not claim the event.
[tester respondTextInputWith:NO];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used to set the responses from the mocked TextInputPlugin, not to check. (Maybe I should add a method to check its value...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't mean to use this as a "verification" statement. The verification step is at line 385 & 386 where we make sure the event doesn't go to the primary responder(s) even with text input not claiming the key event for itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so this line does little and should be removed. And still we haven't ensured that onTextInputKeyEvent is called.

shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm Outdated
Comment on lines 380 to 381
// TextInputPlugin does not claim the event.
[tester respondTextInputWith:NO];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TextInputPlugin does not claim the event.
[tester respondTextInputWith:NO];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can you create a way for the mocked text input plugin to record the events, just like the embedder and channel ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure ideally I want to verify that the text input plugin did get the event.

shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm Outdated

// Send another down event with composing == NO.
tester.isComposing = NO;
[tester respondTextInputWith:YES];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[tester respondTextInputWith:YES];

@dkwingsmt
Copy link
Contributor

Also, why is this not a long-term fix? I thought this was the only way to fix it.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • This pull request has changes requested by @dkwingsmt. Please resolve those before re-applying the label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 15, 2022
@LongCatIsLooong
Copy link
Contributor Author

Also, why is this not a long-term fix? I thought this was the only way to fix it.

macOS has all kinds of popovers that can be displayed during text input and should receive key events when visible, the IME candidate window is just one of them (other examples, the accent menu or the emoji menu). !isComposing doesn't mean there's no popover.

@LongCatIsLooong LongCatIsLooong force-pushed the forward-keyevents-to-system-when-composing branch from ea4d76c to f9247e4 Compare March 16, 2022 01:32
dkwingsmt
dkwingsmt approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LongCatIsLooong LongCatIsLooong added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 16, 2022
@fluttergithubbot fluttergithubbot merged commit cf875d4 into flutter:main Mar 16, 2022
@LongCatIsLooong LongCatIsLooong deleted the forward-keyevents-to-system-when-composing branch March 16, 2022 17:46
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
cf875d4 [macOS] Forward key events to NSTextInputContext when there's…
c9a7a2f 
… composing text (flutter/engine#32051)
@cbracken cbracken mentioned this pull request Mar 16, 2022
Closed
@boyan01 boyan01 mentioned this pull request Mar 17, 2022
Merged
zanderso pushed a commit to flutter/flutter that referenced this pull request Mar 17, 2022
@engine-flutter-autoroll
Roll Engine from ce9eef7 to 46c2cca (10 revisions) (#100244)
1cb9fb5 
* 9f13454 [windows] Support win_debug_x86 compilation target in engine (flutter/engine#30417)

* c5e958e Make tracing safe (flutter/engine#32042)

* cf875d4 [macOS] Forward key events to NSTextInputContext when there's composing text (flutter/engine#32051)

* ffd5c9c Roll Fuchsia Linux SDK from Ee9OX2o6P... to mVqiTwaVa... (flutter/engine#32055)

* b0018c3 Roll Skia from a9bc6c643791 to 8afe53fd76d3 (1 revision) (flutter/engine#32056)

* df12321 Roll Skia from 8afe53fd76d3 to 0d81bc7bb04e (2 revisions) (flutter/engine#32057)

* 398a274 Roll Fuchsia Mac SDK from jvlI1s78T... to vWlaMIVkM... (flutter/engine#32060)

* 3ffa9cf Roll Skia from 0d81bc7bb04e to e253cc3e55d3 (1 revision) (flutter/engine#32063)

* 275cd2b [web] Position spans absolutely within paragraph (flutter/engine#31907)

* 46c2cca Roll Skia from e253cc3e55d3 to 9301fe3779bb (1 revision) (flutter/engine#32064)
@justinmc justinmc mentioned this pull request Jun 1, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cbracken cbracken cbracken approved these changes

@dkwingsmt dkwingsmt dkwingsmt approved these changes

Assignees
No one assigned
Labels
platform-macos waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't navigate composing candidates list with arrow keys on Mac
4 participants
@LongCatIsLooong @dkwingsmt @fluttergithubbot @cbracken