-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add first-class DOM support #38
Conversation
🦋 Changeset detectedLatest commit: 9cec048 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
} | ||
|
||
export const widgetRoles: Record<WidgetRole, RoleData> = { | ||
/** An input that allows for user-triggered actions when clicked or pressed. See related link. */ | ||
button: { | ||
type: ['widget'], | ||
allowedChildRoles: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read a blogpost about the peformance gains of monomorphism and since I had to touch a lot of these anyway, decided why not?
Is it faster? I don’t know. We’ll never know. But it was an attempt at performance
d8c5ec3
to
00a1bdf
Compare
cd8a95b
to
97cc489
Compare
if (!element || typeof element !== 'object' || Array.isArray(element) || typeof element.tagName !== 'string') { | ||
throw new Error(`Expected { tagName, [attributes] } object, received ${JSON.stringify(element)}`); | ||
export function getTagName(element: Element | VirtualElement): TagName { | ||
if (typeof Element !== 'undefined' && element instanceof Element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up with this line of code a lot, to split the code path to run in the DOM. Even though this would be a good helper, TypeScript gets lost and you end up having to make the same type assertions manually, more or less. There’s probably a cleaner way to separate DOM code from non- but it didn’t come to me while writing this.
Functionally I found the code paths branch very little in general, which is a good thing! Pretty much only when we want to find ancestors. So moving this “up” seems like the wrong direction since that would cause the code paths to diverge more than necessary. But it does leave us with this snippet sprinkled here-and-there quite a lot
} | ||
|
||
/** Normalize HTML Elements */ | ||
export function virtualizeElement(element: HTMLElement | VirtualElement): VirtualElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now gone—we no longer do unnecessary work on the DOM turning it into a lower-fidelity format.
switch (tagName) { | ||
case 'aside': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to clean this up nicely with the newly-added nameFrom
property in role data, which exists both on aside
and section
, as well as others. Improved behavior and reduced code.
const LANDMARK_ROLES = Object.keys(landmarkRoles) as LandmarkRole[]; | ||
const LANDMARK_ELEMENTS: TagName[] = ['article', 'aside', 'main', 'nav', 'section']; | ||
const LANDMARK_CSS_SELECTOR = LANDMARK_ROLES.map((role) => `[role=${role}]`) | ||
.concat(...LANDMARK_ELEMENTS.map((el) => `${el}:not([role])`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we want to check for main
as a parent, but NOT main[role="presentation"]
, so we eliminate anything with a role
set. Fortunately, we are checking for [role="main"]
from the roles list, too, so it should find all scenarios correctly, in a compact CSS selector.
We only have these mapping functions to make sure in a list of 10+ selectors there’s not a typo
}, | ||
{ | ||
test: { | ||
name: 'happy-dom', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love Vitest 💚. Now we have tests not only for no DOM, but jsdom, happy-dom, AND chromium via Playwright.
We can also run the same tests in all environments, but I did happen to split them up because I found the setup easier to split. Can possibly recombine in the future
README.md
Outdated
@@ -4,22 +4,28 @@ Utilities for creating accessible HTML based on the [latest ARIA 1.3 specs](http | |||
|
|||
This is designed to be a better replacement for aria-query when working with HTML. The reasons are: | |||
|
|||
- aria-query neglects the critical [HTML to ARIA spec](https://www.w3.org/TR/html-aria). With just the ARIA spec alone, it’s insufficient for working with HTML. | |||
- aria-query neglects the critical specs [HTML Accessibility API Mappings](https://www.w3.org/TR/html-aam-1.0/) and [HTML to ARIA](https://www.w3.org/TR/html-aria). With just the ARIA spec alone, it’s insufficient for working with HTML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbf, aria-query has attempted to take these specs into account, e.g. see:
- Audited and updated roles source of truth to HTML Accessibility API Mapping 1.0 A11yance/aria-query#447 originally by the maintainer
- fix: amend breaking changes and reinstate constraints A11yance/aria-query#515 by myself attempting to flesh out
It's more that the API surface of aria-query is not consumer friendly for all use-cases, e.g. difficult to make their "constraints" for context based role decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Hm yeah I’m speaking purely from a pragmatic standpoint. But I guess until we squash all the bugs here, one could say the same about this library too. I only wanted to provide a “why would I use this” statement upfront.
Would you phrase it more like your last sentence?
It's more that the API surface of aria-query is not consumer friendly for all use-cases, e.g. difficult to make their "constraints" for context based role decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I suspect you could still rightly angle for saying this library provides ergonomic APIs supporting this specs which are impractical with aria-query. At this point you might even been supporting more things, would need to compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the README with these thoughts. Thanks!
I do struggle with finding the right balance between brevity and “getting to the point” but without losing nuance. Thanks for feedback.
Co-authored-by: Craig Morten <[email protected]>
I’m going to merge this if there are no objections, but will do another pass on AAM before releasing |
Sure thing - apologies not had the bandwidth to do more than a superficial skim. Will find some time to digest what you've done properly, but I'm sure any futher tweaks will come out in the wash soon enough. |
Oh yeah for sure—after working through it there were really far fewer breaking changes than I anticipated. With libraries like these, I usually regard the tests as the most-important thing, so my philosophy is that the tests capture all intended behavior (which usually means they end up larger than the codebase itself, but that’s OK when you’re dealing with code that claims to adhere to spec compliance). So really when we gain confidence in the tests, that’s the most important thing. And of course, secondarily benchmarks like we talked about |
Changes
getRole
doesn't handle anHTMLElement
argument whenHTMLElement
is not a global #23: DOM methods supported and tests are added<h1>
=aria-level="1"
.Not only interested in testing it’s working; also interested in benchmarking it across environments to see how it stacks up.
How to Review
Try the test package at:
node
anddom
? Could the duplication among the tests be improved?Checklist