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

Address a11y issues in browser-based console UI #26872

Merged
merged 18 commits into from
May 16, 2024
Merged

Conversation

MelSumner
Copy link
Contributor

@MelSumner MelSumner commented May 7, 2024

If merged, this PR addresses (at least some of) the accessibility issues outlined in https://hashicorp.atlassian.net/browse/VAULT-26829

  • Associate help text with input
  • Update minimize/maximize window message
  • remove tooltip on maximize button
  • Hide decorative glyphs from assistive tech
  • add support for ESC key
  • add focus trap
  • update console so it conditionally renders if it is opened
  • update tests that fail due to previous lack of conditional rendering

Some notes:

  1. The ESC key is not really working as fully intended right now. Trying to use the oDeactivate option from focus-trap and pass it the this.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.
  2. Added the 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.
  3. Because there are interactive elements in the 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 the frame.hbs to conditionally render the Console::UiPanel component if this.console.isOpen is true.
  4. ⚠️ Due to this conditional render change, there are failing tests that attempt to simply target the input and test a command. They need to be updated to toggle the console button first, and then target the input. There are some tests that do this correctly, and these pass.

@MelSumner MelSumner added the a11y label May 7, 2024
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label May 7, 2024
Copy link

github-actions bot commented May 7, 2024

CI Results:
All Go tests succeeded! ✅

@hashishaw hashishaw added the ui label May 8, 2024
@MelSumner MelSumner marked this pull request as ready for review May 8, 2024 17:20
@MelSumner MelSumner requested a review from a team as a code owner May 8, 2024 17:20
@MelSumner MelSumner marked this pull request as draft May 8, 2024 17:21
Copy link

github-actions bot commented May 8, 2024

Build Results:
All builds succeeded! ✅

@MelSumner MelSumner marked this pull request as ready for review May 8, 2024 17:29
@MelSumner
Copy link
Contributor Author

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. 👍

Copy link
Collaborator

@hashishaw hashishaw left a 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

ui/app/templates/components/console/command-input.hbs Outdated Show resolved Hide resolved
ui/app/styles/components/console-ui-panel.scss Outdated Show resolved Hide resolved
@MelSumner MelSumner force-pushed the melsumner/VAULT-26829 branch 3 times, most recently from fefe1cd to 95cfa00 Compare May 13, 2024 22:24
@MelSumner MelSumner requested a review from hashishaw May 13, 2024 22:24
@MelSumner
Copy link
Contributor Author

@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.

changelog/26872.txt Outdated Show resolved Hide resolved
@@ -17,16 +17,17 @@ export default Component.extend({
actions: {
handleKeyUp(event) {
const keyCode = event.keyCode;
const val = event.target.value;
Copy link
Contributor Author

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;
Copy link
Contributor Author

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."
Copy link
Contributor Author

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) {
Copy link
Contributor Author

@MelSumner MelSumner May 13, 2024

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

Copy link
Collaborator

@hashishaw hashishaw left a 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!

changelog/26872.txt Outdated Show resolved Hide resolved
ui/tests/acceptance/oidc-provider-test.js Outdated Show resolved Hide resolved
@hashishaw hashishaw added this to the 1.17.0-rc milestone May 15, 2024
Copy link
Collaborator

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

✨ beautiful, thank you!

MelSumner and others added 16 commits May 16, 2024 11:27
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
Co-authored-by: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com>
@hashishaw hashishaw enabled auto-merge (squash) May 16, 2024 16:30
@hashishaw hashishaw merged commit 530b266 into main May 16, 2024
31 checks passed
@hashishaw hashishaw deleted the melsumner/VAULT-26829 branch May 16, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants