Origami/ogm 4698 fix accessibility issue for link button - #2846
Ivaylo Dimitrov (EGFireball) wants to merge 4 commits into
Conversation
Ivaylo Dimitrov (EGFireball)
added
patch
Generated by 🚫 Danger Kotlin against ae53bf1 |
Generated by 🚫 Danger Kotlin against 780458d |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Updates the Compose link implementation to improve accessibility by using URL-based link annotations rather than custom clickable tags.
Changes:
- Switches from
LinkAnnotation.Clickable(tag=...)toLinkAnnotation.Url(url=...)for link spans. - Removes per-link index/tag generation in favor of URL annotations.
- Tightens link creation condition to require both link text and URL.
| File | Description |
|---|---|
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/link/internal/BpkLinkImpl.kt | Reworks how markdown links are converted into annotated text to use URL link annotations (accessibility-related). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
780458d to
5efb21f
Compare
Generated by 🚫 Danger Kotlin against 5efb21f |
Generated by 🚫 Danger Kotlin against a6880f0 |
| } | ||
| } | ||
| } else { | ||
| AppendRawText(textColor, match.value) |
There was a problem hiding this comment.
This seems to be just adding the () as seen in the screenshot tests.
|
|
||
| if (linkText.isNotEmpty() || url.isNotEmpty()) { | ||
| val linkAnnotation = LinkAnnotation.Clickable( | ||
| tag = "LINK_$linkIndex", |
There was a problem hiding this comment.
Be good to keep the tag although not sure if its used anywhere.
There was a problem hiding this comment.
The tag parameter is specific to LinkAnnotation.Clickable. LinkAnnotation.Url doesn't support it, so it can't be carried over.
| val linkAnnotation = LinkAnnotation.Clickable( | ||
| tag = "LINK_$linkIndex", | ||
| if (linkText.isNotEmpty() && url.isNotEmpty()) { | ||
| val linkAnnotation = LinkAnnotation.Url( |
There was a problem hiding this comment.
From what I can see in the docs this shouldn't make any difference from clickable? I think it just attempts to open the URI directly first rather than call the linkInteraction listener.
There was a problem hiding this comment.
The reason for switching is purely for accessibility purposes. The LinkAnnotation.Url exposes Role, while Clickable exposes no role at all. That's the WCAG 4.1.2 violation we're fixing - the Cookie Policy link on the consent screen was being focused by TalkBack with no announced role.
Fixes OGM-4698
What
LinkAnnotation.ClickablewithLinkAnnotation.UrlinBpkLinkImpl.Clickableexposes no role tothe accessibility tree;
Urlautomatically exposesRole.Link, soTalkBack now announces inline links as "Link" rather than as a generic
operable element with no role.
Why
WCAG 4.1.2 (Name, Role, Value) requires that the role of every UI
component is programmatically determinable. An accessibility audit on the
"We Value Your Privacy" consent screen identified that the Cookie Policy
link had no announced role when using TalkBack.
Testing
Backpack demo app.
recordScreenshots.