Improve a11y support for table sorting

[allejo]

The current method of triggering a sort in tables is not accessible. This is due to the fact that you are attaching an event listener to the click event on an element that naturally is not clickable; therefore, keyboard only users are not capable of interacting with the headers since focus is never moved onto them.

My proposed syntax moves the contents of a table header cell into a button, which is where the data-action attribute will now live.

<th data-s-table-target="column">
    <button data-action="click->s-table#sort">
        Season
        @Svg.ArrowUpSm.With("js-sorting-indicator js-sorting-indicator-asc d-none")
        @Svg.ArrowDownSm.With("js-sorting-indicator js-sorting-indicator-desc d-none")
        @Svg.ArrowUpDownSm.With("js-sorting-indicator js-sorting-indicator-none")
    </button>
</th>

CSS Changes

I have added some rules described as follows:

• .s-table__sortable thead th[data-s-table-target="column"] :: if a th has this attribute attached to it, then it should be assumed that it's a sortable column and the padding will come from the <button> instead of it so that there's no weird gap between the button and the header cell that's not clickable
• .s-table__sortable thead th button :: there are certain rules that need to happen here so I've stuffed them here to avoid repetition in our markup
  • This can cause problems if there's a button with a popup attached to it. Is this likely to happen?

Backward Compatibility

I have introduced a ensureHeadersAreClickable method that transforms the old table syntax to the next syntax so that any Stacks table gets these a11y fixes for free! I currently have a warning in there telling devs in a non-prod environment to update their markup.

What do these changes NOT do?

This PR does not change/improve any screen reader announcements in terms of announcing new sorting or changing of table captions. For the moment, we'll continue to rely on an SR's support of aria-sort.

Resources

• W3 ARIA APG sortable table
• Deque sortable table

[netlify[bot]]

✅ Deploy Preview for stacks ready!

Name	Link
🔨 Latest commit	946a9687a9e02fa4662242be200c0ab6b3db378b
🔍 Latest deploy log	https://app.netlify.com/sites/stacks/deploys/635c76165e9d1f0009abb405
😎 Deploy Preview	https://deploy-preview-1144--stacks.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[dancormier]

The current method of triggering a sort in tables is not accessible. This is due to the fact that you are attaching an event listener to the click event on an element that naturally is not clickable; therefore, keyboard only users are not capable of interacting with the headers since focus is never moved onto them.

My proposed syntax moves the contents of a table header cell into a button, which is where the data-action attribute will now attentatively live.

<th data-s-table-target="column" class="s-table--sortable-column">
    <button class="s-table--clickable-header" data-action="click->s-table#sort">
        Season
        @Svg.ArrowUpSm.With("js-sorting-indicator js-sorting-indicator-asc d-none")
        @Svg.ArrowDownSm.With("js-sorting-indicator js-sorting-indicator-desc d-none")
        @Svg.ArrowUpDownSm.With("js-sorting-indicator js-sorting-indicator-none")
    </button>
</th>

This PR draft adds two new CSS classes for table header cells: .s-table--sortable-column and .s-table--clickable-header. I will leave it to the Stacks team to decide the best way to organize this Less and naming of the classes. But effectively, new Less needs to be introduced that does the following:

* Clear a button's appearance so that it looks no different from a regular table header cell

* Remove padding from `<th>` and move that to the `<button>` but only when a `<button>` exists

I like it! The only issue I see is with the naming of .s-table--clickable-header. I'd like to keep class names relating to presentation and not tied to their action/events (look away from .s-expandable 😝). That way, if the structure needs to change or styling works in context outside of a given action that can be attached, the class name will still make sense. I'll think on a more appropriate name, but feel free to make suggestions or tell me I'm being foolish. I haven't looked at the diff yet, so it's completely possible I'm speaking nonsense.

Now, in terms of discussion: I have introduced a ensureHeadersAreClickable method that transforms the old table syntax to the next syntax so that any Stacks table gets these a11y fixes for free! I currently have a warning in there telling devs in a non-prod environment to update their markup. Should that warning stay or should this transformation be considered a "feature?"

My vote is for the warning to stay. Tables (both the HTML element and our component) are convoluted enough that I'm in favor of almost any opportunity to unify the expected markup.

What do these changes NOT do?

This PR does not change/improve any screen reader announcements in terms of announcing new sorting or changing of table captions. For the moment, we'll continue to rely on an SR's support of aria-sort.

Resources

• W3 ARIA APG sortable table

• Deque sortable table

BTW, this is a killer enhancement. You da best 💛

[giamir]

To add to what @dancormier said I think this PR is a great step in the right direction to make our tables more accessible. Thanks for initiating this @allejo! 💛

Here are some things I noticed browsing through the feature:

• This PR is associated to this branch from your forked repository but there is also a branch in this repo which has 3 more commits. I would recommend to close this PR and open a new one based on your branch in this repo so that we can get a correct diff.
• I would treat the sorted column highlighting feature as a completely separate task (keep this PR focused exclusively on a11y fixes). I am pretty sure we should consult our designer as well to get a sign off before being able to introduce that. Figma might need to be updated accordingly too.
• aria-sort rule is not working correctly - it looks like the attribute is changed in all the th elements at once. Probably this is because we update it to all elements without distinguishing which is the one that gets sorted (see here for details)
• Personally I am not a fan of introducing code to replace legacy markup dynamically. I would prefer to invest on a "codemod" instead that can be run at build time once. This will result in less complexity for our codebase and a leaner bundle size. The runtime warning for development bundles is a good idea but as far as my understanding goes we don't ship Stacks dev bundles for our consumers (yet).
• (the following is a bit out of scope but I want to mention it anyway): I feel very uncomfortable modifying algorithms without a solid test suite. I would love to prioritize introducing a unit/integration testing suite in Stacks before building on top of what we have. Here is some PoC work to clarify what I mean.

I believe as a next step someone from the core Stacks team should pick this item up and do a thorough review (potentially even pair with you).

How urgent is this a11y fix for you and your team @allejo?

Thanks again for initiating this! 💛

[allejo]

• This PR is associated to this branch from your forked repository but there is also a branch in this repo which has 3 more commits. I would recommend to close this PR and open a new one based on your branch in this repo so that we can get a correct diff.

Oh man, that's what I get for not noticing where Git is pushing my branches to 😅 I've deleted the branch that I pushed to the Stacks repo and will keep the one in my fork solely because I don't want the conversations going on here to be lost due (or behind an extra link/click)

• I would treat the sorted column highlighting feature as a completely separate task (keep this PR focused exclusively on a11y fixes). I am pretty sure we should consult our designer as well to get a sign off before being able to introduce that. Figma might need to be updated accordingly too.

I definitely agree with getting designers' eyes on this! I have no objections to moving that functionality out to a separate PR.

• aria-sort rule is not working correctly - it looks like the attribute is changed in all the th elements at once. Probably this is because we update it to all elements without distinguishing which is the one that gets sorted (see here for details)

Oops! Fixed 😄

• Personally I am not a fan of introducing code to replace legacy markup dynamically. I would prefer to invest on a "codemod" instead that can be run at build time once. This will result in less complexity for our codebase and a leaner bundle size. The runtime warning for development bundles is a good idea but as far as my understanding goes we don't ship Stacks dev bundles for our consumers (yet).

You have my attention about a "codemod" tool, what are you envisioning? I'm not fond of updating legacy markup at runtime each time but didn't have any better ideas that wouldn't require some more involved dev work. And you're right about not shipping dev bundles... Completely forgot about that; the runtime warning wouldn't even exist in the dist used in pubplat.

• (the following is a bit out of scope but I want to mention it anyway): I feel very uncomfortable modifying algorithms without a solid test suite. I would love to prioritize introducing a unit/integration testing suite in Stacks before building on top of what we have. Here is some PoC work to clarify what I mean.

:100: agree! I tried my best to not touch any of the sorting algorithm logic in this PR. But I would have been a lot more comfortable if I could have tests to lean on to check my work and adding new tests for button actions.

I believe as a next step someone from the core Stacks team should pick this item up and do a thorough review (potentially even pair with you).

I'm cool with this! 😄

How urgent is this a11y fix for you and your team @allejo?

Not urgent at the moment. I was working on redesigning some mod pages (that have already been shipped) that had tables and this a11y bug grabbed my attention so I picked it up.

[giamir]

@allejo I added your PR in our internal team backlog for visibility (you are tagged there too). I would prefer if we could keep this PR just about the a11y fixes though.

It would be helpful if you could move the sorted column highlighting functionality/code in a different PR since it needs additional steps before we could pick it up for development (design review, figma update, etc...).

I additionally converted the PR as draft for now too until it's in a better state to be merged.

Thanks again for your work. 💛

[allejo]

I would prefer if we could keep this PR just about the a11y fixes though.

Of course! I have removed the column highlighting functionality from this PR and moved it over to #1165