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

Prevent error summary from being refocused after it has been initially focused on page load #2491

Merged
merged 3 commits into from
Jan 14, 2022

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Jan 5, 2022

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.

This work is being done as part of #1068 – a future PR will make it possible to disable the autofocus functionality.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2491 January 5, 2022 16:17 Inactive
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.
@36degrees 36degrees force-pushed the error-summary-prevent-refocusing branch from 0177f37 to e1b17e0 Compare January 5, 2022 16:32
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2491 January 5, 2022 16:33 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2491 January 5, 2022 16:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2491 January 10, 2022 17:26 Inactive
@36degrees 36degrees merged commit f609d25 into main Jan 14, 2022
@36degrees 36degrees deleted the error-summary-prevent-refocusing branch January 14, 2022 10:58
@EoinShaughnessy EoinShaughnessy changed the title Prevent error summary from being re-focused after it has been initially focused on page load Prevent error summary from being refocused after it has been initially focused on page load Feb 2, 2022
@owenatgov owenatgov mentioned this pull request Feb 8, 2022
andymantell added a commit to surevine/govuk-react-jsx that referenced this pull request Mar 12, 2022
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.

3 participants