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

[a11y] [EuiBasicTable] actions - overrides aria-label for multi-action button #7883

Closed
alexwizp opened this issue Jul 11, 2024 · 7 comments · Fixed by #7994
Closed

[a11y] [EuiBasicTable] actions - overrides aria-label for multi-action button #7883

alexwizp opened this issue Jul 11, 2024 · 7 comments · Fixed by #7994
Assignees
Labels

Comments

@alexwizp
Copy link
Contributor

Description

We have a couple of reported issues within #3637 related to the EuiBasicTable actions API.

The problem is with the default aria-label that we set for euiCollapsedItemActionsButton. For each row, it has the value All actions, which causes accessibility issues for users who use screen readers.

Unfortunately, the current API doesn't allow us to override the aria-label attribute. I see we can fix that issue by using CustomItemAction, but not sure that we should do that. Ideally, will be to find a way to address it for DefaultItemAction.

Could you please give us recommendations on how to resolve these issues?

Screen

image
@alexwizp alexwizp changed the title [a11y] [EuiBasicTable] Actions [a11y] [EuiBasicTable] overrides aria-label for multi-action button Jul 11, 2024
@alexwizp alexwizp changed the title [a11y] [EuiBasicTable] overrides aria-label for multi-action button [a11y] [EuiBasicTable] actions - overrides aria-label for multi-action button Jul 11, 2024
@mgadewoll
Copy link
Contributor

@alexwizp Am I correct to understand that we'd want each row to have a unique aria-label? What kind of label would you expect?

From what I can tell from the code, the "All actions" button is rendered per column. Even if we'd provide a custom render function as for other columns where a custom aria-label could be passed, it would still render the same for all per column. Instead we'd need to specify the labels per row specifically if they should be row specific. And while the internally used CollapsedItemActions button has access to the row data, that data is generic meaning we don't know what the data is. We can only pass along the item data back to the consumer via render function where the data is typed.

💭 My thought here for a possible solution in context of the current available code:

  • we define row data for each column which can be anything, e.g. we could define an actions key that can hold additional data (consumer side)
  • we allow to use custom render for the EuiTableActionsColumnType which returns all required data for a custom "all actions" button (currently using CollapsedItemActions) (EUI side)
  • we allow passing aria-label to CollapsedItemActions and use either the custom label or default i18n one conditionally (EUI side)
  • we need to surface CollapsedItemActions as export of EUI as it's currently an internal component of EuiBasicTable only (EUI side)

Example code:

type User = {
  firstName: string;
  actions?: {
    'aria-label'?: string;
  }
}

const columns = [
  {
    field: 'firstName',
    name: 'First Name'
  },
  {
    name: 'All Actions',
    actions: [
       // define actions as usual here
    ],
    render: (props) => {
      return (
        <CollapsedItemActions
          {...props}
          aria-label={props.item.actions?.['aria-label']}
        />
      );
    },
  }
]

@cee-chen @tkajtoch What do you think?

@alexwizp
Copy link
Contributor Author

Right,aria-labelshould be unique for each row. For example please see the reported issue elastic/observability-accessibility#39

I also want to add @1Copenut into that discussion.

@mgadewoll
Copy link
Contributor

@alexwizp What would be the expected label for those buttons? Should it be fully customizable or something like "All buttons, row N" similar to what was done here?

@alexwizp
Copy link
Contributor Author

@mgadewoll, it's a really good question. I think this approach is a bit different from what @1Copenut wants to see, but from an a11y perspective, it should also be okay. It is much easier to implement and doesn't require changing the API.

But let's wait input from @1Copenut / @dave-gus before continue.

@alexwizp
Copy link
Contributor Author

alexwizp commented Jul 23, 2024

@1Copenut just a reminder if you missed notification. Please have a look

@JasonStoltz
Copy link
Member

@1Copenut Do you have any advice on how to proceed?

@1Copenut
Copy link
Contributor

Apologies for the slow response. I must have confused this issue with another one. grimacing

Anyway, given the complexity with data grids, if we could have an accessible label like "All actions, row 1" that would meet the letter of the ask. Buttons now have a relationship to their related cells of content and screen reader Form Control menus have unique labels instead of multiple "All actions" buttons listed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants