Skip to content

OLYM-9850: [macOS] A control that is off reads as dimmed to VoiceOver (cherry-pick of #190353, plus a selector fix) - #12

Merged
MylioClaude merged 3 commits into
OlymEDRSupportfrom
claude/olym-9850
Nov 11, 2026
Merged

OLYM-9850: [macOS] A control that is off reads as dimmed to VoiceOver (cherry-pick of #190353, plus a selector fix)#12
MylioClaude merged 3 commits into
OlymEDRSupportfrom
claude/olym-9850

Conversation

@MylioClaude

@MylioClaude MylioClaude commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes OLYM-9850: on the Mac, VoiceOver read buttons and checkboxes that are off as working controls, with no "dimmed" (People's "Ignore untagged faces" and "Restore ignored faces" with no faces on the photo, Tab settings' "Info" row, the right-rail test row RR-SR04).

Root cause, in two parts.

  1. The shared desktop accessibility bridge (shell/platform/common/accessibility_bridge.cc) never set a node's restriction. ax_platform_node_mac.mm answers AXEnabled from that restriction (!= kDisabled), so every Flutter control reached macOS with AXEnabled = YES, although Flutter's semantics said hasEnabledState without isEnabled. Upstream fixed this in a11y: Map disabled/read-only semantics to AX node restriction flutter/flutter#190353 (30c289b, 2026-08-03). It is first in the 3.50 beta; no stable release has it (checked 3.47.9).
  2. With only that change, a disabled control got worse on a real Mac: it reached macOS with no role, no name and no AXEnabled. -[AXPlatformNodeCocoa isAccessibilitySelectorAllowed:] returns NO for every selector of a disabled node, and AppKit asks it for every accessibility method this class implements, the getters included. Upstream master has the same code, so upstream's fix has the same effect there; its tests call the getters directly, which skips AppKit's check.

Fix (3 commits).

  1. A cherry-pick of upstream a11y: Map disabled/read-only semantics to AX node restriction flutter/flutter#190353, author kept: kDisabled when is_enabled is false, kReadOnly for read-only nodes, with its bridge tests. Taken without its macOS test, which checks upstream macOS: Set static text accessibility role for read-only text fields flutter/flutter#190330 (a disabled text field shown as static text). macOS: Set static text accessibility role for read-only text fields flutter/flutter#190330 is not carried: it changes how a text field is exposed, which this branch's OLYM-9024 and OLYM-9141 commits also change, and a disabled text field shown that way would still reach the FlutterTextField cast in AccessibilityBridgeMac's VALUE_CHANGED case. So a disabled text field stays as it is today.
  2. A macOS test, FlutterPlatformNodeDelegateMac.DisabledControlsAreNotAccessibilityEnabled: a disabled button and a disabled checked checkbox report isAccessibilityEnabled NO, an enabled button YES.
  3. isAccessibilitySelectorAllowed: on a disabled node refuses only the setters and the accessibilityPerform… actions, like a disabled AppKit control, and allows the getters. The same test checks it. UPSTREAM-CANDIDATES.md: the cherry-pick under "Not candidates", and the selector fix as entry 14.

Windows reads the same restriction (UIA IsEnabled, MSAA unavailable), so NVDA should now also hear disabled controls as unavailable. I could not try that here.

How I tested (sysmac4, macOS 26.7.1; engine built on sysmac3)

  • Built the macOS engine (host_debug_unopt_arm64, flutter_framework) from this branch.
  • Built Olym main at 79111918f4, Inproc - Debug, through the Mylio extension, put this engine into the built app, and started it with "Inproc - Debug (No Build)". Post-upgrade Sample Library (793 photos), ML off.
  • Clicked the first photo (5A7A3469, no faces) and opened Info ▸ People; opened Tab settings (the gear).
  • Read the macOS accessibility tree (the AX API, what VoiceOver reads) and used real VoiceOver (Item Chooser, Control-Option-I).
  • Results:
    • With part 1 only: the two greyed face buttons were elements with no role and no name. That is how part 2 was found.
    • With all 3 commits: AXButton "Ignore untagged faces" AXEnabled=0, AXButton "Restore ignored faces" AXEnabled=0, AXButton "Rescan faces" AXEnabled=1; AXCheckBox "Info" value=1 AXEnabled=0, the other tab rows AXEnabled=1.
    • VoiceOver's Item Chooser lists "Ignore untagged faces dimmed button", "Restore ignored faces dimmed button" and "Info dimmed checked checkbox". On the button it says "This item is dimmed".
    • Clean run: no app exceptions, no error lines in the run's logs, no new panics.log entry.
  • Not run: the engine unit tests, including the new and changed ones (the swarm's check-in rules leave tests to CI and the standing builds).

After the merge. Someone runs Olym's Build Engine workflow for macos (and windows, windows-arm64, which share the bridge) so new tarballs go up under v3.41.2-edr. Olym's engine injector notices the new tarballs by their ETag, so nothing in Olym changes.

Ticket: https://mylio.atlassian.net/browse/OLYM-9850

🤖 Generated with Claude Code

cbracken and others added 3 commits November 11, 2026 08:25
@cbracken Mylio Sysops
a11y: Map disabled/read-only semantics to AX node restriction (flutte…
b9af5f5 
…r#190353)

This is a partial re-land of
flutter#184501, which just lands the
common bridge code and tests as well as a macOS test for behaviour
landed in landed in flutter#190330.

This sets AXNodeData::SetRestriction() from the Flutter semantics flags:
kDisabled when is_enabled is false, and kReadOnly for read-only text
fields. Previously the restriction was never populated, so disabled and
read-only nodes were indistinguishable from editable/enabled ones at the
platform layer.

We also add a macOS regression test that checks that a disabled,
editable text field is exposed as static text rather than a
FlutterTextField.

This guards the !GetData().IsReadOnlyOrDisabled() check in
FlutterPlatformNodeDelegateMac::Init. The GetData() override rewrites a
disabled field's role to kStaticText but leaves kEditableRoot set, so
IsTextField() stays true and the restriction check is what prevents an
editable FlutterTextField from being created. I've added this in
response to a bad comment by the Gemini review bot to ensure that this
doesn't regress.

Issue: flutter#184559

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

## 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.
- [X] 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.

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

If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

(cherry picked from commit 30c289b)

Cherry-picked for OLYM-9850 (on macOS VoiceOver read controls that are off
as working controls, with no "dimmed"). Taken without its macOS test
FlutterPlatformNodeDelegateMac.DisabledTextFieldDoesNotUseFlutterTextField:
that test checks upstream flutter#190330 (a disabled text field shown as static
text), which this branch does not carry. The next commit adds a macOS test
for the buttons and checkboxes this fixes.
Mylio Sysops @claude
OLYM-9850: [macOS] Test that a control that is off reads as not enabled
aa73c70 
Adds FlutterPlatformNodeDelegateMac.DisabledControlsAreNotAccessibilityEnabled
for the cherry-pick before it (upstream flutter#190353): a button and a checked
checkbox whose semantics are off (hasEnabledState without isEnabled) report
isAccessibilityEnabled NO on macOS, so VoiceOver says "dimmed"; an enabled
button still reports YES. Before the cherry-pick every Flutter control on the
Mac reported AXEnabled = YES.

UPSTREAM-CANDIDATES.md lists the cherry-pick under "Not candidates", with why
upstream flutter#190330 (its macOS text-field half) is not carried.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Mylio Sysops @claude
OLYM-9850: [macOS] A disabled control is still read by VoiceOver, as …
17042b9 
…dimmed

With the restriction from upstream flutter#190353 in place, a disabled button or
checkbox reached macOS with no role, no name and no AXEnabled: VoiceOver
found an empty element where the control was.

-[AXPlatformNodeCocoa isAccessibilitySelectorAllowed:] returned NO for every
selector on a node whose restriction is kDisabled. AppKit asks it for every
accessibility method this class implements, the getters included
(accessibilityRole, accessibilityLabel, isAccessibilityEnabled), so it hid
the whole element. Upstream master has the same code, so upstream's fix has
the same effect on a real Mac; its tests call the getters directly, which
skips AppKit's check.

Fix: a disabled node refuses only the setters and the accessibilityPerform
actions, like a disabled AppKit control, and is read as before. VoiceOver
then reads "Ignore untagged faces, dimmed, button" and "Info, dimmed,
checked, checkbox".

Test: FlutterPlatformNodeDelegateMac.DisabledControlsAreNotAccessibilityEnabled
now also checks which selectors a disabled button allows. UPSTREAM-CANDIDATES.md
entry 14.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
MylioClaude
MylioClaude commented Nov 11, 2026
View reviewed changes

@MylioClaude MylioClaude left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: approved

Nothing must change before the merge. Three suggestions below; they stay on the PR.

How I reviewed. This checkout's origin is Olym, not this fork, so the job's git fetch origin OlymEDRSupport pull/12/head could not apply. I read gh pr diff 12 and a read-only sparse copy of the PR head (17042b9). The base tip (a0ad2b3) is the PR's parent, so it merges cleanly. I built and ran nothing.

What I checked

  1. The tests can fail. The new macOS test fails on the base: isAccessibilityEnabled is YES there. It also fails on the cherry-pick alone, where every getter is refused. Bridge tests 1 and 3 fail on the base. Test 2 (enabled button, no restriction) is a guard.
  2. Every reader of the restriction.
    • macOS: AXEnabledInternal and the selector check.
    • Windows: UIA IsEnabled, MSAA unavailable, and the refusals in Invoke, ExpandCollapse, SelectionItem and SetValue.
    • The event generator: its ENABLED_CHANGED and READONLY_CHANGED are already no-op cases in both bridges.
    • Linux (fl_accessible_node.cc) and iOS don't use this bridge, and they already read is_enabled.
    • The engine fills is_enabled as a tristate (embedder_semantics_update.cc:37), so a node with no enabled state gets no restriction.
    • The restriction doesn't change IsTextField(), so no node switches between a FlutterTextField and a plain node.
  3. The platform the bug was seen on. The fix is on the Mac path. It was tried there with the AX tree and real VoiceOver.
  4. Cherry-pick fidelity. The accessibility_bridge.cc hunk and its three tests match upstream 30c289b line for line. Only the macOS text-field test was left out, as the PR says. Upstream master's selector check is unchanged (the file last changed 2024-10-31), so entry 14's "unfixed" holds.
  5. Code. The designated initializers follow FlutterSemanticsFlags field order. I saw no new warnings, no dead code and no debug logging. Docs: UPSTREAM-CANDIDATES.md is updated in the same change.

Windows behaviour changes through the cherry-pick and wasn't tried. That is upstream's behaviour on master since August, and the bug was a Mac bug, so this is not a finding.

Findings

  1. Suggestion. engine/src/flutter/third_party/accessibility/ax/platform/ax_platform_node_mac.mm:828

    • What's wrong: the accessibilityPerform prefix also matches accessibilityPerformAction:. This class runs every action through that one method, including AXScrollToVisible. The bridge gives that action to every node (accessibility_bridge.cc:418, "Every node has show on screen action").
    • Why it matters: the new comment says AppKit checks this method for every selector of the class. If that includes accessibilityPerformAction:, VoiceOver can't scroll a disabled control into view when its cursor reaches one off screen, for example lower in the Info panel. Before this PR it could, because the restriction was never set. A disabled AppKit control still scrolls into view. Not proven either way: the controls in the PR's try were on screen.
    • What to do: let accessibilityPerformAction: through the selector check. In that method, when the node is disabled, return early for every action except NSAccessibilityScrollToVisibleAction. Add one line for it to DisabledControlsAreNotAccessibilityEnabled.

  2. Suggestion. UPSTREAM-CANDIDATES.md:255 and :306

    • What's wrong: entry 14 and the "Not candidates" bullet name the PR's own commit subjects. This repo squash-merges (#8, #10 and #11 each landed as one commit, " (#N)"). After the merge there will be one commit, OLYM-9850: [macOS] A control that is off reads as dimmed to VoiceOver (cherry-pick of #190353, plus a selector fix) (#12).
    • Why it matters: the file says it names commits "by subject", so someone looking for these two subjects won't find them. That one commit also holds both parts: the cherry-pick that "falls away" at 3.50 and entry 14, which stays. A rebase onto 3.50 drops the hunks that already match upstream by itself, so this is only about finding the commit.
    • What to do: name the squashed subject. Say which part is which: the accessibility_bridge.cc hunk and its three bridge tests are flutter#190353; the ax_platform_node_mac.mm hunk and the macOS test are entry 14.

  3. Suggestion. Disabled text fields (shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMac.mm:30)

    • What's wrong: a disabled text field is still a FlutterTextField, an NSTextField that never reads the restriction. VoiceOver will still read it as enabled, with no "dimmed". The PR leaves this on purpose, and the reason is sound (flutter#190330 and this branch's OLYM-9024 and OLYM-9141 commits).
    • What to do: file a follow-up ticket for disabled text fields, so this isn't lost.

The core fix is right. A disabled node now answers its getters through super, as before, and refuses only setters and actions. Nothing after that branch in the method needed to run for a disabled node, because it only handles setters.

MylioClaude
MylioClaude commented Nov 11, 2026
View reviewed changes
Comment thread engine/src/flutter/third_party/accessibility/ax/platform/ax_platform_node_mac.mm
// refuses only what would change it or act on it: the setters and the
// accessibilityPerform actions.
NSString* name = NSStringFromSelector(selector);
if ([name hasPrefix:@"set"] || [name hasPrefix:@"accessibilityPerform"])

@MylioClaude MylioClaude Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Suggestion: this prefix also matches accessibilityPerformAction:, the one method this class runs every action through, AXScrollToVisible included (the bridge gives that action to every node). If AppKit checks this selector too, VoiceOver can't scroll a disabled control into view when it is off screen. Before this PR it could. One fix: let accessibilityPerformAction: through here, and in it refuse every action but NSAccessibilityScrollToVisibleAction when the node is disabled. See finding 1 in the review.

@MylioClaude
MylioClaude merged commit 62b672c into OlymEDRSupport Nov 11, 2026
1 check passed
@MylioClaude

MylioClaude commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by the swarm's jira-watch as review round 1 approved it: a review that approves a pull request merges it as it was reviewed, with no answer session (the swarm's check-in rules, rule 2). The review's suggestions stay on this pull request; the swarm took none of them in it.

@MylioClaude
MylioClaude deleted the claude/olym-9850 branch November 11, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MylioClaude @cbracken