-
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
feat: new rule landmark-complementary-is-top-level #1239
Conversation
cc291e6
to
07ae611
Compare
@@ -1,7 +1,7 @@ | |||
const nativeScopeFilter = 'article, aside, main, nav, section'; | |||
|
|||
// Filter elements that, within certain contexts, don't map their role. | |||
// e.g. a <footer> inside a <main> is not a banner, but in the <body> context it is | |||
// e.g. a <header> inside a <main> is not a banner, but in the <body> context it is | |||
return ( | |||
node.hasAttribute('role') || |
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.
Could this be extracted to "selector": "[role]"
?
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.
You mean in the JSON? The only reason this file has edits is it had a typo in it...
Edit: I don't think we actually need this file for asides.
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 ended up not using this file, and the only thing I changed in it was a comment typo. Leaving as-is.
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 to look it up too, but yes aside always maps to role= complementary, unlike footer and header which only map to a role if they are scoped to the body.
var aside = document.createElement('div'); | ||
aside.setAttribute('role', 'complementary'); | ||
aside.appendChild(mainLandmark); | ||
fixture.appendChild(aside); |
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.
Nitpick: I know, there are multiple tests written with DOM api like above using createElement
, appendChild
etc. I found that using the testutils.fixtureSetup()
function to be helpful for this, and saves us a few lines and also makes it easy to read.
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've updated the tests, you're right that they were more difficult to read!
@@ -23,6 +23,16 @@ describe('landmark-is-top-level', function() { | |||
assert.deepEqual(checkContext._data, { role: 'main' }); | |||
}); | |||
|
|||
it('should return false if the complementary landmark is in another landmark', 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.
Also, just had a thought, what happens if the first header
element after body
element happens with in shadowDOM
?
Question being, do we need to consider shadowDOM
for this rule?
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 check already has Shadow DOM coverage.
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.
See comments.
Also, seems like CI was blocked to move ahead on the integration test.
Output from CI:
http://localhost:9876/test/integration/full/landmark-complementary-is-top-level/landmark-complementary-is-top-level-fail.html
>> passes: 5, failures: 0, duration: 0.025s
Too long with no output (exceeded 10m0s)
It may be worth running circle ci locally to test the same. The command is circleci local execute --job test
@JKODU thanks for the tip. I almost got it working after some fiddling with my machine, but now it's stuck on PhantomJS for some reason. |
Best practice requiring asides and complementary landmarks to be top level, in line with the ARIA Authoring Practices Guide. Closes #795
07ae611
to
65a8485
Compare
The first version of this rule included a body context check copied over from the top-level-banner-landmark rule, and it made very flaky iframe tests. It wasn't really necessary since aside doesn't have the same behavior as header/role=banner.
@@ -1,7 +1,7 @@ | |||
const nativeScopeFilter = 'article, aside, main, nav, section'; | |||
|
|||
// Filter elements that, within certain contexts, don't map their role. | |||
// e.g. a <footer> inside a <main> is not a banner, but in the <body> context it is | |||
// e.g. a <header> inside a <main> is not a banner, but in the <body> context it is | |||
return ( | |||
node.hasAttribute('role') || |
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 to look it up too, but yes aside always maps to role= complementary, unlike footer and header which only map to a role if they are scoped to the body.
Best practice requiring asides and complementary landmarks to be top level, in line with the ARIA Authoring Practices Guide.
Closes #795
Reviewer checks
Required fields, to be filled out by PR reviewer(s)