Set focus to skip link target to improve screen reader announcements

[hannalaakso]

When user activates the skip link, Mac VoiceOver doesn't announce the main content or continue reading
from it as reported in #2187.

To improve the experience for Mac VoiceOver users:

• make the main content element focusable by adding a tabindex attribute
• move focus to it programmatically
• override the native focus outline to none whilst tabindex is present
• remove the tabindex attribute and the style override on blur

This follows the pattern we already use in the error summary and notification banner components, and follows the approach recommended by @selfthinker.

Results from screen reader testing

This solves the issue in Mac VoiceOver: when the user activates the skip link, VoiceOver now announces the main element. If the user then tries to read the next element, VoiceOver now correctly continues reading from the main content. There is also a smaller improvement to the announcements on JAWS (both with Chrome and IE11): JAWS now announces it's on the main region, before it starts reading the main content.

Behaviour	Announces skip link when it's navigated to	Announces main has been navigated to	Reads out content of main
JAWS 2020 / Chrome	same page link skip to main content	enter main region main region page has 4 regions and 8 headings <main content>	enter main region main region page has 4 regions and 8 headings <main content>
JAWS 2020 / IE 11	skip to main content same page link	enter main region page has 4 regions and 8 headings <main content>	enter main region page has 4 regions and 8 headings <main content>
NVDA 2021.1/ Firefox 90	skip to main content link	main landmark <main content>	main landmark <main content>
Voiceover / Safari (macOS Mojave)	skip to main content, link	main you are currently on a text area	<main content>
Voiceover / Safari (iOS 15)	Skip to main content, in-page link	After double-tapping to follow link: "Press release, main landmark"	After double-tapping to follow link: "Press release, main landmark"
Talkback / Chrome 96 (Android 11)	skip to main content, link	After double-tapping to follow link: "main, Press release, "	After double-tapping to follow link: "main, Press release, "

(Announcements that have changed from the live version are in bold ^)

See #2187 (comment) for more details.

Closes #2187

[36degrees]

I think this will set up a new event listener every time the user uses the skip link, so you'll end up with multiple event listeners bound that all do the same thing.

I think if it was a named function (for example, if it was a separate function in the module) it would only ever be added once.

An alternative approach would be for it to unbind itself?

Suggested change

	$content.addEventListener('blur', function () {
	$content.removeAttribute('tabindex')
	$content.classList.remove('govuk-skip-link-focused-element')
	})
	var handler = $content.addEventListener('blur', function () {
	$content.removeAttribute('tabindex')
	$content.classList.remove('govuk-skip-link-focused-element')
	$content.removeEventListener('blur', handler)
	})

[hannalaakso]

I don't think this is true, it's valid to use an anonymous function with an event listener: https://developer.mozilla.org/en-US/docs/Web/API/EventListener#javascript I don't think a named function would change the actual behaviour in any way. But I'm going to leave this open, we can compare notes on Monday 👀

[hannalaakso]

My bad, I've had another look and this was actually a good catch 💯 I'm now binding the function on the event listener which stops the event listener being created more than once.

[hannalaakso]

This still isn't working as expected - I want to refactor the code so that we set the event listener at the point of initialisation but going to pick this up later so that we can get the actual change to the component behaviour tested as part of the pre-release.

[36degrees]

To make this easier to follow, what about making the variable names consistent with the method name and refer to the thing we want to focus as the target, rather than content?

Suggested change

	var contentId = this.getFragmentFromUrl($target.href)
	var $content = document.getElementById(contentId)
	if (!$content) {
	return false
	}
	if (!$content.getAttribute('tabindex')) {
	var targetId = this.getFragmentFromUrl($target.href)
	var $target = document.getElementById(contentId)
	if (!$target) {
	return false
	}
	if (!$target.getAttribute('tabindex')) {

etc…

[hannalaakso]

I had tried to find a better name for the variable but we're constrained by the existing naming conventions here. $target is already defined, it contains the skip link element. The function duplicates logic in the error summary (ideally we should be extracting this to be a reusable function but as just discussed in a different context doing that will be part a bigger piece of work) so I was loath to rename the function or its parameter in order to keep that linkage.

In the error summary we call the elements to focus by the HMTL element name ($input and $legendOrLabel) but $main in this context felt too specific, so that's why I settled on $content.

[36degrees]

That's a good point – but I think points to a bigger naming issue (which is absolutely already present in the error summary) and which is potentially even more confusing… target means two different things in two different parts of the code. Within the context of handleClick target refers to the 'event target' (the thing that was clicked), but focusTarget at least suggests that the target is the element that we want to focus.

Base on the function's signature (focusTarget($target)) you would definitely expect it to focus whatever $target is. And that's also what the doc block says it does! But it doesn't do that – it focuses whatever $target links to. The target of the $target, I guess…?

I think this is worth trying to fix rather than introducing the issue to another component.

[hannalaakso]

I've amended the wording, just for this component for now, to distinguish the page content that the skiplink targets from the event target of the skiplink. Have a look and let me know if you think this makes the distinction clearer.

[EoinShaughnessy]

@hannalaakso Changelog update is great! Just suggesting one word-change, but otherwise this looks good to go. 👍🏻

[vanitabarrett]

This PR looks really good, thanks @hannalaakso !

As discussed in dev catchup, this PR is good apart from one issue Ollie noticed where the event listener is added each time a user interacts with the skip link. It feels unlikely that this would happen or cause many problems, but it's something we should fix.

However, we want to include this change in the pre-release so I'm going to approve it for now so it can be merged. We'll revisit this to fix the multiple event listeners issue before the actual v4.0.0 release.