-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Minor changes to make CodeMirror accessible (label and color-contrast) #6920
Conversation
addon/search/search.js
Outdated
el("span", {className: "CodeMirror-search-label"}, cm.phrase("Search:")), " ", | ||
el("input", {type: "text", "style": "width: 10em", className: "CodeMirror-search-field"}), " ", | ||
label, " ", | ||
el("input", {type: "text", "style": "width: 10em", className: "CodeMirror-search-field", id: "CodeMirror-search-field"}), " ", |
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.
Using a fixed ID here will break if there are multiple editors in the page. Maybe just wrap the input in the label instead?
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.
Done
src/input/input.js
Outdated
@@ -131,5 +131,6 @@ export function hiddenTextarea() { | |||
// If border: 0; -- iOS fails to open keyboard (issue #1287) | |||
if (ios) te.style.border = "1px solid black" | |||
disableBrowserMagic(te) | |||
te.setAttribute("aria-hidden","true") |
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.
Hiding the input element from the screen reader seems a very bad idea. What is the purpose of this line?
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.
Sorry, I probably misunderstood what is this textarea for. Maybe it would be good to just add aria-label for this textarea?
If so, what should be in the aria-label?
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 creates the main focused element when inputMode
is "textarea"
. Its label can be set with this option.
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.
Ok, I reverted this change and set the label locally. That's all for this pull request
…eet color-contrast requirements
Minor change made for the project to be a bit more accessible ('label' violation)