fix(2859) presentation role conflict - #2948
Conversation
| if (prohibited.length === 0) { | ||
| return false; | ||
| } | ||
There was a problem hiding this comment.
Odd, looks like prettier isn't running on my machine :/. I'll have to look at that.
| // until we have proper implicit role lookups for svgs we will | ||
| // avoid giving them one | ||
| if (node && node.namespaceURI === 'http://www.w3.org/2000/svg') { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Why did you remove this?
There was a problem hiding this comment.
It looked like an unnecessary check - we don't have implicit role lookups for svg, so it will return null without this check based on the lines that follow.
If we left it in, we'd have to add another check here for whether to use the chromium roles, and return the svg's implicit chromium role or else null. That's the same logic as is below.
| if (!role) { | ||
| if (chromiumRoles) { |
There was a problem hiding this comment.
Not a fan of these double if's. Can you refactor so this function flattens out? Could probably just do a return role || null at the end of the function, instead of returning null in a separate if.
There was a problem hiding this comment.
Were you thinking something like this?
if (!role && chromiumRoles) {
const chromiumRole = implicitChromiumRoles[nodeName];
return chromiumRole || null;
}
if (typeof role === 'function') {
return role(vNode);
}
return role || null;
}
| import { getNodeFromTree } from '../../core/utils'; | ||
| import AbstractVirtuaNode from '../../core/base/virtual-node/abstract-virtual-node'; | ||
|
|
||
| const implicitChromiumRoles = { |
There was a problem hiding this comment.
Looking at this list now... this could probably just be a property on standards/html-elms.js. That's probably better, that would make it configurable. So maybe something like:
const htmlElms = {
audio: {
...otherProps,
chromiumRole: 'Audio'
}
}There was a problem hiding this comment.
Would that be preferable to including it on standards/implicit-html-roles, which is currently what getImplicitRole uses?
Co-authored-by: Wilco Fiers <[email protected]>
Co-authored-by: Wilco Fiers <[email protected]>
| * @param {boolean} options.abstracts Allow role to be abstract | ||
| * @param {boolean} options.dpub Allow role to be any (valid) doc-* roles | ||
| * @param {boolean} options.noPresentational return null if role is presentation or none | ||
| * @param {boolean} options.includeChromiumRoles Include implicit roles from chromium-based browsers in role result |
There was a problem hiding this comment.
This looks a little out of place with the rest. Just calling it .options.chormium seems like it would be more consistent with the other options here.
| contentTypes: ['phrasing', 'flow'], | ||
| allowedRoles: false | ||
| allowedRoles: false, | ||
| chromiumRole: 'progressbar' |
There was a problem hiding this comment.
Should this be capitalised? If it isn't in Chromium then not, but it seems inconsistent.
There was a problem hiding this comment.
Yep, they are all inconsistent in Chrome(ium)
| }, | ||
| allowedRoles: ['application'] | ||
| allowedRoles: ['application'], | ||
| chromiumRole: 'video' |
There was a problem hiding this comment.
Same here. Why is it Audio but not Video?
There was a problem hiding this comment.
Same answer - it's inconsistently reported in the browser.
| import { getImplicitRole } from '../commons/aria'; | ||
|
|
||
| function hasImplicitChromiumRoleMatches(node, virtualNode) { | ||
| return getImplicitRole(virtualNode) !== null; |
There was a problem hiding this comment.
This still needs to use the new option. We'll need tests for it too.
Co-authored-by: Wilco Fiers <[email protected]>
|
Reviewed for security. |
* fix typo * add matcher so rule only applies when element has an implicit role that could conflict * add matcher so rule only applies when element has an implicit role that could conflict * use virtual node * add chromium implicit roles to lookup * remove temp variable Co-authored-by: Wilco Fiers <[email protected]> * use chromiumroles for getimplicitrole in this matcher Co-authored-by: Wilco Fiers <[email protected]> * rename chromiumRoles to includeChromiumRoles to add clarity and add it to getRole * refactor aria-prohibited-attr and add test * rename to has-implicit-chromium-role-matches * put chromium role on standards/html-elm and use it to get the implicit chromium role * consolidate function parameters Co-authored-by: Wilco Fiers <[email protected]> * rename option to just chromium Co-authored-by: Wilco Fiers <[email protected]>
…low label and body from being named (#5259) After looking over the HTML in ARIA spec, I realized that we could better handle how we determine when an attribute is prohibited. Basically if the element doesn't have an explicit role, then we use the `htmlElm` spec to determine prohibited attrs and naming, otherwise we use the `ariaRoles` spec to determine that. This should allow us to eliminate the `elementsAllowedAriaLabel` option that was [introduced to fix which elements allowed naming](#2935) and then modified when we [added Chromium roles](#2948) to fix conflict name resolution. However the option still has the `applet` element which isn't in our html spec so I'm leaving it for now. Elements which allow naming now include: `section`, `blockquote`, `address`, `hgroup`, `ruby`. This also adds enforcement for elements that are not allowed any aria-* attribute (which has never been used in our code) by changing the `noAriaAttrs` spec property to an empty `allowedAriaAttrs` so that they now throw in the rule `aria-allowed-attr`. Closes: #5185 Closes: #3410
2 participants
Adds a matcher to the presentation-role-conflict rule so that only elements that have an implicit role that could potentially conflict with an explicit role should be tested against the rule.
Since
divs don't have an implicit role and thus should not be matched by the rule, two of the passing tests had to be removed.Closes issue: #2859