Improvement/lighthouse accessibility changes - #2295
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2295 +/- ##
==========================================
+ Coverage 43.18% 43.54% +0.36%
==========================================
Files 601 602 +1
Lines 25741 25806 +65
Branches 7666 8592 +926
==========================================
+ Hits 11116 11238 +122
+ Misses 14576 14511 -65
- Partials 49 57 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-accessibility-changes [VRT] Update baselines for improvement/lighthouse-accessibility-changes
There was a problem hiding this comment.
I think you've done a good job here, but unfortunately they were both a lot of things to find and some subtleties that need some more knowledge of accessibility/our codebase.
If you've got any questions about the changes I'm requesting, please do feel free to drop a message and ask 😊
Quick list of individual violations I found on the reviewed pages when re-running lighthouse tests. I think it makes sense to also bundle fixes for them here:
- This question finder button on <=md screens (Note that the
<i>within it isaria-hidden, but the button itself should still be labelled) - Theses h4s for IsaacReorderQuestion (and the equivalent ones for IsaacParsonsQuestion)
- The title tag for modal headers (although be careful here because
undefinedsets them to<h5>. We need to set a non-<h*>tag manually and then ensure that.modal-titlehas correct styling by itself on both sites) - All the
<h*>s in related questions, including the fact that there are<h4>as direct children of a<ul>(<ListGroup>). Only<li>should be children here, so we should extract those. - Topic headers
- Assignment card titles
There are also definitely a lot more issues with most other <h*> uses in the codebase, but not in the pages that were tested for the report - so I'm collating those for another card.
Also have you intentionally not addressed "The user's focus is directed to new content added to the page - when questions are listed, focus remains on the "Apply filters" button"? If so that's fine, I'd just like to make sure its recorded somewhere.
| return <MyAccountTab | ||
| leftColumn={<> | ||
| <h3>Account details</h3> | ||
| <h2>Account details</h2> |
There was a problem hiding this comment.
This make sense logically to be <h2>, but visually it looks off - especially on Isaac where this makes this heading bigger than the actual <h1> page heading.
I think we can keep the rank logic, but also maintain the styling. i.e.
| <h2>Account details</h2> | |
| <h2 className="h3">Account details</h2> |
Also, Lighthouse doesn't catch this because it only does an initial page load, but this <h3> is an issue for every tab on the My Account page. In reality we care beyond just the page load, so they should all have the same treatment as this.
| {/* the key above ensures that any state inside tabs is reset to initial values if activeTab is changed while accountInfo is dirty | ||
| (i.e. user has unsaved changes they do *not* want to commit) */} | ||
| <TabPane tabId={ACCOUNT_TAB.account}> | ||
| <TabPane tabId={ACCOUNT_TAB.account}{...tabPanelProps(ACCOUNT_TAB.account)}> |
There was a problem hiding this comment.
I've got a couple suggestions about these props:
- There's slight redundancy, but I'd rather we keep these props actually local to their object, so one doesn't need to scroll back and forth to find out values when maintaining this code.
- Since it's an enum value, just using the
ACCOUNT_TABdirectly is making the id contain a number rather than title, which isn't very descriptive. We'd rather it be intuitive which id belongs to which object. - Not here, but it would be nice if the
NavLinkhas anaria-controlsproperty linking to this id to make the connection two-way.
Something like this should do to cover the first two of these points for this pane specifically, but there are changes to make in several places if you want to implement this.
| <TabPane tabId={ACCOUNT_TAB.account}{...tabPanelProps(ACCOUNT_TAB.account)}> | |
| <TabPane | |
| tabId={ACCOUNT_TAB.account} id={`account-panel-${ACCOUNT_TABS[ACCOUNT_TAB.account].title.toLowerCase()}`} | |
| {...(isAda && {"role": "tabpanel", "aria-labelledby": `account-tab-${ACCOUNT_TABS[ACCOUNT_TAB.account].title.toLowerCase()}`})} | |
| > |
|
I have started implementing the changes you requested, which I agree with all of them. I knew about the div being empty and still added the aria-hidden since the lighthouse report was complaining about that, but if you think it's being overly twitchy on that, then we can undo that change. I've tried doing the account tab stuff, and I made it so it has a slug function, and replaces the space with "-" since if a tab contained more than a single word, the space would break the tag. I went through all the user tabs and tried to fix the headings, I think I got everything. I am starting the work on the other headings you mentioned, I haven't done anything with them. And for the final thing, the about the user focus, I haven't done that, it must have slipped my checks, I will look into that as well. |
4 participants
This pull request covers most of the accessibility issues raised by @jacbn with Lighthouse on the 3rd of August.
Most of the changes are small, h3 -> span/div, adding aria-labels, etc.
The big change is in the MyAccount file, where a few helper functions have been added in order to make the tab selection accessible:
This is following the standard at MDN.