[SR][SR Tree] Add screen reader tree to interactive graph editor #2062
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (b3e2e88) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2062If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2062 |
|
Size Change: +979 B (+0.08%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
nishasy
requested review from
benchristel,
catandthemachines,
anakaren-rojas and
a team
| }; | ||
|
|
||
| // Exported for testing | ||
| export function fetchAriaLabels(container?: Element): AttributeMap[] { |
There was a problem hiding this comment.
This function is returning more than ARIA labels, so how about naming it something like getAccessibilityAttributes?
| export function fetchAriaLabels(container?: Element): AttributeMap[] { | |
| export function getAccessibilityAttributes(container?: Element): AttributeMap[] { |
| // Arrange | ||
| const container = render( | ||
| <div> | ||
| <div aria-label="label1" /> | ||
| <div aria-label="label2" /> | ||
| </div>, | ||
| ); | ||
|
|
||
| // Act | ||
| const result = fetchAriaLabels(container.container); |
There was a problem hiding this comment.
container.container is confusing. I think the thing returned by render is just an object containing the actual container? So maybe destructuring would be appropriate here.
| // Arrange | |
| const container = render( | |
| <div> | |
| <div aria-label="label1" /> | |
| <div aria-label="label2" /> | |
| </div>, | |
| ); | |
| // Act | |
| const result = fetchAriaLabels(container.container); | |
| // Arrange | |
| const {container} = render( | |
| <div> | |
| <div aria-label="label1" /> | |
| <div aria-label="label2" /> | |
| </div>, | |
| ); | |
| // Act | |
| const result = fetchAriaLabels(container); |
| if (attribute.name === "aria-describedby") { | ||
| // Aria-description is a space-separated list of ids. | ||
| // Use the ids to get the actual description strings. | ||
| const descriptions = attribute.value.split(" "); |
There was a problem hiding this comment.
What if there are multiple spaces between description IDs? It might be good to use a regex here:
| const descriptions = attribute.value.split(" "); | |
| const descriptions = attribute.value.split(/ +/); |
There was a problem hiding this comment.
I'll update it to regex so it's more deliberate, but I tried testing it with " " just for fun, and it looks like it still passes 🤔
| const descriptions = attribute.value.split(" "); | ||
| for (const description of descriptions) { | ||
| const descriptionString = | ||
| document.getElementById(description)?.textContent; |
There was a problem hiding this comment.
Question: why textContent and not innerText here?
There was a problem hiding this comment.
textContent includes hidden text, whereas innerText does not. All the descriptions that the aria-describedbys are referring to are within hidden text, so we have to use textContent for them to show up.
I'll add a comment here and a test for this.
There was a problem hiding this comment.
Hmm... if I use innerText instead, the test passes even though the descriptions stop showing up in the tree in the editor 🤔
There was a problem hiding this comment.
| innerText | textContent |
|---|---|
 |
 |
| // followed by an unordered list of its aria attributes. | ||
| return ( | ||
| <ol style={{listStyle: "revert", marginLeft: 8}}> | ||
| {elementArias.map((ariaString, index) => ( |
There was a problem hiding this comment.
The elements of elementArias aren't strings, so I'd prefer a different name here:
| {elementArias.map((ariaString, index) => ( | |
| {elementArias.map((aria, index) => ( |
| const elementAttributes: Array<Attribute> = []; | ||
|
|
||
| const attributes = element.attributes; | ||
| for (const attribute of attributes) { |
There was a problem hiding this comment.
Why use a loop to get the attributes here? It seems like it might be simpler to look up the specific attributes we care about, e.g.
const ariaLabel = element.getAttribute("aria-label");Then we'd have more direct control over the order in which the attributes are displayed.
I don't think an element can have multiple attributes with the same name, so this preserves behavior.
There was a problem hiding this comment.
Ah!! I'm so silly.
I tried that, but then my holiday brain thought that aria-describedby can be added multiple times, rather than it being one attribute that's space-separated strings 🤦🏽♀️
| ], | ||
| }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
This test still passes if I change the code to use innerText instead of textContent, which is super weird. That's not how it behaves in practice. Not sure what I'm doing wrong. 🤔
Summary:
To make it easier for content authors (and us devs while we're still
working on the screen reader experience) to build the screen reader
experience for an interactive graph without constantly having to
turn on the screen reader, add a screen reader tree directly into
the editor.
To do this, I used
querySelectorAll()to get the mafs containerand all its children, and read their
aria-labelandaria-describedbyattributes.After building an object with the elements, roles, and attributes,
display a tree showing these in the editor.
Issue: https://khanacademy.atlassian.net/browse/LEMS-2714
Test plan:
yarn jest packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-sr-tree.test.tsxStorybook