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.

Sign up for GitHub

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

Jump to bottom

fix: filter block a11y #980

Merged
merged 1 commit into from
Sep 5, 2024
Merged

fix: filter block a11y #980

merged 1 commit into from
Sep 5, 2024

Conversation

PahaN47
Copy link
Contributor

@PahaN47 PahaN47 commented Aug 7, 2024

No description provided.

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

NikitaCG
NikitaCG requested changes Aug 9, 2024
View reviewed changes
src/sub-blocks/Content/Content.tsx Outdated
@@ -67,10 +68,11 @@ const Content = (props: ContentProps) => {
title={titleProps}
colSizes={{all: 12}}
id={titleId}
{...a11yProps?.title}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we need a11yProps if we have static role for element ?title always title, list always list and e.t.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this specific case its here to give Title role="link"

Kyzyl-ool
Kyzyl-ool suggested changes Aug 9, 2024
View reviewed changes
src/sub-blocks/BasicCard/BasicCard.tsx Outdated
@@ -37,7 +33,7 @@ const BasicCard = (props: BasicCardProps) => {
className={b()}
contentClassName={b('content')}
{...cardParams}
extraProps={{'aria-describedby': descriptionId, 'aria-labelledby': titleId}}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to remove descriptions? we had lost the labels and desciptions from link cards

src/sub-blocks/BasicCard/BasicCard.tsx Outdated
@@ -37,7 +33,7 @@ const BasicCard = (props: BasicCardProps) => {
className={b()}
contentClassName={b('content')}
{...cardParams}
extraProps={{'aria-describedby': descriptionId, 'aria-labelledby': titleId}}
extraProps={{role: 'group'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the card (especially card with url) should have group role? If one needs the group role specified, then it should be possible to specify it via props, e.g. via extraProps.

src/sub-blocks/BasicCard/BasicCard.tsx Outdated
additionalInfo={additionalInfo}
links={links}
list={list}
buttons={buttons}
size="s"
colSizes={{all: 12, md: 12}}
controlPosition={areControlsInFooter ? 'bottom' : 'default'}
a11yProps={{
title: {role: 'link', tabIndex: 0},
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want link role, it is better to use <a> tag: no need to specify the role and tabIndex. But be aware of layout change due to browser default styles

@PahaN47
Copy link
Contributor Author

PahaN47 commented Aug 13, 2024

To address the comments:
The expected way of interacting with cards is not interacting with the card itself, but interacting with its contents sequentially.
In testing I noticed that the user would focus on the entire card, then they would hear the title and go on to the next card. It appears that descriptions are not read out loud by default in some screen reader programs.
When the user was told there was a description that they missed only then they were able to find and access it.
Then the user knowing the way cards are usually handled assumed that they were focusing on a card title which was a link and it had aria-describedby with the card description. So they commented that there is no need to add the description to title's aria-desccribedby as it is normal to traverse cards by focusing on both their title link and description one after another.
What the user had no way of knowing is that they had never focused on neither the card's title nor it's description. They were focused on the entire card element with aria-label containing title.
The goal of these changes was to bring the link-wrapped card up to the expected accessibility standard: a group of elements where title is a link and description is the following text that can be focused by screen readers as a separate element. Generally wrapping a card inside an <a> tag is considered a bad a11y practice, but changing that inside the CardBase component would be a potentially breaking change.
There may be flaws in my implementation, but the expected result nonetheless should be:

  • card itself is not focusable
  • title is a link
  • description is text that follows the title and is a separate element

@PahaN47
Copy link
Contributor Author

PahaN47 commented Sep 3, 2024

Removed changes related to cards as they are a separate issue now #998

@PahaN47 PahaN47 merged commit 41b2bc3 into main Sep 5, 2024
4 checks passed
@PahaN47 PahaN47 deleted the filter-block-a11y branch September 5, 2024 11:54
@gravity-ui-bot gravity-ui-bot mentioned this pull request Sep 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kyzyl-ool Kyzyl-ool Kyzyl-ool approved these changes

@NikitaCG NikitaCG NikitaCG approved these changes

@gorgeousvlad gorgeousvlad Awaiting requested review from gorgeousvlad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PahaN47 @gravity-ui-bot @Kyzyl-ool @NikitaCG