feat(feed): support feed role article focus management - #241
AdamMagued wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| const parentFeed = | ||
| tree.role === "feed" && isElement(tree.node) |
There was a problem hiding this comment.
Consider an isFeedRole function to mirror isDialogRole above?
| : tree.parentFeed ?? | ||
| (isElement(tree.node) | ||
| ? ((tree.node as HTMLElement).closest?.('[role~="feed"]') as HTMLElement ?? null) | ||
| : null); |
There was a problem hiding this comment.
Is the fallback needed here?
If as we recurse we are setting tree.parentFeed then is it possible to be nested in a feed without tree.parentFeed already being set?
| const parentArticle = | ||
| parentFeed && tree.role === "article" && isElement(tree.node) | ||
| ? (tree.node as HTMLElement) | ||
| : parentFeed | ||
| ? tree.parentArticle | ||
| : null; |
There was a problem hiding this comment.
The double check for parentFeed feels a bit off?
| const parentArticle = | |
| parentFeed && tree.role === "article" && isElement(tree.node) | |
| ? (tree.node as HTMLElement) | |
| : parentFeed | |
| ? tree.parentArticle | |
| : null; | |
| const parentArticle = | |
| !parentFeed | |
| ? null | |
| : tree.role === "article" && isElement(tree.node) | |
| ? (tree.node as HTMLElement) | |
| : tree.parentArticle; |
Or just nest the whole parentArticle logic inside of the if (parentFeed) { above and then don't need to have the nested ternary?
| } else if (focusableElement) { | ||
| const activeElement = | ||
| article.ownerDocument?.activeElement ?? | ||
| globalThis.document?.activeElement; | ||
|
|
||
| if (activeElement !== focusableElement) { | ||
| this.#focusElement(focusableElement); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
My interpretation of the specification is transitioning from one article to another SHOULD focus on the containing article, but if the first element you land on during that transition happens to be a focusable element, we MAY place focus on that in lieu of setting focus on the containing article.
So unless I'm misinterpreting the spec I believe we can drop this else if logic - the spec isn't saying to force focus to every focusable element inside the article as you visit, it is only with respect to the transition of the reading cursor from one article to another. E.g. if say navigating by role type (links etc.) and therefore, unlike simply using .next() you might skip over visiting the parent article itself.
| } else if (focusableElement) { | |
| const activeElement = | |
| article.ownerDocument?.activeElement ?? | |
| globalThis.document?.activeElement; | |
| if (activeElement !== focusableElement) { | |
| this.#focusElement(focusableElement); | |
| } | |
| } | |
| } | |
| } | |
| } |
|
|
||
| const previousArticle = this.#activeNode?.parentArticle ?? null; | ||
| const isArticleChange = previousArticle !== article; | ||
|
|
There was a problem hiding this comment.
Following previous comment, can probably early return here if it's not an article change and save some DOM traversal cost
| expect(document.activeElement).toBe(art2); | ||
| }); | ||
|
|
||
| it("should focus a focusable element when landing directly on it inside an article", async () => { |
| // Move to button (focusable element) | ||
| await virtual.next(); | ||
| expect(await virtual.lastSpokenPhrase()).toBe("button, Like"); | ||
| expect(document.activeElement).toBe(btn1); |
There was a problem hiding this comment.
| expect(document.activeElement).toBe(btn1); | |
| expect(document.activeElement).toBe(art1); |
| expect(document.activeElement).toBe(ownedArt); | ||
| }); | ||
|
|
||
| it("should not focus disabled elements inside an article", async () => { |
There was a problem hiding this comment.
Great edge-case test, like it - sim. to previous comments if we can adjust to test the "in lieu of..." like behaviour rather than auto-focus then still get good value out of it, i.e. jump to disabled button (or sim) from outside of the feed/article and assert that it's the article that get's focus not the disabled control.
2 participants
Description
Resolves #13.
This PR implements W3C WAI-ARIA feed role focus management according to the ARIA 1.2/1.3 specification:
Verification