Skip to content

Commit

Permalink
Prevent error summary from being re-focused
Browse files Browse the repository at this point in the history
Always adding tabindex="-1" to the error summary means that as well as being programatically focusable the summary will also take focus if a user clicks within it, which may be confusing as the focus indicator will appear.

Instead, add the tabindex just before focusing the element, and then remove the tabindex attribute as soon as focus is lost, preventing the element from taking focus again afterwards.

This also makes the approach consistent with the notification banner and skip link, which also remove tabindex on blur.

It would make sense for users to remove the tabindex attribute from their own markup as we are now adding it using JavaScript, but the component should work fine either way so I don't believe this is a breaking change.
  • Loading branch information
36degrees committed Jan 5, 2022
1 parent 1deb3a5 commit e1b17e0
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 9 deletions.
19 changes: 18 additions & 1 deletion src/govuk/components/error-summary/error-summary.js
Expand Up @@ -11,11 +11,28 @@ ErrorSummary.prototype.init = function () {
if (!$module) {
return
}
$module.focus()

this.setFocus()
$module.addEventListener('click', this.handleClick.bind(this))
}

/**
* Focus the error summary
*/
ErrorSummary.prototype.setFocus = function () {
var $module = this.$module

// Set tabindex to -1 to make the element programmatically focusable, but
// remove it on blur as the error summary doesn't need to be focused again.
$module.setAttribute('tabindex', '-1')

$module.addEventListener('blur', function () {
$module.removeAttribute('tabindex')
})

$module.focus()
}

/**
* Click event handler
*
Expand Down
9 changes: 9 additions & 0 deletions src/govuk/components/error-summary/error-summary.test.js
Expand Up @@ -13,6 +13,15 @@ describe('Error Summary', () => {
expect(moduleName).toBe('govuk-error-summary')
})

it('removes the tabindex attribute on blur', async () => {
await page.goto(baseUrl + '/components/error-summary/preview', { waitUntil: 'load' })

await page.$eval('.govuk-error-summary', el => el.blur())

const tabindex = await page.$eval('.govuk-error-summary', el => el.getAttribute('tabindex'))
expect(tabindex).toBeNull()
})

const inputTypes = [
// [description, input id, selector for label or legend]
['an input', 'input', 'label[for="input"]'],
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/error-summary/template.njk
@@ -1,5 +1,5 @@
<div class="govuk-error-summary
{%- if params.classes %} {{ params.classes }}{% endif %}" aria-labelledby="error-summary-title" role="alert" tabindex="-1"
{%- if params.classes %} {{ params.classes }}{% endif %}" aria-labelledby="error-summary-title" role="alert"
{%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %} data-module="govuk-error-summary">
<h2 class="govuk-error-summary__title" id="error-summary-title">
{{ params.titleHtml | safe if params.titleHtml else params.titleText }}
Expand Down
7 changes: 0 additions & 7 deletions src/govuk/components/error-summary/template.test.js
Expand Up @@ -32,13 +32,6 @@ describe('Error-summary', () => {
expect(roleAttr).toEqual('alert')
})

it('has the correct tabindex attribute to be focussed', () => {
const $ = render('error-summary', examples.default)
const tabindexAttr = $('.govuk-error-summary').attr('tabindex')

expect(tabindexAttr).toEqual('-1')
})

it('renders title text', () => {
const $ = render('error-summary', examples.default)
const summaryTitle = $('.govuk-error-summary__title').text().trim()
Expand Down

0 comments on commit e1b17e0

Please sign in to comment.