-
Notifications
You must be signed in to change notification settings - Fork 118
Fix TextField accessibility - contentDescription was ignored #2680
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.
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
base: jb-main
Are you sure you want to change the base?
Fix TextField accessibility - contentDescription was ignored #2680
Conversation
Before this fix, TextField ignored contentDescription in .semantics {},
making it impossible to add proper labels for accessibility tools.
Text content was used as both label and value.
Now uses contentDescription as accessible name when available,
fixing VoiceOver usability on macOS.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| if (setText != null) { | ||
| return semanticsConfig | ||
| .getOrNull(SemanticsProperties.ContentDescription) | ||
| ?.mergeText() | ||
| ?: text?.toString() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we should not just always return
(semanticsConfig.getOrNull(SemanticsProperties.Text) ?: semanticsConfig.getOrNull(SemanticsProperties.ContentDescription))
?.mergeText()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e., why only if setText != null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setText != null check is necessary because Text ?: ContentDescription doesn't work for the fallback case.
I tested your suggestion and it fails the textFieldAccessibleNameFallsBackToTextContent test. The issue is that for TextFields, SemanticsProperties.Text may exist in the config but doesn't provide the actual text content—that comes from the text field (via AccessibleText).
So when there's no contentDescription, we need text?.toString() as fallback, not SemanticsProperties.Text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, sorry, I meant the other way around (ContentDescription ?: Text).
My point was that it seems to me the content description should be used (if set) not just for text fields (or editable text components in general).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point—using ContentDescription ?: Text for all components would be more consistent.
However, this fails the textFieldAccessibleNameFallsBackToTextContent test because SemanticsProperties.Text is null/empty for editable TextFields.
The fallback needs text?.toString() (from the Java AccessibleText API), not SemanticsProperties.Text (from Compose semantics). The setText != null check ensures we use the correct fallback source specifically for text fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question wasn't about semanticsConfig.getOrNull(SemanticsProperties.Text) vs. text (which is just a calculated property of ComposeAccessibleComponent, by the way; it doesn't come from the Java API), although that's a valid question too. It was about using ContentDescription even if setText is null. Is there a reason not to do that?
About the question of semanticsConfig.getOrNull(SemanticsProperties.Text) vs. text (which itself is EditableText ?: Text): I don't think it's correct to use EditableText in getAccessibleName (or getAccessibleDescription.
So my suggestion is:
- In
getAccessibleNamereturnContentDescription ?: Text. - In
getAccessibleDescriptionreturnContentDescription.
Do you know of any cases where this would result in undesirable behavior from the OS accessibility system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, getAccessibleDescription already returns ContentDescription.
So just the first suggestion then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I hadn’t understood it properly.
On my side, I also didn’t fully understand why EditableText was being used in getAccessibleName, but I intentionally kept it to avoid introducing overly large changes. My goal was really to fix this specific bug, without further altering the existing behavior.
To my knowledge, this should not cause any issues with screen readers: they should be able to read the content correctly through the AccessibleText interface.
Would you like me to apply the proposed changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to apply the proposed changes?
Yes, let's do that, and if/when anyone complains we'll fix it (and document exactly why).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented your suggestion.
However, when running all AccessibilityTest tests together, 10 tests fail with assert(activeControllers.isEmpty()) in the cleanup. These same tests pass when run individually.
Streamline accessible name retrieval by prioritizing `contentDescription` and fallback to semantics text. Update test to reflect behavior when `contentDescription` is absent.
|
Please format the PR summary according to https://raw.githubusercontent.com/JetBrains/compose-multiplatform-core/refs/heads/jb-main/.github/PULL_REQUEST_TEMPLATE.md Also, if you haven't already, sign the Google Contributor's License Agreement at https://cla.developers.google.com to let us upstream your code to Google's AOSP repository |
|
@MSasha I've reformatted the PR according to the template. The description, testing details, and release notes are now properly structured. Regarding the Google CLA, I've signed it at https://cla.developers.google.com ✓ |
|
@kdroidFilter Please rebase on latest |
Description
Fixes TextField accessibility issue on Desktop (macOS) where
contentDescriptionprovided viaModifier.semantics {}was completely ignored by VoiceOver screen readers. Before this fix, TextField components were unusable for screen reader users because the text content was used as both the accessible name (label) and value (content), making unlabeled fields indistinguishable.The fix updates
ComposeAccessible.getAccessibleName()to prioritizecontentDescriptionas the accessible name for text fields, aligning the behavior with iOS accessibility standards.Before
VoiceOver: "[email protected]" (no label)
After
VoiceOver: "Email", "[email protected]" (label + value)
Visual comparison:
Before fix:


After fix:


Testing
Unit Tests:
AccessibilityTest.ktcovering:contentDescription→ uses contentDescription as accessible namecontentDescription→ falls back to text contentManual Testing:
contentDescriptionRelease Notes
Fixes - Desktop
contentDescriptionwas ignored by screen readers (VoiceOver). TextField now properly usescontentDescriptionas the accessible name/label, making forms usable with assistive technologies.Note: I have signed the Google Contributor's License Agreement at https://cla.developers.google.com