fix(2859) presentation role conflict #2948
Conversation
test/integration/rules/presentation-role-conflict/presentation-role-conflict.html
Show resolved
Hide resolved
| }); | ||
|
|
||
| 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.
lib/commons/aria/implicit-role.js
Outdated
| 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;
}
lib/commons/aria/implicit-role.js
Outdated
| @@ -2,6 +2,19 @@ import implicitHtmlRoles from '../standards/implicit-html-roles'; | |||
| 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]>
lib/commons/aria/get-role.js
Outdated
| @@ -158,6 +165,7 @@ function resolveRole(node, { noImplicit, ...explicitRoleOptions } = {}) { | |||
| * @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.
| @@ -601,7 +607,8 @@ const htmlElms = { | |||
| }, | |||
| meter: { | |||
| 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)
| @@ -914,7 +923,8 @@ const htmlElms = { | |||
| contentTypes: ['embedded', 'phrasing', 'flow'] | |||
| } | |||
| }, | |||
| 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]>
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