[Windows] Propagate the enabled accessibility state#184501
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the propagation of enabled and read-only states to the accessibility tree. Changes in the common accessibility bridge set kDisabled or kReadOnly restrictions based on Flutter semantics, while the Windows bridge is updated to dispatch state change events upon enabled status updates. Unit tests are included to verify these state transitions and event dispatches. Feedback recommends ensuring the kEditable state is removed for disabled text fields to avoid conflicting accessibility states and handling READ_ONLY_CHANGED events in the Windows bridge to ensure state changes are properly propagated.
| if (flags->is_enabled == FlutterTristate::kFlutterTristateFalse) { | ||
| node_data.SetRestriction(ax::mojom::Restriction::kDisabled); | ||
| } else if (flags->is_read_only) { | ||
| node_data.SetRestriction(ax::mojom::Restriction::kReadOnly); | ||
| } |
There was a problem hiding this comment.
When a text field is disabled, it should not be marked as editable in the accessibility tree. Currently, if is_text_field is true and is_enabled is false, the node will have both the kEditable state (added in the preceding block) and the kDisabled restriction. This can lead to confusing behavior for assistive technologies. It is recommended to remove the kEditable state when the widget is disabled.
if (flags->is_enabled == FlutterTristate::kFlutterTristateFalse) {
node_data.SetRestriction(ax::mojom::Restriction::kDisabled);
node_data.RemoveState(ax::mojom::State::kEditable);
} else if (flags->is_read_only) {
node_data.SetRestriction(ax::mojom::Restriction::kReadOnly);
}There was a problem hiding this comment.
kEditable is only set if !isReadOnly (L373)
4490672 to
7d670ef
Compare
|
cc @hannah-hyj as this touches your recent tristate changes. |
mattkae
left a comment
There was a problem hiding this comment.
This looks great, thanks for the fantastic tests and well-defined scope 😄
|
@loic-peron-inetum-public Hello, it looks like this PR is based off a bad master commit. Could you rebase off of the latest master? Thanks! |
7d670ef to
3f07a5a
Compare
|
Rebased on master. |
3f07a5a to
58837ac
Compare
|
Reason for revert: breaks |
…#184501)" (flutter#186492) <!-- start_original_pr_link --> Reverts: flutter#184501 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: cbracken <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: breaks `FlutterPlatformNodeDelegateMac.SelectableTextHasCorrectSemantics` on macOS builds. * [Build 1](https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20mac_unopt/12646/overview), [Log](https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8681872617106699361/+/u/test:_Host_Tests_for_host_profile/stdout) * [Build 2](https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20ma <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: loic-peron-inetum-public <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {hannah-hyj, mattkae} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: This PR propagates the enabled/diabled/read-only state of Flutter widgets to the Windows Accessiblity tree. see flutter#184559 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
|
First off apologies for the revert. They're always a bummer but more frustrating when they're profile/release-only crashes. Warning: I haven't looked into this at all, but here's what Gemini had to say. My own commentary at the bottom. 1. Role Mapping Change in accessibility_bridge.ccIn commit c6f7839 , the role assignment logic in accessibility_bridge.cc was • Before the commit: A read-only text field failed this check and fell through to be assigned ax::mojom::Role::kStaticText . 2. Native Accessible Object Creation on macOSIn FlutterPlatformNodeDelegateMac.mm within FlutterPlatformNodeDelegateMac.mm , the underlying platform node Because the role changed from kStaticText to kTextField , the test node now instantiates a FlutterTextInputSemanticsObject.h instead of a standard AXPlatformNodeCocoa . 3. The Unloaded View in the Unit TestIn the failing test FlutterPlatformNodeDelegateMacTest.mm, the FlutterViewController.h is created, but (Note: Because the view controller's lifecycle is never started, runWithEntrypoint: is never invoked on the When the test calls GetNativeViewAccessible() to retrieve the native accessibility element, Because the view was never loaded, EnsureAttachedToView() returns false , causing GetNativeViewAccessible() to return nil . Before the commit, the standard AXPlatformNodeCocoa did not require an attached view and successfully returned a non-nil accessibility object. 4. The C++ Undefined Behavior (SIGSEGV)Finally, lines 100–102 of the test attempt to read the string value into a std::string : Since native_accessibility is nil :
In debug builds, standard library assertions might catch this or permit it to pass harmlessly, but in optimized profile and release builds, std::string attempts to call strlen(nullptr) , leading directly to the observed SIGSEGV segmentation fault. Okay @cbracken talking again now. What it's saying sounds plausible to me, but LLMs are great at saying things that sound plausible. If what it's saying is accurate (and that's often a very big "if"), then in We should be doing that regardless in most of these for the tests to approximate reality; not a lot of a11y typically happens in a viewcontroller that's not even onscreen. That'll turn this from a segfault into a test failure. There's the bigger question of what the correct behaviour is though product-wise for macOS. If you take a look at this code, you can see that we intentionally treat read-only text fields as static text: It seems like there are a couple possible options here: If we think read-only, selectable text fields should continue acting as static text on macOS, we'd need to update FlutterPlatformNodeDelegate's initialiser to only create a FlutterTextPlatformNode for editable text fields. That preserves the current behaviour. You'd need to update this code: Basically you'd change the current conditional check to something like: There might be more places you need to update but that seems like the most obvious to me. If, on the other hand, we think that read-only text fields should be exposed as native text fields on macos, then we'd need to ensure that read-only text fields are returned with role NSAccessibilityTextFieldRole and fix the code I mentioned above in FlutterTextInputSemanticsObject.mm and update the test expections. Again, there might be other places you need to update, and I'm assuming there must have been some reason we went with the approach we did -- maybe Chromium was doing that? What's right for the web isn't necessarily right for apps though. Someone would need to look at what stock AppKit/SwiftUI apps do and make a call on what the right approach is. If it were me... in the interest of getting Windows working, I'd take the first approach and leave the behaviour as-is on macOS. If we determine the right behaviour is the second one, then treat that as a separate change... or just file a bug and hope someone gets to it 😉. Even bigger caveat than taking Gemini with a grain of salt: it's a couple years since I worked on the macOS a11y code at all, so you'd probably want to check with the current owners. My recollection is that @chunhtai wrote the original implementation so is the best person I know of to offer thoughts on a fix. /cc @jmagman would know who the current experts on this are. |
…11713) Manual roll requested by bmparr@google.com flutter/flutter@23f6f58...0541913 2026-05-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Windows] Propagate the enabled accessibility state (#184501)" (flutter/flutter#186492) 2026-05-13 srawlins@google.com [dev] Use super parameters in missed spots (flutter/flutter#186193) 2026-05-13 loic.peron@inetum.com [Windows] Propagate the enabled accessibility state (flutter/flutter#184501) 2026-05-13 matt.boetger@gmail.com [flutter_tool] filter out MotionEvent-JNI warning spam from logcat (#174783) (flutter/flutter#186079) 2026-05-13 engine-flutter-autoroll@skia.org Roll Packages from 93cbed6 to 2ec2236 (1 revision) (flutter/flutter#186464) 2026-05-13 mdebbar@google.com [web] Fix untriaged issues link label (flutter/flutter#186465) 2026-05-13 bdero@google.com [Impeller] Namespace user-supplied shaders to prevent entrypoint collisions (flutter/flutter#186332) 2026-05-13 1063596+reidbaker@users.noreply.github.com [flutter_tools] Migrate detectLowCompileSdkVersionOrNdkVersion to AGP task (flutter/flutter#184731) 2026-05-13 jason-simmons@users.noreply.github.com Update the Flutter Gallery web app template files to support running with Wasm (flutter/flutter#186268) 2026-05-13 jason-simmons@users.noreply.github.com [web] Use heap allocation for buffers that would consume too much space on the Wasm stack (flutter/flutter#186228) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 56ca5896c0d9 to 27f7bba22600 (3 revisions) (flutter/flutter#186444) 2026-05-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from z7ICmPtn4hspu02zk... to y6uQHA5xUN83IF395... (flutter/flutter#186442) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 6385958d2feb to 56ca5896c0d9 (1 revision) (flutter/flutter#186441) 2026-05-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 9576691c37d8 to 8e30b88e4d5a (1 revision) (flutter/flutter#186429) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 77a21bc723dc to 6385958d2feb (9 revisions) (flutter/flutter#186428) 2026-05-13 164032450+AlexEduV@users.noreply.github.com Docs/improving docs for semantics UI lib (flutter/flutter#186125) 2026-05-12 jason-simmons@users.noreply.github.com [Tool] Support glob patterns when parsing workspaces in FlutterProject (flutter/flutter#185715) 2026-05-12 nico.reiab@gmail.com docs: fix overriden -> overridden in MediaQueryData dartdoc (flutter/flutter#186323) 2026-05-12 brackenavaron@gmail.com [Test cross imports] No material in `test/foundation`, `test/gestures`, `test/semantics`, `test/services` (flutter/flutter#186144) 2026-05-12 nico.reiab@gmail.com docs: fix "tha" -> "that" typo in widget_inspector_test comment (flutter/flutter#186322) 2026-05-12 nico.reiab@gmail.com docs: Fix doubled-word typos in framework dartdoc (flutter/flutter#186319) 2026-05-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186418) 2026-05-12 30870216+gaaclarke@users.noreply.github.com Bumped required mediatek vender sdk version. (flutter/flutter#186405) 2026-05-12 magder@google.com Make DeepLinkJsonFromManifestTask Gradle task build cacheable (flutter/flutter#185903) 2026-05-12 66727653+ishaq2321@users.noreply.github.com Harden dev tooling scripts against command injection and log leaks (flutter/flutter#186076) 2026-05-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186274) 2026-05-12 bdero@google.com [Flutter GPU] Allow allocating multi-mip textures and overwriting specific (mip, slice) levels (flutter/flutter#185890) 2026-05-12 zhongliu88889@gmail.com [web] Fix MenuAnchor dismiss when semantics enabled (flutter/flutter#183093) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#11713) Manual roll requested by bmparr@google.com flutter/flutter@23f6f58...0541913 2026-05-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Windows] Propagate the enabled accessibility state (#184501)" (flutter/flutter#186492) 2026-05-13 srawlins@google.com [dev] Use super parameters in missed spots (flutter/flutter#186193) 2026-05-13 loic.peron@inetum.com [Windows] Propagate the enabled accessibility state (flutter/flutter#184501) 2026-05-13 matt.boetger@gmail.com [flutter_tool] filter out MotionEvent-JNI warning spam from logcat (#174783) (flutter/flutter#186079) 2026-05-13 engine-flutter-autoroll@skia.org Roll Packages from 93cbed6 to 2ec2236 (1 revision) (flutter/flutter#186464) 2026-05-13 mdebbar@google.com [web] Fix untriaged issues link label (flutter/flutter#186465) 2026-05-13 bdero@google.com [Impeller] Namespace user-supplied shaders to prevent entrypoint collisions (flutter/flutter#186332) 2026-05-13 1063596+reidbaker@users.noreply.github.com [flutter_tools] Migrate detectLowCompileSdkVersionOrNdkVersion to AGP task (flutter/flutter#184731) 2026-05-13 jason-simmons@users.noreply.github.com Update the Flutter Gallery web app template files to support running with Wasm (flutter/flutter#186268) 2026-05-13 jason-simmons@users.noreply.github.com [web] Use heap allocation for buffers that would consume too much space on the Wasm stack (flutter/flutter#186228) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 56ca5896c0d9 to 27f7bba22600 (3 revisions) (flutter/flutter#186444) 2026-05-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from z7ICmPtn4hspu02zk... to y6uQHA5xUN83IF395... (flutter/flutter#186442) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 6385958d2feb to 56ca5896c0d9 (1 revision) (flutter/flutter#186441) 2026-05-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 9576691c37d8 to 8e30b88e4d5a (1 revision) (flutter/flutter#186429) 2026-05-13 engine-flutter-autoroll@skia.org Roll Skia from 77a21bc723dc to 6385958d2feb (9 revisions) (flutter/flutter#186428) 2026-05-13 164032450+AlexEduV@users.noreply.github.com Docs/improving docs for semantics UI lib (flutter/flutter#186125) 2026-05-12 jason-simmons@users.noreply.github.com [Tool] Support glob patterns when parsing workspaces in FlutterProject (flutter/flutter#185715) 2026-05-12 nico.reiab@gmail.com docs: fix overriden -> overridden in MediaQueryData dartdoc (flutter/flutter#186323) 2026-05-12 brackenavaron@gmail.com [Test cross imports] No material in `test/foundation`, `test/gestures`, `test/semantics`, `test/services` (flutter/flutter#186144) 2026-05-12 nico.reiab@gmail.com docs: fix "tha" -> "that" typo in widget_inspector_test comment (flutter/flutter#186322) 2026-05-12 nico.reiab@gmail.com docs: Fix doubled-word typos in framework dartdoc (flutter/flutter#186319) 2026-05-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186418) 2026-05-12 30870216+gaaclarke@users.noreply.github.com Bumped required mediatek vender sdk version. (flutter/flutter#186405) 2026-05-12 magder@google.com Make DeepLinkJsonFromManifestTask Gradle task build cacheable (flutter/flutter#185903) 2026-05-12 66727653+ishaq2321@users.noreply.github.com Harden dev tooling scripts against command injection and log leaks (flutter/flutter#186076) 2026-05-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186274) 2026-05-12 bdero@google.com [Flutter GPU] Allow allocating multi-mip textures and overwriting specific (mip, slice) levels (flutter/flutter#185890) 2026-05-12 zhongliu88889@gmail.com [web] Fix MenuAnchor dismiss when semantics enabled (flutter/flutter#183093) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This PR propagates the enabled/diabled/read-only state of Flutter widgets to the Windows Accessiblity tree.
see #184559
Pre-launch Checklist
///).