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): improve sortable table accessibility #1280

Merged
merged 27 commits into from
May 9, 2023
Merged

Conversation

dancormier
Copy link
Contributor

This PR is built on top of #1144. It includes a merge of develop with conflict fixes the addition of a test (currently failing, but it's a start), the porting of styles to the refactored tables.less. Once the test is in a good place, we should be able to merge this work.

@allejo I couldn't commit directly to your branch, so I made this one. Feel free to cherry pick from here or lift anything that's useful into your branch.

@netlify
Copy link

netlify bot commented Feb 24, 2023

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit d7a1a6b
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/645a80fb6087c90008cb61a5
😎 Deploy Preview https://deploy-preview-1280--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dionrhys
Copy link
Member

Just noticed this new PR for sortable tables - copying over my PR comments from #1144 so we don't miss some doc + styling fixes:

I'm really excited by this change! We'll be wanting sortable tables in an internal project and having sorting be keyboard and screen-reader accessible would be a great thing.

Here's another link for reference: https://adrianroselli.com/2021/04/sortable-table-columns.html. It's marked out-of-scope for this PR but this link also presents various different ways to have sortability and sort change to be announced for screen readers. If we do enhance the feature with instructions/announcements here or in future, the HTML will need to provide localised/translated strings for these.


I think we should update the documentation in this PR to ensure devs write in the initial aria-sort attribute for the non-JS table examples where columns are already sorted. (It'd also apply to the JS examples, but those columns aren't pre-sorted in the examples)

Something like this for example:

--- a/docs/product/components/tables.html
+++ b/docs/product/components/tables.html
@@ -634,14 +634,14 @@ description: Tables are used to list all information from a data set. The base s
 <section class="stacks-section">
     {% header "h2", "Sortable tables" %}
     <p class="stacks-copy">To indicate that the user can sort a table by different columns, add the <code class="stacks-code">s-table__sortable</code> class to the table.</p>
-    <p class="stacks-copy">The <code class="stacks-code">&lt;th&gt;</code> cells should include arrows to indicate sortability or the currently applied sorting. In addition, the column that is currently sorted should be indicated with the <code class="stacks-code">is-sorted</code> class on its <code class="stacks-code">&lt;th&gt;</code>.</p>
+    <p class="stacks-copy">The <code class="stacks-code">&lt;th&gt;</code> cells should include arrows to indicate sortability or the currently applied sorting. In addition, the column that is currently sorted should be indicated with the <code class="stacks-code">is-sorted</code> class and the <code class="stacks-code">aria-sort</code> attribute with the <a href="https://www.w3.org/TR/wai-aria-1.1/#aria-sort">appropriate value</a> on its <code class="stacks-code">&lt;th&gt;</code>.</p>
     <div class="stacks-preview">
 {% highlight html %}
 <div class="s-table-container">
     <table class="s-table s-table__sortable">
         <thead>
             <tr>
-                <th scope="col" class="is-sorted">Listing @Svg.ArrowDownSm</th>
+                <th scope="col" class="is-sorted" aria-sort="descending">Listing @Svg.ArrowDownSm</th>
                 <th scope="col">Status @Svg.ArrowUpDownSm</th>
                 <th scope="col">Owner @Svg.ArrowUpDownSm</th>
                 <th scope="col" class="ta-right">Views @Svg.ArrowUpDownSm</th>
@@ -659,7 +659,7 @@ description: Tables are used to list all information from a data set. The base s
                 <table class="s-table s-table__sortable">
                     <thead>
                         <tr>
-                            <th scope="col" class="is-sorted">
+                            <th scope="col" class="is-sorted" aria-sort="descending">
                                 Listing
                                 {% icon "ArrowDownSm" %}
                             </th>

Screen readers can announce the initial state of the sorting even if interactive sorting isn't enabled for the table.


I've noticed that the styling doesn't quite match the previous appearance, especially noticeable in the font using the default <button> font instead of the bolded header font. The cursor is the regular arrow at the moment (instead of cursor) and it's also not possible to select and copy the header text any more:

Screenshot of JS table example with button elements that use default font and can't be selected

I think it just needs some CSS tweaks to get it looking and behaving like before when the text was directly within the <th>. The s-btn__unset class has rules font: unset and user-select: auto that I think fix these. Plus I think cursor: pointer can be added to fix that cursor. Applying these new rules seem to fix things up:

Screenshot of JS table example with button elements that retain font and text selectability of the encompassing TH

(It might need additional tweaking for things I haven't noticed are not identical to before)

I'd love it if the headers had a focus shadow/outline like we see on other elements (right now it's default for button), but I have no idea how to get those showing up without being cropped by the boundaries of the <th> element.


Now that there's a test framework for Stacks, hopefully we can add in tests to verify no breaking changes here (e.g. test ensureHeadersAreClickable keeps working with future changes).

Great work on implementing this! 🥳 Let me know if there's anything I can help with re. testing or anything.

@allejo allejo self-assigned this Mar 27, 2023
lib/tsconfig.test.json Outdated Show resolved Hide resolved
@allejo
Copy link
Contributor

allejo commented Mar 31, 2023

@StackExchange/stacks I'll defer to you all on whether or not this PR is in a good spot to review/test/merge. I'm comfortable with the tests I've written so far and think they cover a good chunk of the component's functionality. I need to step away from this PR to work on a few other things on my plate so I don't have time to write tests for the actual sorting logic right now.

Where I left off:

  • Tests written for buildIndex and getCellSlot
  • Tests written for making sure the correct SVG indicators are shown
  • Tests written to make sure the right column (and no other columns) is marked with aria-sort
  • Tests written for confirming the sorting logic in the table cells
  • Address Dion's comments about the accidental styling changes

Notes:


I've removed myself from the reviewers list since I wrote the majority of this code and don't feel comfortable reviewing it myself.

@allejo allejo removed their request for review March 31, 2023 04:48
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Thanks @allejo to make progress on this PR.
I have reviewed the source code and left a couple of comments that needs to be addressed by our team before being able to merge.

I haven't spent much time reviewing the tests. At a glance they look quite complex, but so it's the algorithm. Thanks for starting the effort though.

I would like to close up this PR soon, let me catchup with @dancormier and see how we can address the remaining items.

lib/components/table/table.less Outdated Show resolved Hide resolved
*
* @param headerEl
*/
private ensureHeadersAreClickable = (headerEl: HTMLTableCellElement) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel strongly about NOT introducing a permanent markup transformer in Stacks to support legacy usage.
If we can automate the change at runtime we can do this also with a codemod that run once in Core before the library get upgraded.
We could look into codemods to automate everything but I suspect it will go faster to semi-automatically adjust (e.g. replace with regex in editor) the 20 occurrences in Core.

If we are concerned about breaking changes and semantic versioning we can make sure to support both elements (button and th) as valid targets until we are ready to ship v2.0 (where we can finally deprecate th).
If we decide to do that we should create some sort of a roadmap for Stacks 2.0.0 where we keep track of todos for the major release and also come up with a schedule (e.g. every 6 months we release a major).
Given that Stacks is primarily used by Core (we are not React or Bootstrap) I don't feel strongly about following semantic versioning by the book honestly (and for example communicate this small breaking change in a minor release v1.10.0). Alternatively we could release v2.0.0 and accept that the major numbers will change relatively often (in my previous team when I left we were at 12.x.x because we did not wait for breaking changes to pile up before cutting a release - as soon as they were ready we released and bump that major number). @dancormier any thoughts?

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'm trying to recall my thoughts from when we discussed this a couple weeks ago 😂 Feels like forever ago. Anyways…

I feel strongly about NOT introducing a permanent markup transformer in Stacks to support legacy usage.
If we can automate the change at runtime we can do this also with a codemod that run once in Core before the library get upgraded.
We could look into codemods to automate everything but I suspect it will go faster to semi-automatically adjust (e.g. replace with regex in editor) the 20 occurrences in Core.

I agree. I think the markup transformer is a little too much magic for Stacks. I think we could do the find+replace without too much complication. Only big issue I could see is if any tables are partially rendered with JS, but I figure we could always open a PR and find out.

I don't feel strongly about following semantic versioning by the book honestly

As for the semver discussion, I remember starting from a place of "I want to follow semver strictly" to "strict semver can cause complication for a library like ours". I doubt we'd benefit from waiting for several breaking changes as the overhead for our small team would be a lot, so we'd instead be doing major releases for any minor breaking change. So, I'm good to ship the occasional breaking change in minor versions as long as we communicate it and provide support.

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

@dancormier Thanks for bringing this PR to an end. I have left few final minor comments to address. After that feel free to merge.
Great work (also to you @allejo for initiating this in the first place) 🎉

lib/components/table/table.ts Outdated Show resolved Hide resolved
lib/components/table/table.test.ts Show resolved Hide resolved
docs/product/components/tables.html Show resolved Hide resolved
@dancormier dancormier merged commit 90b78fb into develop May 9, 2023
@dancormier dancormier deleted the pr/1144 branch May 9, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants