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

Add aria-label 'Search' to search input when dropdown opens. #5824

Merged
merged 3 commits into from Jan 5, 2021

Conversation

adityatoshniwal
Copy link
Contributor

@adityatoshniwal adityatoshniwal commented Apr 6, 2020

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made:
I've added an aria-label to the search box when dropdown opens. I've added a translation function for the label so that it can be set as per the language.

@kevin-brown
Copy link
Member

I'm going to evaluate the need for this soon as we are making some accessibility changes in the upcoming 4.1.0 release which would be a solid time for this pull request. Unfortunately it looks like this pull request includes changes to the dist directory which need to be reverted. We regenerate the dist periodically and including changes to that directory in pull requests causes the directory to quickly become out of sync with any code changes.

@adityatoshniwal
Copy link
Contributor Author

Got it. Generally, other packages required the dist files to be included in the pull.
I'll make the changes.

@adityatoshniwal
Copy link
Contributor Author

Oh wait. This got closed somehow. I messed up my fork and did hard reset it to this branch. I've committed the changes.

@kevin-brown
Copy link
Member

I'm pretty sure we need this for the dropdown search, and for consistency it probably makes sense to have it for the selection search as well (when the dropdown is open). The main changes that need to happen are:

  • Set the attribute using setAttribute or jQuery.attr() (depending on context) instead of injecting it using string concatenation
    • This avoids potential XSS issues further down the line and is in line with how we do other translation injection
  • Unit tests that ensure the attribute is being set properly in the right cases

@adityatoshniwal
Copy link
Contributor Author

Hey @kevin-brown, I've made the changes as mentioned. I've also added the test cases.
Please review.

@kevin-brown kevin-brown merged commit 83f288d into select2:develop Jan 5, 2021
anttikuuskoski pushed a commit to anttikuuskoski/select2 that referenced this pull request Mar 29, 2022
…#5824)

* Add aria-label 'Search' to search input when dropdown opens.

* Fixex per review comments. Added test cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants