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

Ensure dashes not read in screenreaders #3122

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

beccapearce
Copy link
Contributor

@beccapearce beccapearce commented Mar 12, 2024

Nearly every screen reader (except NVDA of the major ones) reads out the “en dash“ (VoiceOver on macOS says “space“) before each item/link in the “Page contents“ list. Screen reader users found that frustrating to read through.

To solve this we have used a version of the contents list in the publishing components which fixes this issue. This has also meant that we have removed some of the custom elements that were applying to the contents list in favour of the standard govuk-publishing-components. This means that there is a change in the appearance of the contents due to this.

Before:

contents_list_old.mov

After:

Screen.Recording.2024-04-05.at.13.18.54.mov

Trello card: https://trello.com/c/prtvfJV5

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 March 12, 2024 16:02 Inactive
@beccapearce beccapearce force-pushed the ensure-dashes-not-read-in-screenreaders branch from de66c82 to 184adea Compare March 12, 2024 16:05
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 March 12, 2024 16:05 Inactive
@beccapearce beccapearce force-pushed the ensure-dashes-not-read-in-screenreaders branch from 184adea to e4b7abb Compare March 13, 2024 12:57
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 March 13, 2024 12:57 Inactive
@beccapearce beccapearce marked this pull request as ready for review March 13, 2024 15:36
@@ -8,7 +8,7 @@ def body

def header_links
header_links = details.fetch("header_links", {})
Array(header_links).map { |h| ActiveSupport::HashWithIndifferentAccess.new(h) }
Array(header_links).map { |h| ActiveSupport::HashWithIndifferentAccess.new(h.transform_keys { |k| k == "title" ? "text" : k }) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is the best way of doing this, very open to suggestions that make it clearer, or I can add some comments

@davidtrussler
Copy link
Contributor

This looks great to me. 👍 Nice to see a bunch of stuff removed as part of the work.
The only thing that's slightly concerning is if the tracking is correct. I see that it is now using the tracking built into the component but it seems to give a very different result to the existing links. Saying that I don't really know what I'm looking at with tracking so it might be fine. 🤷

@beccapearce
Copy link
Contributor Author

Yes! Martin and I took a look at the tracking and we think it looked ok when we tested it on integration

@davidtrussler
Copy link
Contributor

Yes! Martin and I took a look at the tracking and we think it looked ok when we tested it on integration

OK, I suspect that's just a function of using the component then.

The other thing (sorry!) is that it does create a fairly different experience for the user: where it used to retain the list of links in view and scroll the content to it, we now have standard skip links. Should we (or maybe we have) consult with someone on the design side about this? Maybe we'd need to introduce some kind of "back to top" mechanism or some-such. I tend to think that using the component makes for a more consistent experience across all parts of the site but it's quite a big change. (I'm sure we can live with the change from en dash to em dash)

@beccapearce
Copy link
Contributor Author

AH no I don't think we have checked with design, I can do that!

@beccapearce beccapearce force-pushed the ensure-dashes-not-read-in-screenreaders branch from e4b7abb to a4557fc Compare March 14, 2024 09:00
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 March 14, 2024 09:00 Inactive
@MartinJJones
Copy link
Contributor

MartinJJones commented Mar 14, 2024

@beccapearce Nice work on this and removing the bespoke CSS and JavaScript.

I had a further look at the GA events firing after switching to the contents-list component, it looks like we need to test the changes on Integration as the GA4 event do not appear to fire on Heroku.

When comparing the difference for the select_content event, the only event data that changes is for the section as shown below, this looks good to me as it matches the new content.

- Event Data (section): Page contents:
+ Event Data (section): Contents

The only other change that I could see relating to Google Analytics events, is that using the contents-list component will also fire an "Event" for Universal Analytics as well.

@andysellick @AshGDS sorry to tag you in directly, just want to double-check my understanding here and if there is anything I may be overlooking relating to Google Analytics with this change? thanks

@andysellick
Copy link
Contributor

Thanks for asking about this @MartinJJones.

I had a further look at the GA events firing after switching to the contents-list component, it looks like we need to test the changes on Integration as the GA4 event do not appear to fire on Heroku.

Using the contents list component means that tracking will happen automatically (it's build into the component). You're right, the GA4 stuff probably won't fire on Heroku so checking it on integration is a good idea. If you type window.dataLayer into the console it'll show you events that have already fired, or window.GOVUK.analyticsGa4.showDebug = true to see events as they happen.

The only other change that I could see relating to Google Analytics events, is that using the contents-list component will also fire an "Event" for Universal Analytics as well.

Realistically Universal Analytics is going to stop working at the end of June anyway so it's probably not something to be overly concerned about.

@MartinJJones
Copy link
Contributor

Thanks for the further info @andysellick 👍

When the contents list on service manual guides was being read out by
screen readers it would read the dashes in the contents list as "en dash"
This was not desired behaviour and has been fixed in the
govuk-publishing-components. This uses the fixed publishing component to
take advantage of the fixed behaviour.

Using the new publishing component also makes the page more consistent
with other pages within the app.
In the previous commit we changed the contents list on the service service_manual_guide
page to use the publishing component. This means that we are now able to
delete the custom styles and javascript that were being used previously.
@beccapearce beccapearce force-pushed the ensure-dashes-not-read-in-screenreaders branch from a4557fc to 271654f Compare April 5, 2024 12:05
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 April 5, 2024 12:05 Inactive
Copy link
Contributor

@davidtrussler davidtrussler left a comment

Choose a reason for hiding this comment

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

Nice! Sorts out previous concerns with user experience elegantly.
All looks good to me 👍

Add a margin to the top of the contents list on service manual guides so
it isn't too high up the page when scrolling
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3122 April 5, 2024 14:02 Inactive
@beccapearce
Copy link
Contributor Author

While completing this work, we noticed that the functionality in the sticky module is similar to that used by html publications' content lists Ideally we could combine the two into one place to tidy up the code and reduce duplication.

We should complete this work as a separate PR since it may cause some changes that should be discussed separately.

@beccapearce beccapearce merged commit 109a708 into main Apr 8, 2024
14 checks passed
@beccapearce beccapearce deleted the ensure-dashes-not-read-in-screenreaders branch April 8, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants