feat(aria-allowed/prohibited-attr, aria-required-parent/children): partially support element internals role#5080
feat(aria-allowed/prohibited-attr, aria-required-parent/children): partially support element internals role#5080
Conversation
…pport element internals implicit role
| fixture.setAttribute('id', 'fixture'); | ||
| document.body.insertBefore(fixture, document.body.firstChild); | ||
| } | ||
| testUtils.fixture = fixture; |
There was a problem hiding this comment.
Adding fixture to the testutils objects makes getting it a bit easier instead of having to do const fixture = document.querySelector('#fixture') in every test that needs it.
| afterEach(function () { | ||
| fixture.innerHTML = ''; | ||
| }); | ||
| describe('aria.implicitRole', () => { |
There was a problem hiding this comment.
Most of this file is cleaning it up to match the new es6 style. Bottom of the file has the newest tests.
| @@ -1,170 +1,115 @@ | |||
| describe('aria-required-parent', function () { | |||
| 'use strict'; | |||
| describe('aria-required-parent', () => { | |||
There was a problem hiding this comment.
Most of this file is cleaning it up to match the new es6 style. Bottom of the file has the newest tests.
| } else if (role || hasGlobalAriaOrFocusable) { | ||
| const attr = globalAriaAttr || 'tabindex'; | ||
| const attr = | ||
| globalAriaAttr || (vNode.hasAttr('tabindex') ? 'tabindex' : undefined); |
There was a problem hiding this comment.
This code assumed that if an element had a role it also had a global aria attr available, which wasn't true. This lead to a custom element with an internal role to always have [tabindex] added to the selector even though that attr didn't exist.
straker
changed the title
| describe('ElementInternals', () => { | ||
| it('should detect incorrectly used attributes', () => { | ||
| const vNode = queryFixture( | ||
| '<testutils-element with-role="link" id="target" tabindex="1" aria-selected="true"></testutils-element>' |
There was a problem hiding this comment.
See the changes to the test/testutils.js file for how this test custom element works.
chutchins25
left a comment
chutchins25
left a comment
There was a problem hiding this comment.
Nice work! A few questions for you:
-
Are we ok bundling large refactors like this in with the feature work? Would it be worth splitting the ES6 cleanup into its own PR next time to keep the feature diff focused? CLAUDE.md §3 leans that way, but curious if there's a reason the bundle made sense here.
-
On naming — since
internalslands onVirtualNodeas public-ish surface (consumers can reach it through any VirtualNode instance), renaming it later would be a breaking change. WaselementInternalsconsidered as a more explicit name, or is the short form intentional?
Co-authored-by: Chris Hutchins <chris.hutchins@deque.com>
Yep. Any time we edit files that use the old syntax we convert the file first to the new syntax and then add the changes. It allows us to slowly convert the files over time without having to take the time to do it on it's own.
I can rename it |
2 participants
Note that this can only support rules that don't select for
[role]on the element itself, which is why we can't supportaria-required-attryet, or any rules that look for specific roles (e.g.aria-command-name). Additionally we can only test the supported rules partially as those rules look for an element with[role]. For example, foraria-required-parentwe can test that the parent of an element with[role]has a required internal role, but we cannot test a child with an internal role has a required parent.Fully supporting rules can only be supported once we've figured out how to update our selectors to look at more than just CSS selectors (planned but not fully spec'd out yet).
Closes: #5039
Closes: #4259