-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Add placeholder and aria-label to the quick input box #190924
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
Conversation
| if (this.title) { | ||
| ariaLabel += ` - ${this.title}`; | ||
| } | ||
| this.ui.inputBox.setAttribute('aria-label', ariaLabel!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change, and the change in quickInputBox.ts isn't needed because the placeholder will be read by the screen reader, right? You only need the line in the gettingStarted.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder isn't always read by every screen reader. Please see the "Placeholder text" section in https://www.w3.org/WAI/tutorials/forms/instructions/
placeholder text is not a replacement for labels. Assistive technologies, such as screen readers, do not treat placeholder text as labels. Moreover, at the time of writing this tutorial, placeholder text is not broadly supported across assistive technologies and not displayed in older web browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find. Can you instead implement this by following how we set the placeholder:
vscode/src/vs/platform/quickinput/browser/quickInputBox.ts
Lines 74 to 80 in a02490d
| get placeholder() { | |
| return this.findInput.inputBox.inputElement.getAttribute('placeholder') || ''; | |
| } | |
| set placeholder(placeholder: string) { | |
| this.findInput.inputBox.setPlaceHolder(placeholder); | |
| } |
have this call into the QuickInputBox to set the ariaLabel and then have that set the aria label via:
this.findInput.inputBox.setAriaLabel(...)follow the existing handling of placeholder to make sure you handle all the cases.
Oh and if there's no aria label passed in, use the placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a setAriaLabel in quickInputBox.js and used it. I am not sure why you need a get/set arialabel in quickInputBox.js. I am happy to add these if needed.
|
Adding @bhavyaus for Walkthrough words. We should have this quick pick follow our best practices as called out here: I question whether this qp needs a |
| if (this.title) { | ||
| ariaLabel += ` - ${this.title}`; | ||
| } | ||
| this.ui.inputBox.setAriaLabel(ariaLabel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently only running if !ariaLabel but I would expect that the ariaLabel of the inputBox would be set even if ariaLabel was already set.
in other words, I'm expecting this if to be refactored to like:
if (visibilities.inputBox) {
if (!ariaLabel) {
// set it
}
this.ui.inputBox.setAriaLabel(ariaLabel);
}|
Closing as stale for now but if you want to revive this, just let me know! |
This PR fixes issue #190923