Skip to content
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

Fix hidden external link text warning so it can be read by screen readers #56

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Oct 12, 2024

Brief overview of changes

Updated the CSS for the hidden warning in external_link().

Why are these changes being made?

Honestly? I never tested it properly in the first place (copying in @rmbielby and @jen-machin as past reviewers for sight too).

I noticed something at a conference this week about the display: none CSS value, which actually hides content from screen readers and remembered I'd made the CSS for the external_link() function on the assumption it only visually hid content but still gave that to screen readers to read out.

As a result I've now gone back and tested it, realised my mistakes, and found a new suggested sr-only CSS class from WebAIM.

Detailed description of changes

  1. Swap old visually-hidden CSS for the sr-only CSS suggested by WebAIM
  2. Change the placement of the warning to mirror the visual warning more

I realised when testing this with a screen reader that links are automatically announced as a link, so having Link. This link opens in a new tab. R Shiny. felt pretty clunky! That example now reads as Link. R Shiny (opens in a new tab). to screen readers, which mirrors how screen readers read the main visual version of the warning too.

  1. Updated test expectations to match the new behaviours
  2. Removed www from .gitignore, as actually we do want to Git control any custom CSS like we have here
  3. Bumped the version up to 0.4.3 as a patch as this is a bug fix and added notes to the news.md file

Additional information for reviewers

Windows has narrator built in, NVDA also has a free download to use, these are probably the easiest screen reader options for testing this.

Issue ticket number/s and link

No related ticket as I raised the PR as soon as I realised.

@cjrace cjrace marked this pull request as draft October 12, 2024 12:03
@cjrace cjrace self-assigned this Oct 12, 2024
@cjrace cjrace added bug Something isn't working accessibility Issues that specific cause accessibility problems on the server labels Oct 12, 2024
@cjrace cjrace marked this pull request as ready for review October 12, 2024 12:41
@jen-machin jen-machin self-requested a review October 16, 2024 09:45
Copy link
Contributor

@jen-machin jen-machin left a comment

Choose a reason for hiding this comment

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

Tested with Narrator and despite some odd behaviour locally, it works as expected when opened in the browser.

Can't see any evidence of the following issue happening either: https://medium.com/@jessebeach/beware-smushed-off-screen-accessible-text-5952a4c2cbfe

@cjrace cjrace merged commit 546f4c1 into main Oct 16, 2024
12 checks passed
@cjrace cjrace deleted the fix/hidden-link-text-screen-reader branch October 16, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Issues that specific cause accessibility problems on the server bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants