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

Accessibility fixes #3283

Merged
merged 5 commits into from
Feb 20, 2024
Merged

Accessibility fixes #3283

merged 5 commits into from
Feb 20, 2024

Conversation

JamesCGDS
Copy link
Contributor

@JamesCGDS JamesCGDS commented Feb 16, 2024

What

This PR fixes a bug where focus is lost on the "Sort by" select element after using it to sort search results. This is because the DOM is updated with new elements when a user uses the "Sort by" filter. This includes inserting an entirely new "Sort by" select element and removing the previous select element which had received focus. I have applied a fix which retains the original select element and only updates the option elements within, therefore retaining the focus on the select element after it's been used.

This is achieved by using a regex on the new stringified HTML (results.sort_options_markup) that is received when updateSortOptions() is run following the application of filters to extract the option elements. The new option elements then replace the existing option elements in the "Sort by" select element.

Why

Identified as an issue as part of the WCAG audit of GOV.UK - trello card

Visual changes

None.

@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3283 February 16, 2024 12:12 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3283 February 16, 2024 14:51 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3283 February 16, 2024 14:55 Inactive
@JamesCGDS JamesCGDS marked this pull request as ready for review February 16, 2024 15:04
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Nice work, though it doesn't seem to be working in IE11

app/assets/javascripts/live_search.js Outdated Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3283 February 19, 2024 13:07 Inactive
@JamesCGDS
Copy link
Contributor Author

Thanks for the review @AshGDS - should be ready for a re-review now

Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Looks good - just a couple more things to think about 👍

app/assets/javascripts/live_search.js Outdated Show resolved Hide resolved
app/assets/javascripts/live_search.js Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3283 February 19, 2024 15:08 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@JamesCGDS JamesCGDS merged commit d21177a into main Feb 20, 2024
14 checks passed
@JamesCGDS JamesCGDS deleted the accessibility_fixes branch February 20, 2024 10:14
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