-
-
Notifications
You must be signed in to change notification settings - Fork 478
Improve keyboard and screen reader navigation on homepage #1667
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
Improve keyboard and screen reader navigation on homepage #1667
Conversation
43f3275
to
05f7306
Compare
Note that, even if this will have impact only after the bigger keyboard-related PR is merged, this can already be merged as is. It doesn't break anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/** | ||
* Helpers to create unstyled variants of various HTML elements that have default styles. | ||
* | ||
* Useful to easily build semantic content without having to deal with removing styles all the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this one. <3
So useful.
app/client/ui/HomeIntroCards.ts
Outdated
), | ||
}), | ||
// the now unused `webinarsLinks` is kept to prevent breaking existing translation strings | ||
unstyledH2(t('Learn more {{webinarsLinks}}', {webinarsLinks: ''})), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not the best strategy but I chose to keep existing translations with an now obsolete param in it, rather than creating a new translation string. In order to prevent extra work on translations for no real value (except having clean, better self-explanatory code). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer a clean, better self-explanatory code. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to have a new clean translation string and note for later that I'll change the translations myself to prevent extra work on people usually translating things.
edit: done
b109022
to
7d03167
Compare
fe4f432
to
8354cb3
Compare
8354cb3
to
ad4a67d
Compare
Deployed commit |
|
||
const title = `Version ${version.version}` + | ||
((version.gitcommit as string) !== 'unknown' ? ` (${version.gitcommit})` : ''); | ||
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); | |
const altText = t('{{ - organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops, good catch
946ceac
to
ed6e959
Compare
Deployed commit |
- the grist version on hover has been moved to the grist icon in the footer. I'd say this rather hidden feature didn't have much value placed in the app header - now the app header home link correctly sets up an alt text for screen reader users, so that they know the link goes back to the current org homepage
Add a new unstyledButton css helper to output html `<button>` that do not have any style. It allows us to easily replace clickable-but-not kb-focusable divs with actual buttons, with basically no work to do on the visuals, since those reset buttons kinda look like divs by default. The intent behind using actual html button tags is that they are keyboard-friendly by default, making us not having to specifically code keyboard-specific stuff so that they work correctly.
the "…" menu of each workspace was only attainable with the mouse on hover. Now we correctly show the dots also on keyboard focus ; and make sure they can be focused themselves (by using a button tag). we also add an alt text on the dots menu to help screen reader users
we will certainly want to create unstyled `<a>`, `<ul>`, etc. Just keep those in the same file
one way to navigate with a screen reader is by going from html region to html region. Most common region elements are header, nav, main, footer. This adds a few region elements on specific parts of the app. This also changes the role of some of our Region (our way of calling the main app sections that we can cycle through with kb). The main region is now a correct main role, and the top panel is now a header.
a `<a>` was used without href, making it non keybaord focusable. we shouldn't use a `<a>` here anyway, as this acts as a button. using a button tag makes it kb accessible.
the better way to resolve this would be to have only one "help center" link instead of two next to each other, but i'm not sure of all the implications (mobile view, customization, etc). So just add an alt text for the icon-only link.
this makes undo/redo/notifications/share buttons focusable
and add some alt text for screen readers
use actual heading elements (or role=heading elements, same result)
- add a hidden heading so that SR users can quickly go to the documents list - make sure the "no docs" image is ignored by SRs - make sure the headings row of docs (name/workspace/edited) is ignored by SRs, it's no use on its own - use ul+li tags on the doc list, that makes SR users able to quickly navigate around it
- using one big `<a>` tag wrapping the whole row is not that great for SR users as it gets pretty difficult to understand each part of the row. now the link is actually only on the doc name, but the whole row is still clickable thanks to a "stretchableLink" helper (taken from bootstrap) - adding some hidden "workspace" text before each workspace to help SR users understand context - hiding the doc icon from SR users as its purely decorative - make the document dots button targettable through keyboard + understandable for SR users
in a recent previous commit we made it possible to use keyboard to target the "…" menu of each workspace in the left panel. this was great but didn't really take care of screen reader users. The `<a>` wrapping the `<button>` caused issues. The html structure was reworked so that the whole row is not an html `<a>` tag anymore. Now the row is a div, containing a `<a>` (link to workspace), and a `<button>` (dots menu) next to each other. The `<a>` tag is still clickable on the whole row thanks to "stretched link" css.
this has the downside of generating new translation strings for the same things but at least the code is better : i'll update translation strings myself later in the translation app
- i'd say describing what the button actually is first, and then its context, is a better order
thanks George!
with the few layout changes on doc list to improve keyboard navigation, I broke the mobile layout by forgetting to add an `href` on the mobile-specific doc names.
before, we added padding only when focusing items to easily have clear focus rings without impacting too much code/layout. that was not such a great idea, we'd rather have layout not shift at all. For that, we add padding on the doc list name (anchors) and refine margins around them (gaps/line right margin). Make sure to handle the mobile specific doc name anchors also.
on the homepage, the doc list header is sticky positioned, making the normal main panel focus ring go below it. Handle that specific case by setting a top border on the sticky doc header div when the main panel is focused.
the focus ring is a tiny bit better visually now that it doesn't try to render out of the topbar (because the topbar hides overflowing stuff)
ed6e959
to
aa801d5
Compare
Thanks for reviewing this George! I fixed what you talked about and rebased from a fresh main branch.
You are right, I didn't want to change existing spacing much and figured a bit of clipping when using keyboard was better than moving the whole line for that. But for a 4 pixels change you are right my reasoning was a bit much :') I updated that.
Here I had the same logic as above. But now that you are talking about it I saw how to keep spacings intact while preventing layout shifts. I think I just wasn't thinking straight at the time 😅 Thanks and sorry. Going back on this, I also noticed I had introduced a worrying bug: the items of the list weren't clickable anymore in mobile view 👀 this is also fixed now, I didn't catch that the mobile view had a specific rendering for the list items.
Oh good catch, I have to say I didn't even pay attention to that since it's stuck at the border of the frame, I didn't notice. Just adding negative offset is good, thanks. As a sidenote, I also took care of the main panel focus ring (when using Ctrl+O) being hidden by the sticky-positioned doc header div. Before that the the focused main panel outline wasn't perfect, with the top border being hidden by the document header. I had to handle that in a rather specific way because of the sticky position, and now it's perfect 👌 |
Deployed commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks @manuhabitela.
Thanks @georgegevoian 🎉 |
Context
When full keyboard navigation will be possible, we'll want to target every link and button with keyboard only. Also, we'll want to understand everything if using a screen reader.
As for now, even when working on a grist version with the keyboard navigation PR code applied, some interactive elements here and there can't be targeted with keyboard. And some elements also don't have any text tied to them (some icon buttons, for example), making it pretty hard to understand things with a screen reader.
edit: the full keyboard navigation PR has been merged, so you can already see on the main branch the issues I talk about.
Proposed solution
This is a first pass to handle most cases I stumbled upon when on the grist homepage, listing our documents.
The whole idea is to:
Some things are not perfect (yet), but it's a good first pass. Every change should be pretty understandable, on its own commit.
note that, if you want to test the behavior right now, you should rebase this on my keyboard branch, as it is not merged yet and it's the code allowing normal browser tab navigation on the homepage.Has this been tested?
I didn't add any tests and wonder if I should right now ; those are lots of small technical changes. Maybe the best way to move forward would be to move along (updating current tests if they break). I'd like to, in parallel, try to include https://github.com/IBMa/equal-access in the tests suite to handle accessibility tests in a more global way.
Screenshots / Screencasts
Before this PR, (rebased on the keyboard PR branch):
keyboard-home-before.mp4
After this PR, (rebased on the keyboard PR branch):
keyboard-home-after.mp4