fix(a11y): replace fatalError with assertionFailure + safe recovery#617
Merged
Conversation
…ead of crashing Replace fatalError with safe-recovery paths in two AccessibilityDeferral sites and three setter-override sites. The framework now no-ops or falls back to safe behavior when a consumer misconfigures something rather than crashing the host app: - AccessibilityDeferral.swift L242: duplicate sources for one identifier now take the first match (rest stay exposed) instead of fatalError. - AccessibilityDeferral.swift L484: a batch with mismatched updateIdentifiers now falls through to replaceContent instead of fatalError, treating the batch as a fresh pass rather than a merge. - AccessibilityElement.swift L130/L147 and AccessibilityContainer.swift L118: setters on read-only computed properties now assertionFailure + no-op instead of fatalError, matching the existing pattern at ReceiverContainerView.accessibilityPath setter. Also adds a recovery test in AccessibilityDeferralTests covering the mismatched-updateIdentifier path. Note: BlueprintUIAccessibilityCoreTests is not wired into Package.swift (only Bazel currently builds it). The new test runs under Square's internal CI but won't run via \`swift test\` until that's addressed in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mismatched-updateIdentifier guard in Receiver.apply previously replaced existing content with the malformed batch. We cannot assume the malformed batch is fresher than what's already applied, so leave existing content untouched — the next legitimate broker pass will overwrite it. Also drops two comments that referenced pre-change behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The deferral recovery sites cover programming bugs in consumer code, not legitimate runtime states. Add assertionFailure at all three so they surface loudly in DEBUG while release builds still recover gracefully. Drop the malformed-batch recovery test since the new assertion would trip it in DEBUG and the assertion itself is the regression net. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The assertionFailure messages already describe the constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When multiple sources share an identifier, ambiguity is the bug — picking the first match depends on subview ordering and would silently hide one source. Treat as no match instead: no source gets hidden, no content gets consolidated, all duplicates stay visible to assistive tech. Matches the >1-receiver site which also fails safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RoyalPineapple
changed the title
robmaceachern
approved these changes
May 27, 2026
johnnewman-square
approved these changes
May 27, 2026
Contributor
johnnewman-square
left a comment
johnnewman-square
left a comment
There was a problem hiding this comment.
This looks good - each updated site is totally recoverable, like the description mentions 💯
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces
fatalErrorwithassertionFailure+ safe-recovery in five accessibility sites. The framework now surfaces programming bugs loudly in DEBUG while recovering gracefully in release rather than crashing the host app on user devices.Behavior table
AccessibilityDeferral.swiftL229ParentContainercontains >1ReceiversassertionFailurethen clear all receiversAccessibilityDeferral.swiftL242sourceIdentifierassertionFailurethen ignore (no source hidden, all stay visible to a11y)AccessibilityDeferral.swiftL484apply()batch has mismatchedupdateIdentifiersassertionFailurethen ignore batchAccessibilityElement.swiftL130, L147accessibilityFrame/accessibilityPathsettersassertionFailurethen no-opAccessibilityContainer.swiftL118accessibilityElementssetterassertionFailurethen no-opAll five sites cover programming bugs (consumer misconfiguration or framework-internal misuse), not legitimate runtime states. Crashing on a programming bug in a shipping UI library is asymmetric — small upside (loudness in development) vs large downside (production crash on user devices). The
assertionFailurekeeps the development-time signal; the recovery prevents the production crash.The setter pattern matches the existing one at
ReceiverContainerView.accessibilityPathsetter (L325) which already usedassertionFailure+ no-op. The deferral recovery patterns all fail safe — when there's ambiguity (multiple receivers, duplicate source identifiers, mismatched batch) no content gets applied rather than guessing.Why no tests
The L484 recovery path would be the only candidate, but
XCTesttraps onassertionFailurein DEBUG builds — testing the release-mode hedge would require either an injectable assert wrapper (more infrastructure) or release-mode-only tests (CI complexity). TheassertionFailureitself is the regression net: any future change that re-introduces the bug class will trip the assert in dev.Test plan
BlueprintUIAccessibilityCoreandBlueprintUICommonControlsschemes build cleanly on iOS Simulator.AccessibilityDeferralTestsorAccessibilityCompositionTests.Follow-ups (out of scope here)
BlueprintUIAccessibilityCoreTestsis not wired intoPackage.swifttoday (only Bazel builds it). AlsoBlueprintUITestshas an undeclared dependency onBlueprintUICommonControls, which currently breaksswift testfor external SwiftPM consumers. Worth fixing together.REC-002proposes adding dedicated unit tests for the eight untested a11y wrappers.🤖 Generated with Claude Code