-
Notifications
You must be signed in to change notification settings - Fork 779
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
feat(get-role): add presentation role resolution and inheritance #2281
Conversation
properties: { type: 'button' } | ||
} | ||
]; | ||
const alwaysTitleElements = ['iframe']; |
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 shouldn't be necessary. IsFocusable is getting this wrong. For the purpose of what we're doing here, iframes are focusable areas. Unlike other areas they route focus to the first focusable area inside of it, unless there is none in which case you can clearly see iframes are focusable. Take this example:
<button>hello</button>
<iframe width="100" height="100"></iframe>
<button>world</button>
There is a tab stop between "hello" and "world". It isn't visible, the :focus style doesn't apply, but activeElement is set. Here's where to find it in the HTML spec: https://html.spec.whatwg.org/#bc-focus-ergo-bcc-focus
I think what we need to do is update isFocusable
, give it a flag to return true for iframes. Something like this:
const myFrame = document.querySelector('iframe')
isFocusable(myFrame) // false
isFocusable(myFrame, { focusRouters: true }) // true
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.
That's fine, but I think that's outside the scope of this pr. We should create another pr that fixes the iframe issue of isFocusable.
Co-authored-by: Wilco Fiers <[email protected]>
Co-authored-by: Wilco Fiers <[email protected]>
Co-authored-by: Wilco Fiers <[email protected]>
Co-authored-by: Wilco Fiers <[email protected]>
|
||
it('throws an error if the tree is incomplete', function() { | ||
fixture.innerHTML = | ||
'<ul role="presentation"><li id="target">foo</li></ul>'; |
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 get it throws without the ul, why does it throw when the ul is there?
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.
Because I specifically made the virtual tree have just the <li>
to show that it will fail even if the parent exists in the DOM.
var node = fixture.querySelector('#target');
flatTreeSetup(node);
lib/commons/aria/get-role.js
Outdated
dt: ['dl'], | ||
dd: ['dl'], | ||
dl: [] |
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 just looked these up, as far as I can tell this stuff doesn't work with dl, which is good because otherwise we'd have to deal with div being allowed to group dd / dt elements.
dt: ['dl'], | |
dd: ['dl'], | |
dl: [] |
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.
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 had assumed <div>
would stop the chain as we've seen in other things, but after testing it does in fact carry through the role inheritance so we'll need to add that.
With proper conflict resolution, we can now set up rule
selector
to be a matcher which would allow us to only find elements that match their role. This would allow simpler resolutions, such as the bug #2217 could be fixed by having theselector
be:and since the presentation role on the
ul
orol
parent would change the role topresentation
for the child, we would not have to look at the parent role in the check (the same for similar bug fordl
elements).Closes issue: #2212
Reviewer checks
Required fields, to be filled out by PR reviewer(s)