-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Address a11y issues in browser-based console UI #26872
Conversation
CI Results: |
64fc50b
to
2b6fcba
Compare
Build Results: |
There might be some future improvements that could be done to make this more of a lush experience but for the time being, this will provide compliance with WCAG and fix the critical a11y issues with this part of the UI. 👍 |
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 needs a changelog so that users know the rest of the UI will not be interactive when the Web REPL is open. I will follow up on slack with our instructions on how to add a changelog to Vault
fefe1cd
to
95cfa00
Compare
@chelshaw there are still some tests failing, but I can't see what they have to do with any of the changes I made. I might need some expert eyes to help me here. |
@@ -17,16 +17,17 @@ export default Component.extend({ | |||
actions: { | |||
handleKeyUp(event) { | |||
const keyCode = event.keyCode; | |||
const val = event.target.value; |
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.
I changed this to make it more consistent with the rest of the UI code so the implementation matches.
@@ -118,11 +120,11 @@ $console-close-height: 35px; | |||
|
|||
.panel-open .console-ui-panel { | |||
box-shadow: $box-shadow-highest; | |||
min-height: 400px; | |||
min-height: 425px; |
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.
I think eventually we'll want to re-visit this to make it a little more flexible, but right now it needed to be increased to account for the close button.
The close button was positioned in the DOM outside of the container it was then positioned inside of, which was causing the element to overlap. We don't want it to overlap other content.
@@ -10,19 +10,20 @@ | |||
<Chevron /> | |||
{{/if}} | |||
<input | |||
aria-label="command input" | |||
aria-label="web R.E.P.L." |
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.
Adding it as "R.E.P.L." instead of just REPL helps screen readers to read it out correctly.
@@ -54,12 +54,18 @@ export default { | |||
eventProperties: { keyCode: keys.ENTER }, | |||
}), | |||
hasInput: isPresent('[data-test-component="console/command-input"] input'), | |||
runCommands: async function (commands) { | |||
runCommands: async function (commands, shouldToggle = 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.
We needed to add an extra condition here, because there are tests that don't actually need the toggle to happen. Maybe it's rendering the console/ui-panel directly, or maybe it's a test that runs multiple commands and multiple toggles would cause those tests to fail.
But in most cases this is a useful thing to do, so it defaults to true
5a4f9eb
to
0bb73ef
Compare
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.
Looks great! My only concern is about no longer throwing errors on setup. I'm also going to check double check if this is okay to get in now, after feature freeze for 1.17. Thank you for working through all those test failures!
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.
✨ beautiful, thank you!
Added console focus trap Added else conditional for console-panel update aria-label for input reverting height style change reverting focus trap for now fixed typo re-adding focus trap to console::uiPanel component refactor conditional yield of console ui panel remove duplicated option for ui panel move ESC command to focus-trap change focus-trap to use onDeactivate
move toggle to commands helper file move toggle to ui-panel helper file added shouldToggle arg to runcommand fix console tests
remove unnecessary comment
Co-authored-by: Chelsea Shaw <[email protected]>
4291e2d
to
b9d6b0c
Compare
If merged, this PR addresses (at least some of) the accessibility issues outlined in https://hashicorp.atlassian.net/browse/VAULT-26829
Some notes:
oDeactivate
option fromfocus-trap
and pass it thethis.closeConsole
action. It moves the focus back to the toggle button, but it doesn't actually close the console. 🤔 So this is still something to figure out more completely.clickOutsideDeactivates
option to focus trap, that way if a mouse user still wants to use the background and navigate around they can. A keyboard-only user will stay inside of the focus-trap area until they explicitly take an action to leave it or close it.Console::UiPanel
component, it's not enough for height of the parent container to be set to 0; the elements are still in the DOM and therefore violate hiding interactive elements. To account for this, I've updated theframe.hbs
to conditionally render theConsole::UiPanel
component ifthis.console.isOpen
is true.