refactor: use virtual node attr/hasAttr in rules and checks#5198
Open
chutchins25 wants to merge 2 commits into
Open
refactor: use virtual node attr/hasAttr in rules and checks#5198chutchins25 wants to merge 2 commits into
chutchins25 wants to merge 2 commits into
Conversation
Convert getAttribute/hasAttribute reads to the virtual node .attr()/.hasAttr() equivalents in the rules/*-matches cluster (xml-lang-mismatch, duplicate-id-active, duplicate-id-misc, frame-title-has-text, landmark-has-body-context). These are behavior-preserving drop-ins — .attr() returns the raw value or null with no trimming, matching getAttribute. Part of #5043
…ic checks Convert getAttribute/hasAttribute reads to the virtual node .attr()/.hasAttr() equivalents in help-same-as-label, hidden-explicit-label, multiple-label, parsing/duplicate-id, tables/scope-value, and generic/page-no-duplicate. Behavior-preserving drop-ins — .attr() returns the raw value or null, matching getAttribute. Update the affected check tests to pass a virtual node argument. Part of #5043
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors a targeted set of rule matchers and check evaluators to use VirtualNode.attr() / VirtualNode.hasAttr() in places where those calls are intended to be behavior-preserving drop-in replacements for getAttribute() / hasAttribute(), and updates impacted unit tests to pass the required virtualNode argument.
Changes:
- Updated selected
lib/rules/*-matches.jsmatchers to read attributes viavirtualNode.attr()/virtualNode.hasAttr()and adjusted matcher signatures where needed. - Updated selected
lib/checks/**/*-evaluate.jschecks to read attributes viavirtualNode.attr()and adjusted evaluate signatures where needed. - Updated affected rule-matcher and check unit tests to pass a
VirtualNodeargument (oraxe.utils.getNodeFromTree(...)where appropriate).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/rule-matches/html-xml-lang-mismatch.js | Passes a VirtualNode into the matcher to support .attr() reads. |
| test/rule-matches/frame-title-has-text-matches.js | Passes a VirtualNode into the matcher to support .attr('title'). |
| test/checks/tables/scope-value.js | Updates check invocation to provide virtualNode for .attr('scope'). |
| test/checks/parser/duplicate-id.js | Updates check invocation to provide virtualNode for .attr('id'). |
| test/checks/label/multiple-label.js | Updates check invocation to provide a tree VirtualNode for .attr('id'). |
| lib/rules/xml-lang-mismatch-matches.js | Replaces getAttribute reads with virtualNode.attr(...). |
| lib/rules/landmark-has-body-context-matches.js | Replaces hasAttribute('role') with virtualNode.hasAttr('role'). |
| lib/rules/frame-title-has-text-matches.js | Replaces getAttribute('title') with virtualNode.attr('title'). |
| lib/rules/duplicate-id-misc-matches.js | Replaces getAttribute('id') with virtualNode.attr('id'). |
| lib/rules/duplicate-id-active-matches.js | Replaces getAttribute('id') with virtualNode.attr('id'). |
| lib/checks/tables/scope-value-evaluate.js | Replaces getAttribute('scope') with virtualNode.attr('scope'). |
| lib/checks/parsing/duplicate-id-evaluate.js | Replaces getAttribute('id') with virtualNode.attr('id') and updates signature. |
| lib/checks/label/multiple-label-evaluate.js | Replaces getAttribute('id') with virtualNode.attr('id') and updates signature. |
| lib/checks/label/hidden-explicit-label-evaluate.js | Replaces getAttribute('id') with virtualNode.attr('id'). |
| lib/checks/label/help-same-as-label-evaluate.js | Replaces getAttribute(...) reads with virtualNode.attr(...). |
| lib/checks/generic/page-no-duplicate-evaluate.js | Replaces actualNode.hasAttribute('role') with hasAttr('role') on vNodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Converts
getAttribute/hasAttributecalls to the VirtualNode.attr()/.hasAttr()equivalents where it's a safe, behavior-preserving drop-in..attr()/.hasAttr()return the raw attribute value (ornull) exactly likegetAttribute/hasAttribute— no trimming or normalization — so these swaps preserve behavior.Scope
A full sweep of
lib/found 71 rawgetAttribute/hasAttributecalls (74 grep hits − 3 JSDoc comments). Of these, 13 are converted here; the remaining 58 are intentionally left as-is (52 must-stay + 6 judgment). #5043 is satisfied by converting where it makes sense, not by driving the count to zero.✅ Converted (13)
lib/rules/*-matches(6):xml-lang-mismatch-matchesgetAttribute('lang'),getAttribute('xml:lang')→.attr()duplicate-id-active-matchesgetAttribute('id').trim()→.attr('id').trim()duplicate-id-misc-matchesgetAttribute('id').trim()→.attr('id').trim()frame-title-has-text-matchesgetAttribute('title')→.attr('title')landmark-has-body-context-matcheshasAttribute('role')→.hasAttr('role')lib/checks/(7):label/help-same-as-labelgetAttribute('title'),getAttribute('aria-describedby')→.attr()label/hidden-explicit-labelgetAttribute('id')→.attr('id')label/multiple-labelgetAttribute('id')→.attr('id')parsing/duplicate-idgetAttribute('id').trim()→.attr('id').trim()tables/scope-valuegetAttribute('scope').toLowerCase()→.attr('scope').toLowerCase()generic/page-no-duplicateelm.actualNode.hasAttribute('role')→elm.hasAttr('role')matchesfunctions receivevirtualNodeas the 2nd argument and checks as the 3rd, so the virtual node is always available (a few functions had the parameter added). Affectedmatches/check tests were updated to pass a virtual node argument.⏸️ Left as-is — must-stay (52)
These operate on real DOM by design; converting them would be incorrect or pointless.
core/utils/is-hidden,preload-media,get-xpath,get-selector,preload-cssomcore/utilsworks on real DOM by designcore/base/context/create-frame-contextcore/base/virtual-node.attr()/.hasAttr()implementation + constructorcommons/table(to-grid,is-header,is-data-table,get-headers).rows/.cells;is-data-tabletakes a real DOM tablecommons/aria/get-accessible-refschildNodesrecursioncommons/dom(idrefs,is-current-page-link,url-props-from-attribute,is-visible)idrefsleft per #5138; live URL props;is-visible:147is an intentional vNode-null fallbackchecks/tables/th-has-data-cells,td-headers-attrchecks/aria/aria-required-parentparentElementwalk on the raw elementrules/aria-hidden-focus-matchesrules/color-contrast-matchescontrolcomes fromgetAccessibleRefs(raw refs); color-contrast is #5150 scope⏸️ Left as-is — judgment (6)
These mix attribute reads with live DOM/property reads, so converting only the attribute reads would be inconsistent.
commons/aria/lookup-tableevaluateRoleForElementhandlers read live props (node.type, etc.) alongside the attributechecks/media/no-autoplay-audio-evaluate,rules/no-autoplay-audio-matchesnode.duration,node.currentSrc)Testing
matches/check tests updated to pass a virtual node argument (some previously passed only the DOM node and would throw on the new.attr()reads).npm run eslint,npm run test:tsc,npm run build, and the affected rule-matches + check suites all pass.Closes #5043