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

feat(TableContainer): support id-ref for the header #4807

Closed
wants to merge 9 commits into from

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Dec 3, 2019

This change adds a capability to specify the element ID of the <h4> in <TableContainer>. This allows application to set up our data table so VoiceOver announces the content in such <h4> when it announces "You are currently in a table" for <table>.

Fixes #3805.

Changelog

New

  • titleId prop in <TableContainer>.

Testing / Reviewing

Testing should make sure <TableContainer> is not broken.

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for the-carbon-components ready!

Built with commit 4abe74d

https://deploy-preview-4807--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for carbon-elements ready!

Built with commit b5edac6

https://deploy-preview-4807--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for carbon-components-react ready!

Built with commit b5edac6

https://deploy-preview-4807--carbon-components-react.netlify.com

@abbeyhrt
Copy link
Contributor

abbeyhrt commented Dec 3, 2019

Hey! @asudoh since this adds a new prop, is this technically a feat, not a fix?

@asudoh
Copy link
Contributor Author

asudoh commented Dec 3, 2019

@abbeyhrt You are right technically, I made it a fix given the addition is minor and it's for an a11y fix. But don't hesitate to speak up if you want feat commit message. Thanks!

@joshblack
Copy link
Contributor

This change adds a capability to specify the element ID of the `<h4>`
in `<TableContainer>`. This allows application to set up our data table
so VoiceOver announces the content in such `<h4>` when it announces
"You are currently in a table" for `<table>`.

Fixes carbon-design-system#3805.
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Definitely a bummer that we would need to add props to target inner children. I would assume we would also need to add descriptionId for aria-describedby? Are there any alternatives we could explore that don't rely on this?

@asudoh asudoh changed the title fix(TableContainer): support id-ref for the header feat(TableContainer): support id-ref for the header Dec 3, 2019
@asudoh
Copy link
Contributor Author

asudoh commented Dec 3, 2019

An alternative may be aria-label={title}. If we are OK with screen reader speaking both of title and description, no change is required for fixing the issue.

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

this looks good to me

@joshblack
Copy link
Contributor

@asudoh got it 👍 How would this work with the secondary label for the table btw? Specifically bx--data-table-header__description

@asudoh
Copy link
Contributor Author

asudoh commented Dec 10, 2019

User of assistive technology can still hear the secondary label via a way of reading regular text.

@joshblack
Copy link
Contributor

Would we need to use aria-describedby the same way we need aria-labelledby? That's all that I'm asking. I believe what you said also would apply to the title, but we are making this addition to the component API to support it. Specifically, the issue calls out that the title and description are unable to be used which is why I was asking about it.

@asudoh
Copy link
Contributor Author

asudoh commented Dec 11, 2019

Would we need to use aria-describedby the same way we need aria-labelledby?

Are you asking adding both of aria-labelledby pointing to title and aria-describedby pointing to description to <Table>, or something else?

@joshblack
Copy link
Contributor

That was my interpretation from the linked issue, let me know if I'm off here! Just trying to understand requirements to figure out the best way to support them.

@asudoh
Copy link
Contributor Author

asudoh commented Dec 12, 2019

Adding both of aria-labelledby pointing to title and aria-describedby pointing to description to <Table> causes VoiceOver read only the description not the title.

@joshblack
Copy link
Contributor

@asudoh I'm unable to reproduce, could you help me understand what the issue is?

demo

Regardless, would it make sense for this to configurable as a prop for the user to specify for title and description or would it help to integrate this with DataTable and getTableProps? Maybe through context? Another option seems to be not passing in a title and rendering the markup as children, in this case, would we want to expose the title and description as components that people could use?

@asudoh
Copy link
Contributor Author

asudoh commented Dec 13, 2019

I see both title/description are read on forward-navigation, but not backward-navigation.

@joshblack
Copy link
Contributor

Oh no! What were the steps that ended up with you getting that behavior? Here is what I was trying:

demo

@asudoh
Copy link
Contributor Author

asudoh commented Jan 16, 2020

I'm late on this, but here are the steps:

  1. Checkout the branch in this PR
  2. Run the React Storybook
  3. Open http://localhost:9000/iframe.html?id=datatable--default
  4. Go to DOM inspector
  5. Add id="table-container-description" to <p class="bx--data-table-header__description">
  6. Replace <table aria-describedby="table-container-title" class="bx--data-table bx--data-table--no-border"> with <table aria-labelledby="table-container-title" aria-describedby="table-container-description" class="bx--data-table bx--data-table--no-border">
  7. Set keyboard focus on "Show Info" button (The DOM element that comes next to the container of the <div class="bx--data-table-container">)
  8. Turn on VoiceOver
  9. Hit Ctrl-Option-Left
  10. VO annnounces "end of table", then "With default options. You are currently in a table. To navigate the cells in this table press Control-Option, and then Up Arrow, Down Arrow, Left Arrow, or Right Arrow."

Leaving such problem is OK for me though. Wrt getTableProps(), are you thinking of auto-generating the ID-refs with it (as well as getTableContainerProps())?

@joshblack
Copy link
Contributor

I think we could definitely go down that route if we're doing the titleId and descriptionId for aria-labelledby and aria-describedby since we already have getTableContainerProps.

Another idea was to create TableTitle and TableDescription components and do the wiring up like you have currently, e.g.

<TableContainer>
  <TableTitle id="table-title">Title of the table</TableTitle>
  <TableDescription id="table-desc">Table description</TableDescription>
  <Table aria-labelledby="table-title" aria-describedby="table-desc">
    {/* ... */}
  </Table>
</TableContainer>

@asudoh
Copy link
Contributor Author

asudoh commented Mar 11, 2020

Back on this, and made a change to auto-generate ID refs for aria-labeledby and aria-describedby.

@joshblack joshblack mentioned this pull request Mar 20, 2020
82 tasks
Base automatically changed from master to main March 8, 2021 16:35
@dakahn dakahn requested a review from a team as a code owner March 8, 2021 16:35
@joshblack joshblack closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants