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

test(loading): add a11y tests #4621

Merged
merged 32 commits into from
Dec 6, 2019
Merged

test(loading): add a11y tests #4621

merged 32 commits into from
Dec 6, 2019

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Nov 8, 2019

reference #4611

Adds some base line checks for various required aria attributes on our Loading component. As well as AAT with Axe and DAP

I'll probably bundle a few components together moving forward but wanted a quick check on my style and approach here before I do too much more work. 👍

@dakahn dakahn requested a review from a team as a code owner November 8, 2019 00:19
@dakahn dakahn added this to Pull requests 📋 in Carbon React accessibility via automation Nov 8, 2019
@netlify
Copy link

netlify bot commented Nov 8, 2019

Deploy preview for the-carbon-components ready!

Built with commit b8e27b0

https://deploy-preview-4621--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 8, 2019

Deploy preview for carbon-elements ready!

Built with commit b8e27b0

https://deploy-preview-4621--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 8, 2019

Deploy preview for carbon-components-react ready!

Built with commit b8e27b0

https://deploy-preview-4621--carbon-components-react.netlify.com

@joshblack
Copy link
Contributor

Quick question, do these overlap with the tests in:

it('should be an assertive live region when active', () => {
const wrapper = mount(<Loading active={false} />);
const getLiveRegion = () => wrapper.find('[aria-live]');
expect(getLiveRegion().prop('aria-atomic')).toBe('true');
expect(getLiveRegion().prop('aria-live')).toBe('off');
expect(getLiveRegion().prop('aria-labelledby')).toBeDefined();
wrapper.setProps({ active: true });
expect(getLiveRegion().prop('aria-live')).toBe('assertive');
wrapper.setProps({ active: false });
expect(getLiveRegion().prop('aria-live')).toBe('off');
});
it('should have an associated label for the live region', () => {
const wrapper = mount(<Loading />);
const node = wrapper.find('[aria-live][aria-labelledby]');
const id = node.prop('aria-labelledby');
expect(wrapper.find(`#${id}`)).toBeDefined();
});

@dakahn
Copy link
Contributor Author

dakahn commented Nov 8, 2019

@joshblack For sure! I didn't catch that. Maybe let's move those into the new -accessibility block?

@joshblack
Copy link
Contributor

joshblack commented Nov 8, 2019

@dakahn would it help if we did a dedicated AVT group with different levels? e.g. a block for AVT, a nested block under that for AVT1, 2, 3, etc. Or does it make sense all together as a functional test?

Thinking about it more, here's what might live in each section:

  • AVT1
    • aXe or potentially DAP runs
  • AVT2
    • Using functional e2e tests with pressTab and pressEnter or pressSpace to verify reachability and operability
    • Zoom, not sure how to address this
  • AVT3
    • Assert relevant ARIA exists that must exist, checking for labels, etc

@dakahn
Copy link
Contributor Author

dakahn commented Nov 8, 2019

@joshblack do we want to do Axe now with an eye toward DAP later -- that was a question I had while working.

As for your suggestion. I like the concept, but the three levels of AVT are kinda IBM-only acronym, ya know? Maybe AAT, Keyboard, and Screenreader?

@joshblack
Copy link
Contributor

@dakahn aXe sounds great now, one thought was that the AVT1 test would be essentially an end-to-end test and we would run aXe on each meaningful stage.

Definitely down to swap out names, what would be preferred? Do we need to sub-divide into the categories like perceivable, operable, understandable, and robust? Or just keep it at AAT, Keyboard, Screen reader?

@dakahn
Copy link
Contributor Author

dakahn commented Nov 8, 2019

Even P.O.U.R is esoteric enough to maybe confuse. I like the three broad categories. How would we handle -- like in this case -- not having any specific keyboard accessibility tests?

@elizabethsjudd
Copy link
Contributor

@dakahn we have been using POUR and are actually swapping it out to the three step approach (because we are internal to IBM only we use AVT 1 -3 but I understand how for open source that might be more confusing). The issue we ran in to with POUR was that is was sometimes confusing to known what category a test/failure fell in to where the 1 - 3 was much more obvious and ultimately more clear to the reader as well.

@joshblack
Copy link
Contributor

joshblack commented Nov 8, 2019

Thanks so much for the insight @elizabethsjudd! Really appreciate it 🙏 Definitely not going down that path then haha.

@dakahn I think we're fine with omitting sections if it doesn't make sense. In this case, is the rational that we don't need keyboard because of its status being a live region? If using this in a context of a page, would we expect any keyboard behavior to be happening/not happening? Like if it's pull page loading, would we want nothing focusable, focus lock, etc.

@dakahn
Copy link
Contributor Author

dakahn commented Nov 8, 2019

Right, the expected behavior is that loading is not focusable, but still announces to screen readers

@dakahn dakahn changed the title test(loading): add a11y tests (WIP)test(loading): add a11y tests Nov 12, 2019
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Would we want to run both on under toHaveNoViolations? Or would that get too weird?

@dakahn
Copy link
Contributor Author

dakahn commented Nov 12, 2019

Keeping them distinct makes sense to me -- breadcrumb for debugging might be a little more clear, but I'm not committed to the idea if combining them is best.

packages/react/src/components/Loading/Loading-test.js Outdated Show resolved Hide resolved
packages/react/src/components/Loading/Loading-test.js Outdated Show resolved Hide resolved
packages/react/src/components/Loading/Loading-test.js Outdated Show resolved Hide resolved
packages/react/src/components/Loading/Loading-test.js Outdated Show resolved Hide resolved
@@ -9,6 +9,8 @@

global.__DEV__ = true;

jest.setTimeout(20000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work if we put it in the toHaveNoDAPViolationsmatcher instead of our setup file?

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 would have thought this was part of Jest's setup -- at least that's what the Jest docs implied. That's where I'd look for it at least 🦐

@joshblack joshblack requested a review from a team as a code owner November 19, 2019 23:52
@ghost ghost requested review from aledavila and asudoh November 19, 2019 23:52
@joshblack joshblack merged commit a0f1d75 into carbon-design-system:master Dec 6, 2019
Carbon React accessibility automation moved this from Pull requests 🙏🏿 to Closed issues 🏁 Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Carbon React accessibility
Closed issues 🏁
Development

Successfully merging this pull request may close these issues.

None yet

6 participants