Skip to content

Conversation

manuhabitela
Copy link
Collaborator

@manuhabitela manuhabitela commented Jun 16, 2025

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:

  • make all necessary elements keyboard focusable
  • make sure all focusable elements have visible focus rings when focused
  • add text alternative to elements missing them, so that focusing the elements with a screen reader correctly announces what we focus
  • when necessary, add hidden text, like headings, to help screen reader users understand where they are

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.

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here (maybe?)
  • 🙋 no, because I need help

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

@manuhabitela manuhabitela changed the title Improve keyboard navigation on homepage Improve keyboard and screen reader navigation on homepage Jun 16, 2025
@manuhabitela manuhabitela force-pushed the improve-kb-sr-a11y branch 2 times, most recently from 43f3275 to 05f7306 Compare June 16, 2025 15:45
@manuhabitela manuhabitela marked this pull request as ready for review June 16, 2025 15:58
@manuhabitela
Copy link
Collaborator Author

manuhabitela commented Jun 16, 2025

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.

@manuhabitela manuhabitela moved this from In Progress to Needs feedback in French administration Board Jun 16, 2025
@manuhabitela manuhabitela moved this from Needs feedback to Needs Internal Feedback in French administration Board Jun 18, 2025
Copy link
Collaborator

@hexaltation hexaltation left a 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.
Copy link
Collaborator

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.

),
}),
// the now unused `webinarsLinks` is kept to prevent breaking existing translation strings
unstyledH2(t('Learn more {{webinarsLinks}}', {webinarsLinks: ''})),
Copy link
Collaborator Author

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?

Copy link
Collaborator

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. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But maybe @fflorent and @audez have a different point of view as they contribute more than me on french translation.

A lot of the job in other languages seems to be done by AI.

Copy link
Collaborator Author

@manuhabitela manuhabitela Jul 28, 2025

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

@fflorent fflorent moved this from Needs Internal Feedback to Needs feedback in French administration Board Jul 16, 2025
@manuhabitela manuhabitela force-pushed the improve-kb-sr-a11y branch 2 times, most recently from b109022 to 7d03167 Compare July 17, 2025 16:01
@georgegevoian georgegevoian self-requested a review July 17, 2025 17:04
@manuhabitela manuhabitela force-pushed the improve-kb-sr-a11y branch 2 times, most recently from fe4f432 to 8354cb3 Compare July 28, 2025 09:57
@manuhabitela manuhabitela mentioned this pull request Jul 28, 2025
4 tasks
@georgegevoian georgegevoian added the preview Launch preview deployment of this PR label Aug 3, 2025
Copy link
Contributor

github-actions bot commented Aug 3, 2025

Deployed commit 946ceacdfa2a29444d8a5ec2b5d226dcbd427443 as https://grist-manuhabitela-grist-core-improve-kb-sr-a11y.fly.dev (until 2025-09-02T04:49:48.659Z)


const title = `Version ${version.version}` +
((version.gitcommit as string) !== 'unknown' ? ` (${version.gitcommit})` : '');
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name });
const altText = t('{{ - organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woops, good catch

Copy link
Contributor

github-actions bot commented Aug 6, 2025

Deployed commit ed6e9598b8ac22b84eb11c186bb941999d383ed7 as https://grist-manuhabitela-grist-core-improve-kb-sr-a11y.fly.dev (until 2025-09-05T13:21:08.152Z)

- 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
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)
@manuhabitela
Copy link
Collaborator Author

manuhabitela commented Aug 12, 2025

Thanks for reviewing this George! I fixed what you talked about and rebased from a fresh main branch.

The video tour button has some clipping on the top. Giving the element with test ID .test-intro-cards a tiny bit of margin or padding looks like one way to fix it.

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.

The text of the document name does a layout shift when focus is visible due to the padding changing. Wdyt about always giving it a 5px padding, regardless of focus state?

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.

Should the user menu/icon in the top-right corner have a negative outline offset? The top is currently clipped. Part of the bottom is as well. (I think it's the margin/padding of the contents of the main panel. I wonder if nudging it with a tiny bit of margin or padding would help clear it up.)

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 👌

Copy link
Contributor

Deployed commit aa801d5967ebc49071e28bb4cb60f1ace7637880 as https://grist-manuhabitela-grist-core-improve-kb-sr-a11y.fly.dev (until 2025-09-11T09:44:28.564Z)

Copy link
Contributor

@georgegevoian georgegevoian left a 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.

@georgegevoian georgegevoian merged commit 2035c1f into gristlabs:main Aug 12, 2025
25 of 26 checks passed
@github-project-automation github-project-automation bot moved this from Needs feedback to Done in French administration Board Aug 12, 2025
@manuhabitela
Copy link
Collaborator Author

Thanks @georgegevoian 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accessibility gouv.fr preview Launch preview deployment of this PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants