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

[iOS] Using UIView container as text input superview for textinput plugin #29108

Merged
fluttergithubbot merged 6 commits into from
Oct 25, 2021
Merged

[iOS] Using UIView container as text input superview for textinput plugin #29108

fluttergithubbot merged 6 commits into from
Oct 25, 2021

Conversation

@zhongwuzw
Copy link
Member

@zhongwuzw zhongwuzw commented Oct 11, 2021 •
edited
Loading

Fixes flutter/flutter#87809 some cases when makeKeyAndVisible not be called, keyWindow of UIApplication is nil, we can use AppDelegate's window as fallback.

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
Copy link

flutter-dashboard bot commented Oct 11, 2021

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Oct 11, 2021
@jmagman
Copy link
Member

jmagman commented Oct 11, 2021

@zhongwuzw Were you ever able to reproduce this?

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 12, 2021

@zhongwuzw Were you ever able to reproduce this?

@jmagman I didn't reproduce yet, but according to Apple Doc, keyWindow may not be set before makeKeyAndVisible sent, so use AppDelegate's window as a fallback should fix these rare cases.

@jmagman
Copy link
Member

jmagman commented Oct 12, 2021

@zhongwuzw Were you ever able to reproduce this?

@jmagman I didn't reproduce yet, but according to Apple Doc, keyWindow may not be set before makeKeyAndVisible sent, so use AppDelegate's window as a fallback should fix these rare cases.

I'm concerned that this API is deprecated and we may need to instead fall back to the UIWindowSceneDelegate.window on iOS 13+

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 13, 2021

@zhongwuzw Were you ever able to reproduce this?

@jmagman I didn't reproduce yet, but according to Apple Doc, keyWindow may not be set before makeKeyAndVisible sent, so use AppDelegate's window as a fallback should fix these rare cases.

I'm concerned that this API is deprecated and we may need to instead fall back to the UIWindowSceneDelegate.window on iOS 13+

UIWindowSceneDelegate is nil if the project does not use scene, UIWindowScene's keyWindow may use, but I don't know whether it can solve the issue.

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 18, 2021

@jmagman Any feedback about this PR? it seems some Flutter users are waiting for Flutter team to fix crash. :) flutter/flutter#87809

@jmagman
Copy link
Member

jmagman commented Oct 18, 2021

@LongCatIsLooong can you take a look at this? I don't have much context, the comment below this "The application must have a key window since the keyboard client must be part of the responder chain to function" makes me think this isn't the right change.

LongCatIsLooong
LongCatIsLooong reviewed Oct 18, 2021
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The change makes sense to me, although I'm not sure how best to test this as the crash seems to be specific to iOS 15+.

Alternatively, is it possible to pass in and let the text input plugin hold a weak ref to a host UIView to replace the keyWindow usage? Does semantics depend on this specific setup? /cc @cbracken
If it's possible to add new input views as children of the current FlutterViewController instead of as children of the key window we will probably be able to remove the press* methods too, as the key events would be redirected to FlutterViewController.

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 19, 2021

@LongCatIsLooong Using host's view seems ok, I tried to use FlutterViewController's 'view, input and semantics work correctly. Shall we need to switch to host's viewor just useAppDelegate's window` as a fallback?

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Oct 19, 2021

@zhongwuzw thanks for looking into it! Adding the input views to the view controller makes more sense to me, and from looking at the git history I didn't find any indication that adding the input views to the key window would break things.

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 20, 2021

@LongCatIsLooong I switched to host view.

LongCatIsLooong
LongCatIsLooong reviewed Oct 21, 2021
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

This needs a test. Please refer to the FlutterTextInputPluginTests.mm file for an example.

Also /cc @gaaclarke would you like to take a look?

gaaclarke
gaaclarke reviewed Oct 21, 2021
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

This makes sense to me. The objc code looks good. It doesn't have complete test coverage and I'll leave it up to @LongCatIsLooong to make sure it's logically correct wrt the TextInputPlugin.

LongCatIsLooong
LongCatIsLooong approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this!

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 22, 2021

@LongCatIsLooong I added the test. :)

@zhongwuzw zhongwuzw mentioned this pull request Oct 23, 2021
Closed
@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 Oct 25, 2021
@fluttergithubbot fluttergithubbot merged commit 18f3a55 into flutter:master Oct 25, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 25, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 25, 2021
@engine-flutter-autoroll
18f3a55 [iOS] Using application delegate window as fallback for texti…
4025bde 
…nput plugin (flutter/engine#29108)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 25, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 25, 2021
@engine-flutter-autoroll
18f3a55 [iOS] Using application delegate window as fallback for texti…
b9fb4f7 
…nput plugin (flutter/engine#29108)
@zhongwuzw zhongwuzw changed the title [iOS] Using application delegate window as fallback for textinput plugin [iOS] Using UIView container as text input superview for textinput plugin Oct 27, 2021
@TahaTesser TahaTesser mentioned this pull request Nov 24, 2021
Closed
@LongCatIsLooong LongCatIsLooong mentioned this pull request Dec 13, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@gaaclarke gaaclarke gaaclarke left review comments

@LongCatIsLooong LongCatIsLooong LongCatIsLooong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes needs tests platform-ios 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

5 participants

@zhongwuzw @jmagman @LongCatIsLooong @gaaclarke @fluttergithubbot