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

Revisit accordion behaviors on demo page #1819

Closed
smhigley opened this issue Mar 19, 2021 · 4 comments · Fixed by #1830
Closed

Revisit accordion behaviors on demo page #1819

smhigley opened this issue Mar 19, 2021 · 4 comments · Fixed by #1830
Assignees
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Milestone

Comments

@smhigley
Copy link
Contributor

smhigley commented Mar 19, 2021

Thanks to an overly long conversation last week, it seems that the current behavior in the accordion example might be encouraging some less-than-ideal assumptions about how accordions should behave in general.

The particular conversation I had was around using arrow keys to cycle through headers -- the misconception was that since the APG example was coded that way, arrow keys were inherently more beneficial and "accessible." I actually think the reverse is true: arrow keys are pretty important for scrolling the page particularly in content-heavy sections, which covers most accordions including ours. Taking that default functionality away seems generally more harmful than not, especially since the accordion pattern itself is already allowing users to skip past sections of content.

I can see why it might be useful in some very specific cases -- maybe if accordions were used to show/hide groups of links or checkboxes within a nav or filtering region, the benefit of not needing to tab back through a large number of items would be helpful enough to be worth taking away scrolling. The issue is that this doesn't match our example, and also we don't have any documentation on when arrow keys would beneficial.

This is also related to #1719 -- I like the suggestion there to clarify that not all accordions need to enforce one-at-a-time open state within a set. We could even just not model that behavior, and have all sections expandable and collapsible, then add documentation about the option to add one-at-a-time behavior if desired. Anecdotally, I've found that forcing one accordion to remain open is more likely to be confusing than helpful, unless there's a specific reason to include it. Since our specific example includes no context that would make one-at-a-time beneficial, I'd be in favor of removing it and modelling only the more simple expand/collapse behavior.

An overall accordion update would also be a great opportunity to update the component and resolve #1183 😄

TL;DR:
Let's take a look at making the following updates to Accordion:

  • Remove the arrow key behavior in our working example
  • Add documentation to the Keyboard Interaction section on when the optional arrow keys + home/end would actually make sense to implement
  • Consider removing the one-at-a-time behavior in the working example
  • Add documentation on having one-at-a-time behavior as an option (if we remove it), or put more emphasis on it not being necessary (if we keep it)
  • Generally update the code
@smhigley smhigley self-assigned this Mar 19, 2021
@JAWS-test
Copy link

JAWS-test commented Mar 19, 2021

especially since the accordion pattern itself is already allowing users to skip past sections of content.

The opened area cannot be skipped if the arrow key operation no longer exists

arrow keys are pretty important for scrolling the page

That's right. I wonder, however, if it would be sufficient to be able to scroll with page-up/down in the example (which is currently already possible). Besides, when navigating with arrow keys, the accordion also scrolls, namely to the next button.

So I am unsure if it is really necessary to remove the arrow key operation

We could even just not model that behavior, and have all sections expandable and collapsible

I can support that fully. I find it very confusing that the button at the opened area cannot be operated. This would also eliminate the problem of having to tab completely through a single area when the arrow key operation is removed.

@smhigley
Copy link
Contributor Author

Game plan from the APG call:

  1. I will create a PR that removes the arrow key + forcing-one-open behavior on this example, and clean up the code
  2. I'll open a second issue about re-adding the forcing-open behavior either as a checkbox (off by default), or in a second example that has a clearer use case, e.g. moving through a series of steps.

@jonathantneal
Copy link
Contributor

This seems to reduce accordions to an unconnected series of regions, which I’m not opposing. I enjoyed using the demo in #1830.

What is the difference between this pattern and a pattern that is a series of <details> with <summary> and headings?

As an author, when would I be better off using one pattern or the other?

@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern labels Oct 4, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Oct 4, 2021
mcking65 pushed a commit that referenced this issue Oct 4, 2021
…es, implement APG code guidelines (pull #1830)

Updates the accordion example to resolve issues #1819, #616, #304, and #1477.

Made the following Behavior changes:
* Removed optional arrow key support.
* Removed requirement forcing one section to always be expanded.
* Removed constraint limiting only one section to be expanded at a time.
* Updated tests to reflect behavior changes.
* Removed section that talked about focus styling for "enhanced keyboard interaction".
* Removed arrow key/home/end rows in example page keyboard table.
* Removed aria-disabled row in example page attributes table.

Code cleanup:
* Updated JS file to a class syntax.
* Update classnames to be lowercase.
* Fix some border-radius issues in focus styling.
* Updated example page wrapper id  attributes to match other example pages.
@a11ydoer
Copy link
Contributor

a11ydoer commented Mar 7, 2023

@smhigley I would like to add reference to the issue #597 when you work on ths issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Development

Successfully merging a pull request may close this issue.

5 participants