Fix defect in calling code when building the ElementRoleMap #555
Conversation
This file contains 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
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
@jlp-craigmorten I think you might be in the best spot to provide some guidance on what to do with the fixture failures - as I am not sure how to audit what is correct or not. |
|
Hey @samccone 👋 Sure will pull your branch and take a look to see if can make sense of it. Be interesting to understand what the impact of this is, the pertinent PR #447 was merged back in 2022 with no issues raised for it since. Makes you wonder if could consider it YAGNI and just remove 🤔 but depends what value this could potentially bring, or if consumers of this package have been unknowingly incorrect for 2 years and would benefit from this being reinstated. |
|
I agree @jlp-craigmorten - to test this I went back and fixed the original code change at f7f6120#r143856081 with diff --git a/src/elementRoleMap.js b/src/elementRoleMap.js
index 35a2233..bbff1b6 100644
--- a/src/elementRoleMap.js
+++ b/src/elementRoleMap.js
@@ -24,7 +24,7 @@ for (let i = 0; i < keys.length; i++) {
if (relation.module === 'HTML') {
const concept = relation.concept;
if (concept) {
- const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => deepEqual(relation, concept));
+ const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => deepEqual(relation[0], concept));
let roles: RoleSet;
if (elementRoleRelation) {Running the tests with this patch at that state of the tree resulted in failures. 😢 ``` Test Suites: 1 failed, 6 passed, 7 total Tests: 48 failed, 5086 passed, 5134 total Snapshots: 0 total Time: 3.446 s ```I am thinking this may have been broken since this original change - and or the fixture test files were not snapped to the correct state. Wondering if you have any ideas. |
|
@jlp-craigmorten i think that for a11y in particular, we can't assume that something's "good enough" just because nobody's complained; devs rely on a11y tools to do the right thing even for smaller groups of people, so if we need to make a change to make things more correct, I think that'd be a good thing to do. |
|
Ok think have this grok’d. Lemme try and poorly talk through: It is feasible that an element can be assigned more than one role (under relevant spec WAI-ARIA and HTML-AAM). It is rare and specs generally change to remove ambiguity by providing additional context (or REFs:
The section of code in scope for this change is responsible for backsearching through previously generated element to role entries to see if we’re in such a case and make sure roles are correctly applied. As stands in the code today this impacts a single element, namely Coincidentally the concepts for the Where we should see this code have an impact is for the likes of alias roles of Interestingly, even these could be debated as Curiously, this appears to result in duplicate entries in the map where the only difference is the ordering of the roles in the value - open question whether this should be de-duped as not sure what value that has? I think this is a bug and suspect we could add logic such that if we’re in a duplicate case we simply update the existing entry and avoid the final Adding this de-dupe logic could be seen as a breaking change, except:
So no-one should be relying on there being multiple entries afaik. — As an aside I seem to get different test failures when trying the old usage of |
cmorten
reviewed
Jul 6, 2024
src/elementRoleMap.js
Outdated
| if (isUnique) { | ||
| roles.push(key); | ||
| } | ||
| elementRoles.push([concept, roles]); |
There was a problem hiding this comment.
Per my other comment, I think we could consider something like:
Suggested change
| elementRoles.push([concept, roles]); | |
| if (!elementRoleRelation) { | |
| elementRoles.push([concept, roles]); | |
| } |
This would mean the map only ever has a single entry per unique concept (aka element + constraints) rather than multiple (in the case of elements that exhibit an alias role)
cmorten
reviewed
Jul 6, 2024
src/elementRoleMap.js
Outdated
| @@ -23,7 +23,25 @@ for (let i = 0; i < keys.length; i++) { | |||
| if (relation.module === 'HTML') { | |||
| const concept = relation.concept; | |||
| if (concept) { | |||
| elementRoles.push([concept, [key]]); | |||
| const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => ariaRoleRelationConceptAttributeEquals(relation[0], concept)); | |||
There was a problem hiding this comment.
Suggested change
| const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => ariaRoleRelationConceptAttributeEquals(relation[0], concept)); | |
| const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => ariaRoleRelationConceptEquals(relation[0], concept)); |
Care needed here - I think you’ve been working off the wrong type(s).
The relation[0] and concept are of type ARIARoleRelationConcept not ARIARoleRelationConceptAttribute which I think explains why your test case failure count is so high.
The comparison function needs to be amended to take the full concept into account.
Once amended the only failures should relate to the aside element (pending a fix for separate issues)
As discovered in A11yance#554 Address the defect introduced f7f6120#r143856081 This change depends on A11yance#558 which when applied (as in this PR makes the teset cases pass)
Pull Request Test Coverage Report for Build 9833491688Details
💛 - Coveralls |
cmorten
reviewed
Jul 8, 2024
ljharb
approved these changes
Jul 8, 2024
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.
As discovered in #554
Address the defect introduced f7f6120#r143856081
This now exercises all of the code:
Yet now multiple tests are failing in a fixture file used for testing that has been updated since this defect was introduced. I am not sure what the correct state of these fixtures should be