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

[macOS] Move A11yBridge to FVC #38855

Merged
auto-submit merged 1 commit into from
Jan 31, 2023
Merged

[macOS] Move A11yBridge to FVC #38855

auto-submit merged 1 commit into from
Jan 31, 2023

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jan 13, 2023 •
edited
Loading

This PR moves AccessibilityBridgeMac from FlutterEngine to FlutterViewController.

Currently, whether FlutterEngine has a non-null AccessibilityBridge only depends on the value of semanticsEnabled. The _bridge is initialized the moment semanticsEnabled turns true, even if viewController is null. Similarly, setting the view controller to null does not affect _bridge either. This is problematic because conceptually an AccessibilityBridge manages the a11y information for one view. Accessibility information does not make much sense without a view.

By moving the AccessibilityBridge into FlutterViewController, the lifecycles of these 2 classes are tied. The bridge exists only when the view controller exists and semantics has been enabled. This is important because accessibility information should be view-specific.

Before After
FlutterEngine#accessibilityBridge FlutterViewController#accessibilityBridge
FlutterEngine#updateSemantics: FlutterViewController#updateSemantics:
FlutterEngine#createAccessibilityBridge:viewController: FlutterViewController#createAccessibilityBridgeWithEngine:
- FlutterViewController#notifySemanticsEnabledChanged:

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.

loic-sharma
loic-sharma reviewed Jan 13, 2023
View reviewed changes
@loic-sharma loic-sharma requested a review from a-wallen January 13, 2023 23:45
loic-sharma
loic-sharma reviewed Jan 13, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 13, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 13, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 14, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 14, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 14, 2023
View reviewed changes
cbracken
cbracken reviewed Jan 14, 2023
View reviewed changes
@a-wallen
Copy link
Contributor

a-wallen commented Jan 17, 2023

Are we already committed to integers for viewIds?

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Jan 17, 2023

Are we already committed to integers for viewIds?

Not "committed", but what is your suggestion? Strings are not a good option for their cost in comparison and transmission.

@sergiy-sc
Copy link
Contributor

sergiy-sc commented Jan 17, 2023

Last, with this PR, FlutterViewController always initializes its keyboard manager and attaches itself to the engine, even if the engine has not been launched. Although @sergiy-sc's previous PR #36410 seems to have carefully avoided it, I don't see a reason not to do this (and it's much simpler). @sergiy-sc Do you remember why you made it the way?

That's because initializeKeyboard is called from launchEngine, but if engine is already running, it won't be launched again. So I initialized the keyboard in initWithEngine if it's running.

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Jan 17, 2023

Last, with this PR, FlutterViewController always initializes its keyboard manager and attaches itself to the engine, even if the engine has not been launched. Although @sergiy-sc's previous PR #36410 seems to have carefully avoided it, I don't see a reason not to do this (and it's much simpler). @sergiy-sc Do you remember why you made it the way?

That's because initializeKeyboard is called from launchEngine, but if engine is already running, it won't be launched again. So I initialized the keyboard in initWithEngine if it's running.

Thank you. In that case, I think the change in this PR is safe.

@a-wallen
Copy link
Contributor

a-wallen commented Jan 17, 2023

Not "committed", but what is your suggestion? Strings are not a good option for their cost in comparison and transmission.

I don't have anything in mind at the moment. Did you mention anything about it in your design doc?

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Jan 17, 2023 •
edited
Loading

Not "committed", but what is your suggestion? Strings are not a good option for their cost in comparison and transmission.

I don't have anything in mind at the moment. Did you mention anything about it in your design doc?

The doc does have a section "Surface ID" that says the ID would be a uint64_t. Honestly I didn't think about other options since a uint64_t is short and easy to use. The ID is meant to be opaque too (it is generated by the engine, not assigned by the user.)

a-wallen
a-wallen reviewed Jan 18, 2023
View reviewed changes
@a-wallen
Copy link
Contributor

a-wallen commented Jan 18, 2023

Nice work 👍

@loic-sharma
Copy link
Member

loic-sharma commented Jan 18, 2023

The viewId question is interesting given it is crucial for the framework/engine communication. The framework accepts any Object, so int64_t seems reasonable. We should double check with @goderbauer just in case though.

loic-sharma
loic-sharma reviewed Jan 18, 2023
View reviewed changes
loic-sharma
loic-sharma reviewed Jan 18, 2023
View reviewed changes
@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Jan 18, 2023 •
edited
Loading

@loic-sharma @a-wallen I've split a great part of this PR into #38981, so that this PR will contain only accessibility changes. For more discussion on non-accessibility changes, such as the IDs, let's move to that PR.

For incremental changes of this PR based on the base PR, see dkwingsmt#32.

@loic-sharma loic-sharma mentioned this pull request Jan 19, 2023
Merged
8 tasks
@dkwingsmt dkwingsmt self-assigned this Jan 19, 2023
@dkwingsmt dkwingsmt changed the title [macOS] Tie A11yBridge to FVC [macOS] Move A11yBridge to FVC Jan 25, 2023
@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Jan 25, 2023

@cbracken The depended PR has been merged, and this PR should be ready for review.

@dkwingsmt dkwingsmt mentioned this pull request Jan 26, 2023
Merged
8 tasks
chunhtai
chunhtai approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai 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 refactoring this!

loic-sharma
loic-sharma approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

Nice work! 🥳

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2023
@auto-submit auto-submit bot merged commit 999e5e5 into flutter:main Jan 31, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 31, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2023
@engine-flutter-autoroll
999e5e5fc Rebase all (flutter/engine#38855)
0447fcd
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 31, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 77218818138f to 9448f2966c11 (3 revisions) (#…
504e565 
…119592)

* 999e5e5fc Rebase all (flutter/engine#38855)

* 9cc2a8e8f Update bytes str code (flutter/engine#39275)

* 9448f2966 Revert "[fuchsia] Diagnostics directory rights are R* (#39203)" (flutter/engine#39271)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gspencergoog gspencergoog Awaiting requested review from gspencergoog

@goderbauer goderbauer Awaiting requested review from goderbauer

4 more reviewers

@cbracken cbracken cbracken left review comments

@a-wallen a-wallen a-wallen left review comments

@chunhtai chunhtai chunhtai approved these changes

@loic-sharma loic-sharma loic-sharma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dkwingsmt dkwingsmt

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-macos

Projects

Multi Window
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dkwingsmt @a-wallen @sergiy-sc @loic-sharma @cbracken @chunhtai