Skip to content

Add Focus support for iOS platform view #103019

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 3 commits into from
May 24, 2022
Merged

Add Focus support for iOS platform view #103019

fluttergithubbot merged 3 commits into from
May 24, 2022

Conversation

@hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented May 4, 2022 •
edited
Loading

This PR makes iOS platform view focusable.

Since frameworks and engines are separate repos, I have to try-and-catch the non-existing "viewFocused" channel message for now. Will remove the try-catch after we landed and rolled the engine update.

The main idea is that:

  1. In the framework, a platform view needs to be wrapped under FocusNode with a onFocusChange callback saved
  2. In the engine, when a platform view is focused, it should send a channel message "viewFocused" to framework, which will invoke the onFocusChange saved in the previous step
  3. The onFocusChange will then send a channel message "TextInput.setPlatformViewClient" to the engine, which should remove the focus on previously focused text inputs (e.g. EditableText)

I am going through the Flutter Style Guide now (will check the box once I am done). But I tried my best to following existing conventions.

Related Issue

#34187

Pre-launch Checklist

I am going through the Flutter Style Guide now (will check the box once I am done). But I tried my best to following existing conventions.

  • 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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 4, 2022
@hellohuanlin hellohuanlin force-pushed the huan_platform_view_focus_in_framework branch from 0693de8 to 75b147b Compare May 9, 2022 23:03
@hellohuanlin hellohuanlin changed the title [wip] Add focus support for platform view Add Focus support for iOS platform view May 9, 2022
@hellohuanlin hellohuanlin marked this pull request as ready for review May 9, 2022 23:16
@hellohuanlin hellohuanlin requested review from cyanglaz and jmagman May 9, 2022 23:16
jmagman
jmagman reviewed May 9, 2022
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Some nits, @cyanglaz knows more about whether the actual focus logic is correct.

packages/flutter/lib/src/widgets/platform_view.dart Outdated
Copy link
Member

@jmagman jmagman May 9, 2022

Choose a reason for hiding this comment

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

Can you instead land this after the engine change?

Copy link
Contributor Author

@hellohuanlin hellohuanlin May 10, 2022

Choose a reason for hiding this comment

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

If we land engine first, there would be a similar problem when the engine sends "focusView" channel message that's not implemented in framework yet.

Copy link
Contributor

@cyanglaz cyanglaz May 16, 2022

Choose a reason for hiding this comment

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

Maybe split the PRs a little more so the landing orders would be:

  1. Framework receive method channel (it will be a non-op after landing it as the method channel will never be fired from engine)
  2. Engine implementation.
  3. Framework send method channel

This is a little more complicated but we can ensure there is no bad version or TODOs like this.

Copy link
Contributor Author

@hellohuanlin hellohuanlin May 16, 2022

Choose a reason for hiding this comment

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

@cyanglaz I think i can do that.

@jmagman
Copy link
Member

jmagman commented May 9, 2022

Also the ci.yaml failure is because you're branched off a commit from a few weeks ago, it's been fixed if you rebase to top of tree.

@hellohuanlin hellohuanlin force-pushed the huan_platform_view_focus_in_framework branch from 75b147b to d26d4d4 Compare May 10, 2022 18:26
@hellohuanlin hellohuanlin added platform-ios iOS applications specifically a: platform-views Embedding Android/iOS views in Flutter apps labels May 10, 2022
@jmagman
Copy link
Member

jmagman commented May 16, 2022

Friendly ping to take a look at this one @cyanglaz

cyanglaz
cyanglaz reviewed May 16, 2022
View reviewed changes
packages/flutter/lib/src/widgets/platform_view.dart Outdated
Copy link
Contributor

@cyanglaz cyanglaz May 16, 2022

Choose a reason for hiding this comment

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

Maybe split the PRs a little more so the landing orders would be:

  1. Framework receive method channel (it will be a non-op after landing it as the method channel will never be fired from engine)
  2. Engine implementation.
  3. Framework send method channel

This is a little more complicated but we can ensure there is no bad version or TODOs like this.

@hellohuanlin hellohuanlin force-pushed the huan_platform_view_focus_in_framework branch from e425bbc to f967e03 Compare May 20, 2022 20:21
@hellohuanlin hellohuanlin requested a review from cyanglaz May 20, 2022 20:21
@hellohuanlin hellohuanlin force-pushed the huan_platform_view_focus_in_framework branch from f967e03 to bbcc055 Compare May 20, 2022 20:23
jmagman
jmagman reviewed May 20, 2022
View reviewed changes
@hellohuanlin hellohuanlin force-pushed the huan_platform_view_focus_in_framework branch from bbcc055 to c45eb67 Compare May 20, 2022 23:18
@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented May 23, 2022

@cyanglaz can you take another look? thx

cyanglaz
cyanglaz approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit f852092 into flutter:master May 24, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 24, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 24, 2022
@engine-flutter-autoroll
f852092 Add Focus support for iOS platform view (flutter/flutter#103019)
53ae5f7
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 24, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 24, 2022
@engine-flutter-autoroll
f852092 Add Focus support for iOS platform view (flutter/flutter#103019)
15b669b
@hellohuanlin hellohuanlin mentioned this pull request May 24, 2022
Merged
8 tasks
hellohuanlin added a commit that referenced this pull request May 25, 2022
@hellohuanlin
Revert "Add Focus support for iOS platform view (#103019)"
5868058 
This reverts commit f852092.
@hellohuanlin hellohuanlin mentioned this pull request May 25, 2022
Closed
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@hellohuanlin @camsim99
Add Focus support for iOS platform view (flutter#103019)
30addcb
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
f852092 Add Focus support for iOS platform view (flutter/flutter#103019)
4b50273
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
f852092 Add Focus support for iOS platform view (flutter/flutter#103019)
a0aec32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman left review comments

+1 more reviewer

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: platform-views Embedding Android/iOS views in Flutter apps framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hellohuanlin @jmagman @cyanglaz @fluttergithubbot