Skip to content

Conversation

@bennypowers
Copy link
Member

@bennypowers bennypowers commented Dec 21, 2021

Adds unit tests for keyboard accessibility fixes in <pfe-accordion>

Related issues

Preview

Link(s) to demo page(s) where this element can be viewed:

What has changed and why

  • use delegatesFocus when attaching <pfe-accordion-header> shadowroot
  • unit tests for wai-aria practices

Testing instructions

  • Test case 1

    1. npm run test -- --watch --group default --files elements/pfe-accordion/test/pfe-accordion.spec.ts
  • Test case 2

    1. npm start
    2. Manual keyboard testing according to wai aria best practices

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
  • Tests have been updated to cover these changes.
  • Browser testing passed.
  • Changelog updated.
  • Documentation (README.md, WHY.md, etc.) updated or added.
  • Link to the demo recording:
  • Approved by designer.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the [email protected] mailing list!

@github-actions github-actions bot added functionality Functionality, typically pertaining to the JavaScript. tests Related to testing AT passed Automated testing has passed labels Dec 21, 2021
@bennypowers
Copy link
Member Author

paging @kelsS This PR attempts to fix accordion keyboard a11y within the pfe rewrite, #1804

To try it out for yourself, first ensure you have the prerequisites (nvm recommended) clone the pfe repo

git clone [email protected]:patternfly/patternfly-elements.git

then setup the node version with nvm use and install dependencies

npm ci

run the unit tests on the accordion with

npm run test -- --watch --group default --files elements/pfe-accordion/**/*.spec.ts

run the development server

npm start

I was able to get some of the tests passing with minimal code changes. I have some questions about behaviour before I continue implementing fixes:

  1. on the WAI-ARIA practices page (linked above) accordion is specified to close other panels, meaning only 1 panel is ever supposed to be open. Our accordion doesn't work like that, is that intended behaviour?

@bennypowers bennypowers force-pushed the feat/pfe-accordion-keyboard-a11y branch from aa8a9a1 to 5dcf92d Compare December 21, 2021 19:25
@bennypowers bennypowers force-pushed the feat/pfe-accordion-keyboard-a11y branch from 5dcf92d to 81f7e9a Compare December 21, 2021 19:36
@bennypowers bennypowers force-pushed the feat/pfe-accordion-keyboard-a11y branch from 7a95a7c to 7f52cf3 Compare December 27, 2021 17:43
@bennypowers
Copy link
Member Author

bennypowers commented Dec 27, 2021

Note to reviewers: at the target branch gets updated, a github action workflow tries (and fails) to update this branch. That makes it hard to find the relevant changes, so I'm linking to the test cases for keyboard accessibility here:

https://github.com/patternfly/patternfly-elements/blob/feat/pfe-accordion-keyboard-a11y/elements/pfe-accordion/test/pfe-accordion.spec.ts#L452-L904

I've also changed this to a draft so it doesn't get merged until it's been reviewed

@bennypowers bennypowers marked this pull request as draft December 27, 2021 17:47
@bennypowers bennypowers added the no-auto-update Prevent this PR from receiving automatid updates when its base branch is pushed to label Dec 27, 2021
@bennypowers
Copy link
Member Author

@markcaron helped me understand the authoring practices

Some implementations require one panel to be expanded at all times and allow only one panel to be expanded; so, they do not support a collapse function.

That means our implementation is fine. I'll update my tests

test(accordion): update keyboard a11y tests
@bennypowers bennypowers force-pushed the feat/pfe-accordion-keyboard-a11y branch from 4ed591a to 3f48c00 Compare December 29, 2021 07:14
@github-actions github-actions bot added the work in progress POC / Not ready for review label Dec 29, 2021
@bennypowers bennypowers marked this pull request as ready for review December 29, 2021 07:18
@bennypowers
Copy link
Member Author

I'm going to merge this into #1804 now, further review should proceed there

@bennypowers bennypowers merged commit c0e35ab into feat/pfe-tools Dec 29, 2021
@bennypowers bennypowers deleted the feat/pfe-accordion-keyboard-a11y branch December 29, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed functionality Functionality, typically pertaining to the JavaScript. no-auto-update Prevent this PR from receiving automatid updates when its base branch is pushed to tests Related to testing work in progress POC / Not ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants