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