-
Notifications
You must be signed in to change notification settings - Fork 776
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
fix: Allow div groups for dlitem rule #1284
Conversation
|
||
// Unlike with UL|OL+LI, DT|DD must be in a DL | ||
if (parentTagName !== 'DL') { | ||
return false; | ||
} | ||
|
||
const parentRole = (parent.getAttribute('role') || '').toLowerCase(); | ||
if (!parentRole || !axe.commons.aria.isValidRole(parentRole)) { |
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.
isValidRole is done by aria.getRole()
, in case you're wondering.
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'm surprised it works with the noImplicit: true
, can you explain that bit? This applies to native elements as well as bolt-on roles, right? The tests seem to pass, so 🤷♀️
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.
The requirements here are: It needs to be in a dl
, and if that dl
has a role attribute, it can only be a list
. So what's happening is first I check the parentTagName, and then I check if the explicit role (which is what we get from noImplicit: true
is null
or 'list'
.
test/checks/lists/dlitem.js
Outdated
}); | ||
}); | ||
|
||
it('returns false if the dd/dt is in a div with a role with a dl as grandparent', function() { |
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'd add with a list role
to the test name because that's the important bit
test/checks/lists/dlitem.js
Outdated
@@ -88,4 +144,17 @@ describe('dlitem', function() { | |||
assert.isFalse(checks.dlitem.evaluate.apply(null, checkArgs)); | |||
} | |||
); | |||
|
|||
(shadowSupport.v1 ? it : xit)( | |||
'should return truewhen the item is grouped in dl > div', |
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.
Add a space to the test name: true when
. It should also mention Shadow DOM
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.
Just a few comments about test names, otherwise it looks good
Comments resolved. Please take another look.
We wrap each name-value group in the summary list's `<dl>` with a grouping `<div>` element, which is valid according to the current WHATWG HTML spec [1]. At the time we wrote these tests, this was a relatively recent change to the HTML spec, and the axe rules had not been updated to allow it. Since then, the underlying axe rules have been updated to allow wrapping `<div>` elements [2], so we no longer need to disable the `dlitem` and `definition-list` (released in axe-core 3.2.0 [3]) [1]: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element [2]: dequelabs/axe-core#1284 [3]: https://github.com/dequelabs/axe-core/blob/develop/CHANGELOG.md#320-2019-03-04
We wrap each name-value group in the summary list's `<dl>` with a grouping `<div>` element, which is valid according to the current WHATWG HTML spec [1]. At the time we wrote these tests, this was a relatively recent change to the HTML spec, and the axe rules had not been updated to allow it. Since then, the underlying axe rules have been updated to allow wrapping `<div>` elements [2], so we no longer need to disable the `dlitem` and `definition-list` (released in axe-core 3.2.0 [3]) [1]: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element [2]: dequelabs/axe-core#1284 [3]: https://github.com/dequelabs/axe-core/blob/develop/CHANGELOG.md#320-2019-03-04
Allow grouping in divs for the dlitem rule.
Closes #1265
Reviewer checks
Required fields, to be filled out by PR reviewer(s)