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

[macos] Move TextInputPlugin outside of visible area #39031

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

knopp
Copy link
Member

@knopp knopp commented Jan 20, 2023

TextInputPlugin must be visible and in view hierarchy for emoji picker to work. This is trivial change that moves it completely outside of visible area.

Fixes flutter/flutter#118504

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

@flutter-dashboard

This comment was marked as off-topic.

@knopp
Copy link
Member Author

knopp commented Jan 20, 2023

@Hixie, does this need a test?

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

@knopp Thanks for the quick fix here, I really appreciate it! LGTM with a test or a test exemption.

You'll have better luck getting ahold of Hixie on Discord for a test exemption. Or, is it possible to test this? I didn't see anything in the NSTextView docs about getting the frame, but I'm not super familiar with AppKit.

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.

Thanks for fixing! Please add a test so we don't regress this.

You should be able to create a newly initialised plugin, then check NSIsEmptyRect(plugin.frame.size) in a new test in shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm

@knopp
Copy link
Member Author

knopp commented Jan 20, 2023

Thanks for fixing! Please add a test so we don't regress this.

You should be able to create a newly initialised plugin, then check NSIsEmptyRect(plugin.frame.size) in a new test in shell/platform/darwin/macos/framework/Source/FlutterTextInputPluginTest.mm

So, the original comment says that the TextInputPlugin view must not be empty. My linked PR has put the TextInputPlugin to view hierarchy (needed for emoji picker to work), which made it visible.

This PR fixed it by moving it out of visible area, while still keeping the TextInputPlugin not empty.

Hoever I played with this a bit, I made the TextInputPlugin empty (zero size), and everything worked as expected (no visual glitches, IME working as expected, accessibility working too). I think the original non empty size was enforced by @chunhtai in #25600, but it doesn't seem to be necessary?

@knopp
Copy link
Member Author

knopp commented Jan 23, 2023

@chunhtai, any chance you remember why non-empty rect was needed? I tried this with empty rect and everything seems to work just fine (including IME + voiceover). It is possible that the difference before and now is that the plugin is now in view hierarchy (it wasn't originally)/

I'm inclined to go with empty rect since it seems cleaner. Test added.

@cbracken
Copy link
Member

cbracken commented Jan 23, 2023

@chunhtai is out for couple weeks; should be back in early Feb I believe. If the empty rect works, I agree we should go with that. Was there any info in the commit comment or (since our infra was removing commit comments other than the first line, for a while) linked GitHub PR/Issue?

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2023
@auto-submit auto-submit bot merged commit 789a549 into flutter:main Jan 26, 2023
zanderso added a commit that referenced this pull request Jan 26, 2023
zanderso added a commit that referenced this pull request Jan 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 27, 2023
…119299)

* 789a549f4 [macos] Move TextInputPlugin outside of visible area (flutter/engine#39031)

* e17f58747 Revert "[macos] Move TextInputPlugin outside of visible area (#39031)" (flutter/engine#39176)

* a63d98feb Update buildroot to abada33. (flutter/engine#39173)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextField odd behavior , white indicator on the left corner
3 participants