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

Two accordion examples, one basic and one of a group with only one open accordion #1834

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Mar 25, 2021

Related to #1830

This is an example for the purpose of discussion: it demonstrates having two separate accordion examples; one is a basic set of independent accordions, and the other is a grouped accordion example with the restriction of only having one accordion open at a time.

This isn't ready to be merged or formally reviewed -- it's missing documentation updates and tests.

Basic accordion preview link
Group accordion preview link

Review checklist


Preview | Diff


Preview | Diff

@JAWS-test
Copy link

It seems to me that the Basic Example has no difference to Disclosure. I wonder if we need the same example under two different terms?

@shirsha
Copy link

shirsha commented Jun 15, 2021

Thanks @smhigley for the changes.
Following are few suggestion. These were existing from previous code

  • Move the "+" and "-" before the accordion name so that users with screen magnifiers can see state easily.
  • Place the accordions in an unorder list so that it improves the experience for screen reader users.
  • Can we add note saying having the content under each accordion inside a labelled region is a best practice? Having too many landmarks will not help screen reader users. I recommend removing the markup related to region landmark.

@jongund jongund self-requested a review June 15, 2021 17:56
@jongund
Copy link
Contributor

jongund commented Jun 15, 2021

@smhigley
On macOS BigSur (11.4) using Safari and VoiceOver, the button seems to be hidden inside the h3 when you use next or previous item commands. Basically it identifies the heading element and the label, but no information about the button or the state of aria-expanded. When I press the next item command I expected to go to the button, but instead it went to the region.

Works fine when you use the TAB key or using the rotor, just goes to the button element element, but no information about the button being disable when aria-disabled='true is set.

Also I think there needs to be some visual indication when the open region cannot be collapsed, since there is a visual affordance that it can be.

@shirsha
Copy link

shirsha commented Jun 15, 2021

I don't see aria-hidden attribute on Edge, Chrome browser (Windows OS). Jemma can see it in Mac OS.

@jongund
Copy link
Contributor

jongund commented Jun 15, 2021

@smhigley
Works fine on iOS 14.4.2, using VO with Safari. But like VO for macOS it does not provide information about the aria-disabled=true information.

@jongund
Copy link
Contributor

jongund commented Jun 15, 2021

@smhigley
At the APG meeting today is was suggested to remove the aria-required and aria-hidden from the standard form controls, since they are not needed for the accordion pattern and increase the amount of documentation and testing for the example.

I think we also need to add more of a description of the accordion behavior in each example, indicating whether one will always be open or not.

@jongund
Copy link
Contributor

jongund commented Jun 16, 2021

I don't see aria-hidden attribute on Edge, Chrome browser (Windows OS). Jemma can see it in Mac OS.

It is used to hide the "*" in the labels for the input[type=text'] controls in the Group accordion example.

@mcking65 mcking65 linked an issue Oct 4, 2021 that may be closed by this pull request
@mcking65 mcking65 added the Example Page Related to a page containing an example implementation of a pattern label Oct 18, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Oct 18, 2021
@a11ydoer
Copy link
Contributor

@smhigley will write the test procedure.

@a11ydoer
Copy link
Contributor

a11ydoer commented Nov 2, 2021

@smhigley will complete testing code by early next week. Thanks so much, @smhigley

@smhigley smhigley marked this pull request as ready for review November 2, 2021 18:57
@smhigley smhigley changed the title Draft: Two accordion examples, one basic and one of a group with only one open accordion Two accordion examples, one basic and one of a group with only one open accordion Nov 2, 2021
@a11ydoer a11ydoer self-requested a review November 9, 2021 20:09
@a11ydoer
Copy link
Contributor

a11ydoer commented Nov 10, 2021

@mcking65 I think this accordion would not be able to make it to APG 1.2. From quick testing using NVDA, there is mismatch between visual and screen reader annoucment. For example, the accordion was expanded visually but the screenreader says it is "unavailable expanded".

Furthermore, "Jump to" funtion was not implemented either.

@jongund
Copy link
Contributor

jongund commented Nov 10, 2021

@a11ydoer @mcking65 @smhigley There needs to be documentation of the use of aria-disabled and it should also be in the original source code for the group example.

Might want to change:
"The form can only have one step open at a time"
to
"The form has one step open at a time"

@JAWS-test
Copy link

JAWS-test commented Nov 10, 2021

@a11ydoer

For example, the accordion was expanded visually but the screenreader says it is "unavailable expanded".

This has to be that way because of aria-disabled=true, since it can only be opened but not closed. I don't see any error in this, but the match of screenreader output and visual representation.

The only problem is that the initial aria-disabled is missing. It is added only after the operation

@a11ydoer
Copy link
Contributor

@JAWS-test thanks for great feedback! It makes sense. It seems that we need to work on aria-disabled more.

@smhigley
Copy link
Contributor Author

@mcking65 I updated the roles, tests, and related example links. Let me know what you think of it now :)

Copy link

@shirsha shirsha left a comment

Choose a reason for hiding this comment

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

I will raise a separate issue for my suggestions.

@a11ydoer
Copy link
Contributor

a11ydoer commented Apr 5, 2022

issue 974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
7 participants