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

Fix Table aria attributes #1208

Merged
merged 2 commits into from
Oct 2, 2018

Conversation

jsomsanith-tlnd
Copy link
Contributor

@jsomsanith-tlnd jsomsanith-tlnd commented Sep 5, 2018

What is the problem this PR is trying to solve?
#1199 explain accessibility issues. It refers to http://w3c.github.io/aria-practices/#grid.
This PR try to solve the accessibility on Table html.

Current markup is

<div role="grid" />
	<div role="row">
		<div role="columnheader"/>
		<div role="columnheader"/>
	</div>
	<div role="rowgroup">
		<div role="row">
			<div role="gridcell" />
			<div role="gridcell" />
		</div>
	</div>
</div>

What is the chosen solution to this problem?
Follow https://www.w3.org/TR/2018/NOTE-wai-aria-practices-1.1-20180726/examples/grid/dataGrids.html

New markup is

<div role="grid" aria-label="from props" aria-colcount="2" aria-rowcount="1" /> // label and col/row count
	<div role="rowheader"> . // row --> rowheader
		<div role="columnheader" aria-sort="none"/> // sortable but not current
		<div role="columnheader"/>
	</div>
	<div role="rowgroup">
		<div role="row" aria-rowindex="1" > // aria-rowindex
			<div role="gridcell" aria-colindex="1" /> // aria-colindex
			<div role="gridcell" aria-colindex="2" />
		</div>
	</div>
</div>

Before submitting a pull request, please complete the following checklist:

  • The existing test suites (npm test) all pass
  • For any new features or bug fixes, both positive and negative test cases have been added
  • For any new features, documentation has been added
  • For any documentation changes, the text has been proofread and is clear to both experienced users and beginners.
  • Format your code with prettier (npm run prettier).
  • Run the Flow typechecks (npm run typecheck).

edit:

closes: #1199

Copy link

@backwardok backwardok left a comment

Choose a reason for hiding this comment

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

Thanks for working on fixing the a11y issue I filed!

I think you'd also need to update Grid as well (the original bug was referencing the Grid component).

// Note that we specify :rowCount, :scrollbarWidth, :sortBy, and :sortDirection as properties on Grid even though these have nothing to do with Grid.
// This is done because Grid is a pure component and won't update unless its properties or state has changed.
// Any property that should trigger a re-render of Grid then is specified here to avoid a stale display.
return (
<div
aria-label={this.props['aria-label']}
aria-labelledby={this.props['aria-labelledby']}

Choose a reason for hiding this comment

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

This prop isn't in the list of props - seems like it needs to be added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add them in propTypes definition

className={cn('ReactVirtualized__Table', className)}
id={id}
role="grid"
tabIndex="0"

Choose a reason for hiding this comment

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

I noticed that there's a tabIndex prop that's never used in this component, though I also don't think that value should ever be above 0 anyway so it's probably better that it's not being used. I wonder if that prop should just be removed?

Copy link
Contributor Author

@jsomsanith-tlnd jsomsanith-tlnd Sep 6, 2018

Choose a reason for hiding this comment

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

👍 I was testing something, adding it to the focus flow but i was a misundertanding of how SR works. I remove it.

@@ -378,19 +378,26 @@ export default class Table extends React.PureComponent {
};
});

const columns = this._getHeaderColumns();

Choose a reason for hiding this comment

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

This number won't be accurate if disableHeader is set to true. Is it possible to calculate the number of columns a different way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll just get the number of children

@@ -8,7 +8,7 @@ export default function defaultHeaderRowRenderer({
style,
}: HeaderRowRendererParams) {
return (
<div className={className} role="row" style={style}>
<div className={className} role="rowheader" style={style}>

Choose a reason for hiding this comment

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

role="row" was correct here. rowheader is for the cell that represents the heading for a row, if it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my bad, bad understanding of the value. Thanks

@jsomsanith-tlnd
Copy link
Contributor Author

About the Grid component, it's something quite generic. I'm not sure about how we could do that. Adding an extra div to wrap the rows will make it not compatible with the Table implementation.

For now there is a workaround, passing your own cellRenderer (that's what Table does). Or use Table with disableHeader.

@koenoe
Copy link

koenoe commented Sep 26, 2018

This looks good to me and I could really use this. Can we merge this PR in?

@wuweiweiwu wuweiweiwu self-assigned this Sep 26, 2018
Copy link
Contributor

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

Awesome! thanks for contributing! I release a minor version within a week or so :)

@wuweiweiwu wuweiweiwu merged commit 15e0791 into bvaughn:master Oct 2, 2018
@jsomsanith-tlnd jsomsanith-tlnd deleted the jsomsanith/fix/table_a11y branch October 2, 2018 08:29
@koenoe
Copy link

koenoe commented Oct 15, 2018

Thanks for merging it in! When can we expect a new release?

@wuweiweiwu
Copy link
Contributor

@koenoe hopefully this weekend!

errendir added a commit to IdeaFlowCo/react-virtualized that referenced this pull request Oct 24, 2018
* bvaughn/master: (54 commits)
  Update version and changelog for 9.21.0 release (bvaughn#1252)
  chore: update lockfile
  Update ci badge (bvaughn#1227)
  Allow users to override default table row styles (bvaughn#1175)
  Add onColumnClick to Table (bvaughn#1207)
  remove unused variable in Masonry.example.js (bvaughn#1218)
  Fix Table aria attributes (bvaughn#1208)
  Fix typo in CellMeasurer.DynamicHeightTableColumn.example.js (bvaughn#1190)
  Update usingAutoSizer.md (bvaughn#1186)
  Add an extra check for an e.target.className.indexOf function (bvaughn#1210)
  Fix broken Slack badge image (bvaughn#1205)
  docs(CellMeasurer): fix `import` statement (bvaughn#1187)
  Added new friend (bvaughn#1197)
  Fix createMultiSort bug (bvaughn#1051)
  adding new usecase example and fix some typos (bvaughn#1168)
  Updating version to 9.20.1
  Update changelog for the 9.20.1 release (bvaughn#1167)
  Prevent early debounceScrollEndedCallback when there is a slow render (bvaughn#1141)
  removing sideEffects (bvaughn#1163)
  fix for bvaughn#998 with test cases (bvaughn#1154)
  ...
@asnewman asnewman mentioned this pull request Jan 7, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grid is not accessible using VoiceOver
4 participants