Skip to content
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

Merged
merged 12 commits into from
Jun 23, 2020

Conversation

straker
Copy link
Contributor

@straker straker commented Jun 8, 2020

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 the selector be:

{
  "selector": {
    "nodeName": "li",
    "role": "listitem"
  }
}

and since the presentation role on the ul or ol parent would change the role to presentation for the child, we would not have to look at the parent role in the check (the same for similar bug for dl elements).

Closes issue: #2212

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@straker straker requested a review from a team as a code owner June 8, 2020 17:00
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
test/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
properties: { type: 'button' }
}
];
const alwaysTitleElements = ['iframe'];
Copy link
Contributor

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

Copy link
Contributor Author

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.

lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
lib/commons/aria/get-role.js Outdated Show resolved Hide resolved
straker and others added 6 commits June 17, 2020 08:50
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

it('throws an error if the tree is incomplete', function() {
fixture.innerHTML =
'<ul role="presentation"><li id="target">foo</li></ul>';
Copy link
Contributor

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?

Copy link
Contributor Author

@straker straker Jun 19, 2020

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 Show resolved Hide resolved
Comment on lines 42 to 44
dt: ['dl'],
dd: ['dl'],
dl: []
Copy link
Contributor

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.

Suggested change
dt: ['dl'],
dd: ['dl'],
dl: []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work in Chrome but does in Firefox, Safari/VO, and IE11/JAWS:

image

image

Copy link
Contributor Author

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.

@straker straker merged commit e207190 into develop Jun 23, 2020
@straker straker deleted the role-resolution branch June 23, 2020 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants