Fix: allow anchors with role none, presentation or slider - #1870
Open
mjoslyn wants to merge 1 commit into
Open
Conversation
An <a> with role="none" or role="presentation" is removed from the accessibility tree, and an <a> with role="slider" is exposed as a slider. Neither is a link, so neither should be reported as improper link usage. role="slider" joins the existing button and tab allowlist, which stays unconditional: none of those roles is exposed as a link, so the anchor is not being used improperly however it is focused. The presentational roles get a narrower exemption, per the presentational roles conflict resolution: the role is ignored, and the element still exposed, when the anchor is focusable or carries a global ARIA state or property. So <a href="#" role="none"> continues to fail. The global ARIA half is approximated as any aria-* attribute, which errs toward reporting. aria-hidden is excluded because it removes the element from the tree outright, making the role moot, matching how linked-image-alt-present handles it. Update how_to_fix and the rule help text to name the presentational roles the scanner now accepts, following ac471ce. Fixes equalizedigital#1748
Contributor
📝 WalkthroughWalkthroughThe link validation check now accepts ChangesLink role validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mjoslyn
marked this pull request as ready for review
August 6, 2026 12:17
Author
|
@pattonwebz - this is ready for your review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@includes/classes/Rules/Rule/LinkImproperRule.php`:
- Around line 44-49: Update the help text in LinkImproperRule to state that the
presentational-anchor exception requires the anchor to be non-focusable and have
no applicable global ARIA state or property. Also update the corresponding
guidance in src/pageScanner/rules/link-improper.js to include that global-ARIA
condition and both role="none" and role="presentation".
In `@src/pageScanner/checks/link-has-valid-href-or-role.js`:
- Around line 21-22: The role exemption in link-has-valid-href-or-role.js must
honor ARIA token order: resolve the first recognized non-abstract role, then
allow the exemption only when that role is button, tab, or slider rather than
matching any later token. Add the requested regression case in
tests/jest/rules/linkImproper.test.js at lines 218-220 for role="none slider"
with href="#", which should produce the improper-link result.
- Around line 40-44: The global-ARIA detection in link-has-valid-href-or-role.js
must exclude aria-hidden only when its value is "true", not for every
aria-hidden attribute; update the attribute check accordingly. In
tests/jest/rules/linkImproper.test.js lines 245-247, add coverage showing an
anchor with aria-hidden="false" fails validation.
In `@src/pageScanner/rules/link-improper.js`:
- Line 16: Wrap the user-facing help string in the link-improper rule with
wp.i18n.__, using the existing accessibility-checker text domain and preserving
the current message content.
In `@tests/jest/rules/linkImproper.test.js`:
- Around line 245-247: Add a separate fixture in the link rule test cases near
“Passes with role="none" and aria-hidden="true"” for an anchor with role="none"
and aria-hidden="false", and set shouldPass to false to verify explicit false is
rejected while the true hidden case remains passing.
- Around line 218-220: Extend the role-resolution tests near the existing
“multiple roles including none and no href” case with a failing case for an
anchor using href="#" and role="none slider". Ensure the expected result
indicates an improper link, verifying that the scanner selects the first
recognized role, none, and does not let the later slider token suppress the
result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7bba759-cacc-4d82-a87a-62321f44a0be
📒 Files selected for processing (4)
includes/classes/Rules/Rule/LinkImproperRule.phpsrc/pageScanner/checks/link-has-valid-href-or-role.jssrc/pageScanner/rules/link-improper.jstests/jest/rules/linkImproper.test.js
Comment on lines
+44
to
+49
| // translators: %1$s is <code><button></code>, %2$s is <code>role="button"</code>, %3$s is <code>role="none"</code>, %4$s is <code>role="presentation"</code>. | ||
| esc_html__( 'If the element is used to trigger an action, replace the anchor tag with a %1$s. If you cannot replace it, ensure that %2$s is added to the link, along with JavaScript that adds support for triggering it with the space bar key, and that appropriate ARIA attributes are used for toggle states or other functionality. If the anchor is purely decorative and cannot be focused, %3$s or %4$s marks it as presentational so it is not reported.', 'accessibility-checker' ), | ||
| '<code><button></code>', | ||
| '<code>role="button"</code>' | ||
| '<code>role="button"</code>', | ||
| '<code>role="none"</code>', | ||
| '<code>role="presentation"</code>' |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align help text with the evaluator predicate.
Both guidance strings omit part of the presentational-anchor exception. The anchor must be non-focusable and have no applicable global ARIA state or property. The JavaScript help text also omits role="presentation".
includes/classes/Rules/Rule/LinkImproperRule.php#L44-L49: state the global-ARIA condition.src/pageScanner/rules/link-improper.js#L16-L16: state the global-ARIA condition and include both presentational roles.
📍 Affects 2 files
includes/classes/Rules/Rule/LinkImproperRule.php#L44-L49(this comment)src/pageScanner/rules/link-improper.js#L16-L16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@includes/classes/Rules/Rule/LinkImproperRule.php` around lines 44 - 49,
Update the help text in LinkImproperRule to state that the presentational-anchor
exception requires the anchor to be non-focusable and have no applicable global
ARIA state or property. Also update the corresponding guidance in
src/pageScanner/rules/link-improper.js to include that global-ARIA condition and
both role="none" and role="presentation".
Comment on lines
+21
to
+22
| // Allow roles of button, tab or slider | ||
| if ( roles.some( ( r ) => [ 'button', 'tab', 'slider' ].includes( r ) ) ) { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Honor ordered ARIA role tokens.
The evaluator accepts any allowlisted role token. ARIA applies the first recognized non-abstract role token. This permits a later slider, tab, or button token to suppress a required improper-link result.
src/pageScanner/checks/link-has-valid-href-or-role.js#L21-L22: resolve the first recognized ARIA role before applying a role exemption.tests/jest/rules/linkImproper.test.js#L218-L220: add a failingrole="none slider"case withhref="#".
📍 Affects 2 files
src/pageScanner/checks/link-has-valid-href-or-role.js#L21-L22(this comment)tests/jest/rules/linkImproper.test.js#L218-L220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pageScanner/checks/link-has-valid-href-or-role.js` around lines 21 - 22,
The role exemption in link-has-valid-href-or-role.js must honor ARIA token
order: resolve the first recognized non-abstract role, then allow the exemption
only when that role is button, tab, or slider rather than matching any later
token. Add the requested regression case in
tests/jest/rules/linkImproper.test.js at lines 218-220 for role="none slider"
with href="#", which should produce the improper-link result.
Comment on lines
+40
to
+44
| // Approximated as any aria-* attribute, which errs toward reporting. aria-hidden is | ||
| // excluded because it removes the element from the tree outright, making the role moot. | ||
| const hasGlobalAria = Array.from( node.attributes ).some( | ||
| ( attr ) => attr.name.startsWith( 'aria-' ) && attr.name !== 'aria-hidden' | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat every aria-hidden value as hidden.
The presentational-role exemption excludes aria-hidden without checking its value. This incorrectly exempts anchors with aria-hidden="false".
src/pageScanner/checks/link-has-valid-href-or-role.js#L40-L44: exempt onlyaria-hidden="true"from global-ARIA detection.tests/jest/rules/linkImproper.test.js#L245-L247: add anaria-hidden="false"case that fails validation.
📍 Affects 2 files
src/pageScanner/checks/link-has-valid-href-or-role.js#L40-L44(this comment)tests/jest/rules/linkImproper.test.js#L245-L247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pageScanner/checks/link-has-valid-href-or-role.js` around lines 40 - 44,
The global-ARIA detection in link-has-valid-href-or-role.js must exclude
aria-hidden only when its value is "true", not for every aria-hidden attribute;
update the attribute check accordingly. In tests/jest/rules/linkImproper.test.js
lines 245-247, add coverage showing an anchor with aria-hidden="false" fails
validation.
Comment on lines
+218
to
+220
| name: 'Passes with multiple roles including none and no href', | ||
| html: '<a role="foo none bar"><span>Menu</span></a>', | ||
| shouldPass: true, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test ordered-role resolution.
Keep this fallback-role test. Add a failing case for <a href="#" role="none slider">. The scanner must use none because it is the first recognized role. The later slider token must not suppress the improper-link result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/jest/rules/linkImproper.test.js` around lines 218 - 220, Extend the
role-resolution tests near the existing “multiple roles including none and no
href” case with a failing case for an anchor using href="#" and role="none
slider". Ensure the expected result indicates an improper link, verifying that
the scanner selects the first recognized role, none, and does not let the later
slider token suppress the result.
Comment on lines
+245
to
+247
| name: 'Passes with role="none" and aria-hidden="true"', | ||
| html: '<a role="none" aria-hidden="true"><span>Menu</span></a>', | ||
| shouldPass: true, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test aria-hidden="false" separately.
Add <a role="none" aria-hidden="false"> with shouldPass: false. This protects the distinction between a true hidden state and an explicit false value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/jest/rules/linkImproper.test.js` around lines 245 - 247, Add a separate
fixture in the link rule test cases near “Passes with role="none" and
aria-hidden="true"” for an anchor with role="none" and aria-hidden="false", and
set shouldPass to false to verify explicit false is rejected while the true
hidden case remains passing.
pattonwebz
self-requested a review
August 6, 2026 14:44
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
1 participant
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.
Fixes #1748.
link_improperwas flagging two anchors that are not exposed as links:<a role="none">/<a role="presentation">— removed from the accessibility tree entirely.<a role="slider">— exposed as a slider. Not ideal markup, as the issue notes, but common in older media players and technically correct.sliderjoins the existingbutton/taballowlist, which stays unconditional: none of those roles is exposed as a link, so the anchor is not being used improperly however it happens to be focused.The presentational roles get a narrower exemption, following the presentational roles conflict resolution. The role is ignored — and the element therefore still exposed as a link — when the anchor is either:
<a href="#" role="none">continues to fail.aria-*attribute, which errs toward reporting.aria-hiddenis excluded, since it removes the element from the tree outright and makes the role moot — matching howlinked-image-alt-presentalready handles it.Compatibility
Both changes are relaxations. Nothing that passed on
developfails after this change, so it should not surface new issues on existing scans — only remove false positives.Testing
14 cases added to
tests/jest/rules/linkImproper.test.js, including both markup samples from the issue verbatim, the focusability and global-ARIA boundaries in both directions, and a roving-tabindexrole="tab"case pinning the allowlist as unconditional. Full file passes at 46/46;phpcsandwp-scripts lint-jsare clean on the changed files.Checklist
Summary by CodeRabbit
Bug Fixes
role="none"orrole="presentation".Documentation