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

doc: improve table accessibility #35146

Merged
merged 1 commit into from
Sep 12, 2020
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 10, 2020

The compatibility matrix in the N-API doc needs row headers for
accessibility purposes. Unfortunately, those aren't possible in markdown
without resorting to HTML markup. So this converts the table from
markdown to HTML. This is less convenient for documentation authors and
for other readers of the raw markdown docs, but I believe accessibility
for the viewers of our HTML docs outweighs that concern.

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 10, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added the node-api Issues and PRs related to the Node-API. label Sep 10, 2020
@Trott
Copy link
Member Author

Trott commented Sep 10, 2020

Copy link

@gittysachin gittysachin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

I think this is still missing <thead> and <tbody> tags. Perhaps adding a <caption> would be nice too? Not sure what it would say though.

@ahtee
Copy link

ahtee commented Sep 10, 2020

Nit: Quick question on this ❓ ,
When inspecting the table on the live site - I'm using Edge Devtools - I see the table has this already, minus the captions (Apologies for the large screenshot) 😅

See the table to the left on the page here and the compiled markdown-to-html in the devtools on the right.

image

@Trott
Copy link
Member Author

Trott commented Sep 11, 2020

I see the table has this already

What does "this" refer to?

@DerekNonGeneric
Copy link
Contributor

What does "this" refer to?

The <thead> and <tbody> tags.

@Trott
Copy link
Member Author

Trott commented Sep 11, 2020

What does "this" refer to?

The <thead> and <tbody> tags.

I'm not vehemently opposed to including the thead and tbody elements, but I do have my reasons for omitting them in this context.

They add no value to assistive technology in this context. Many authors misunderstand them, thinking for example that you can't or shouldn't include th elements inside a tbody element. If we include them in this table, that is exactly the situation we will be in: th row headers in the tbody element. And I fear that someone trying to "help" will change the th row headers back to td, making the page harder for assistive technology users to consume.

Since they are optional in this situation, I chose to omit them.

All that said: If there's a good argument for including them, sure.

@Trott
Copy link
Member Author

Trott commented Sep 11, 2020

(We should also style the tables so that headers are visually differentiated from data, but that's a separate PR.)

@DerekNonGeneric
Copy link
Contributor

The <thead> and <tbody> elements are Semantic Web / a11y features used by screen readers, etc. Please do add them. :)

@DerekNonGeneric
Copy link
Contributor

@Trott, I'm not trying to be a stickler here, but given the name of the PR title and branch, I would hope we do our best here.

@Trott
Copy link
Member Author

Trott commented Sep 11, 2020

The <thead> and <tbody> elements are Semantic Web / a11y features used by screen readers, etc. Please do add them. :)

It's a common misconception, but thead and tbody are not a11y features. They are not used by screen readers or other assistive technology. From https://webaim.org/techniques/tables/data:

The thead and tfoot elements define header and footer rows for tables. They provide no accessibility functionality and are generally only of use when a long table is printed - the head and/or foot rows will repeat at the top or bottom of each printed page. Similarly, the tbody element defines the body content of a data table (meaning anything that's not a thead or tfoot). Again, this element does not provide any additional accessibility benefit, but there is no harm in using it for table styling or other reasons.

@DerekNonGeneric
Copy link
Contributor

Hmm, you might be right, but I distinctly remember listening to a screen reader speak out that it was inside of a table head.

Evidence to your point: https://developer.mozilla.org/en-US/docs/Learn/HTML/Tables/Advanced#Tables_for_visually_impaired_users

I guess all that would be left to justify them would be if you'd use these tags as CSS selectors for your styling PR.

@Trott
Copy link
Member Author

Trott commented Sep 12, 2020

Landed in b123e08

@Trott Trott merged commit b123e08 into nodejs:master Sep 12, 2020
@Trott Trott deleted the accessible-table branch September 12, 2020 17:05
The compatibility matrix in the N-API doc needs row headers for
accessibility purposes. Unfortunately, those aren't possible in markdown
without resorting to HTML markup. So this converts the table from
markdown to HTML. This is less convenient for documentation authors and
for other readers of the raw markdown docs, but I believe accessibility
for the viewers of our HTML docs outweighs that concern.

PR-URL: nodejs#35146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
The compatibility matrix in the N-API doc needs row headers for
accessibility purposes. Unfortunately, those aren't possible in markdown
without resorting to HTML markup. So this converts the table from
markdown to HTML. This is less convenient for documentation authors and
for other readers of the raw markdown docs, but I believe accessibility
for the viewers of our HTML docs outweighs that concern.

PR-URL: #35146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
4 tasks
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
The compatibility matrix in the N-API doc needs row headers for
accessibility purposes. Unfortunately, those aren't possible in markdown
without resorting to HTML markup. So this converts the table from
markdown to HTML. This is less convenient for documentation authors and
for other readers of the raw markdown docs, but I believe accessibility
for the viewers of our HTML docs outweighs that concern.

PR-URL: nodejs#35146
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.