Skip to content
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

(#112207) Adding view_id parameter to DispatchSemanticsAction and UpdateSemantics #164577

Merged
merged 62 commits into from
Mar 20, 2025
Merged

(#112207) Adding view_id parameter to DispatchSemanticsAction and UpdateSemantics #164577

merged 62 commits into from
Mar 20, 2025
+905 −317

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Mar 4, 2025 •
edited by loic-sharma
Loading

fixes #112207

What's new?

  • Added a view_id on UpdateSemantics
  • Added a view_id on DispatchSemanticsAction
  • Piped the view_id all over creation
  • Updated tests for these actions across the different platforms
  • Added FlutterEngineSendSemanticsAction to the embedder API in order to not break FlutterEngineDispatchSemanticsAction
  • Using this view ID properly on the Windows platform (see engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc)

How to test

  1. Checkout foundation-plus-framework from canonical/flutter
  2. Merge this branch into it
  3. Enable the "Narrator" screen reader on windows
  4. Run the Multi window reference app (see PR for details)
  5. Open up another window, and note that the right buttons and things are being highlighted, as the screenreader would expect 🎉

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], 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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine repository. See also e: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-windows Building on or for Windows specifically platform-linux Building on or for Linux specifically a: desktop Running on desktop labels Mar 4, 2025
@mattkae mattkae changed the title Initial wip (#112207) Initial WIP of piping the view_id through the semantics actions Mar 4, 2025
@github-actions github-actions bot added platform-ios iOS applications specifically platform-fuchsia Fuchsia code specifically team-ios Owned by iOS platform team labels Mar 7, 2025
@github-actions github-actions bot added the platform-android Android applications specifically label Mar 7, 2025
@github-actions github-actions bot added the platform-web Web applications specifically label Mar 8, 2025
@github-actions github-actions bot removed the platform-web Web applications specifically label Mar 10, 2025
@mattkae mattkae requested review from loic-sharma and chunhtai March 17, 2025 17:39
@reidbaker
Copy link
Contributor

@chunhtai would you like an android review on this pr? It showed up during our triage.

chunhtai
chunhtai approved these changes Mar 19, 2025
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 for iOS, Android, shell<-> dart code, just some minor comment

engine/src/flutter/lib/ui/platform_dispatcher.dart
@@ -1332,14 +1334,14 @@ class PlatformDispatcher {
}

// Called from the engine, via hooks.dart
void _dispatchSemanticsAction(int nodeId, int action, ByteData? args) {
void _dispatchSemanticsAction(int viewId, int nodeId, int action, ByteData? args) {
_invoke1<SemanticsActionEvent>(
Copy link
Contributor

Choose a reason for hiding this comment

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

No action needed in this pr since we already integrate viewId into SemanticsActionEvent.

However, after taking a closer look, it feels to me we should also move onSemanticsActionEvent to flutter view given we already move the update semantics to flutter view.

This way, both semanticsupdate and semanticsactionEvent don't need to be annontated with viewId.

engine/src/flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm Outdated
@@ -20,6 +20,7 @@
namespace {

constexpr int32_t kSemanticObjectIdInvalid = -1;
constexpr int64_t kImplicitViewId = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use kFlutterImplicitViewId from common/constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@chunhtai
Copy link
Contributor

Hi @reidbaker , the android portion is basically just adding a todo #142845. so not something that requires android team's attention for now.

loic-sharma
loic-sharma approved these changes Mar 19, 2025
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.

Looks good, excellent work!

@mattkae mattkae enabled auto-merge March 19, 2025 20:27
@mattkae mattkae added this pull request to the merge queue Mar 20, 2025
Merged via the queue into flutter:master with commit 554c814 Mar 20, 2025
172 of 173 checks passed
@mattkae mattkae deleted the feature/112207 branch March 20, 2025 17:18
@Piinks Piinks mentioned this pull request Mar 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robert-ancell robert-ancell robert-ancell approved these changes

@justinmc justinmc justinmc approved these changes

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

@chunhtai chunhtai chunhtai approved these changes

@hbatagelo hbatagelo Awaiting requested review from hbatagelo

@knopp knopp Awaiting requested review from knopp

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: desktop Running on desktop a: text input Entering text in a text field or keyboard related problems engine flutter/engine repository. See also e: labels. platform-android Android applications specifically platform-fuchsia Fuchsia code specifically platform-ios iOS applications specifically platform-linux Building on or for Linux specifically platform-macos platform-windows Building on or for Windows specifically team-ios Owned by iOS platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SemanticsActions need to be associated with a view
7 participants
@mattkae @loic-sharma @reidbaker @chunhtai @chinmaygarde @robert-ancell @justinmc