-
Notifications
You must be signed in to change notification settings - Fork 934
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
USWDS - File input: Fix issues with voice command #5213
Conversation
…ons to let at top of function
… to removeOldPreviews params
|
@briandeconinck I have created a prototype here that changes the This is still a work in progress but before I get too far down the road, would you be able to test this prototype to see if you can access the input by speaking the visible text via Dragon Naturally Speaking (and/or any other voice control software you might have access to)? Please let me know if you have questions! |
|
Hi Amy! Sorry for the slow reply. I see the updated I will note that since the instruction text has the fake-link styling, the command "Click link" still doesn't highlight the input as an option, which would typically be the next thing I try if "Click choose from folder" doesn't get a match. Maybe make it an actual link or button with a JS action to map it to the input? I believe even if it's removed from the tab order with Up to you whether you want to pursue that. What you have here is definitely an improvement and should work most of the time for Dragon users (again, accounting for possible Dragon weirdness). |
|
Thanks for the response, @briandeconinck! Your test results are helpful. Since we aren't hitting consistent results with |
|
Note: considering removing the SR-only element readout if file input is disabled. Might determine that no action is needed or it could be a follow-up issue. Will investigate tomorrow. There is no need for this effort to interrupt any reviews in process. |
|
I added a check for the |
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.
Focused on code in this review and added some comments. Overall no issues, but some minor areas we can improve.
Mainly, saving the default aria label in code and possible duplicate check on disabled state.
|
|
||
| // Adds class names and other attributes | ||
| fileInputEl.classList.remove(DROPZONE_CLASS); | ||
| fileInputEl.classList.add(INPUT_CLASS); | ||
| fileInputParent.classList.add(DROPZONE_CLASS); | ||
| box.classList.add(BOX_CLASS); | ||
| instructions.classList.add(INSTRUCTIONS_CLASS); | ||
| instructions.setAttribute("aria-hidden", "true"); | ||
| dropTarget.classList.add(TARGET_CLASS); |
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.
Minor, but for clarity you could remove line 163 and place box inside of dropTarget instead of fileInputEl.parentNode.
| dropTarget.classList.add(TARGET_CLASS); | |
| dropTarget.classList.add(TARGET_CLASS); | |
| dropTarget.prepend(box); |
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.
Updated in e102337
Note: This does reorder usa-file-input__box and usa-file-input__instructions, but it does not seem to have an impact since both elements already had z-index set in their styles.
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.
Gotcha, thanks for that explanation. My main concern was lack of clarity when it came time to adding all the DOM elements and seeing what went where.
packages/usa-file-input/src/index.js
Outdated
| if (!disabled) { | ||
| createSROnlyStatus(fileInputEl); | ||
| } |
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.
There's some code that also checks for disabled/aria-disabled in createTargetArea(). Could any of that be moved in this check?
Code in createTargetArea():
uswds/packages/usa-file-input/src/index.js
Lines 165 to 173 in 0843926
| // Disabled styling | |
| if (disabled) { | |
| disable(fileInputEl); | |
| } | |
| // Aria-disabled styling | |
| if (ariaDisabled) { | |
| ariaDisable(fileInputEl); | |
| } |
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.
Moved disabled checks into enhanceFileInput() in cd6ba70
packages/usa-file-input/src/index.js
Outdated
| */ | ||
| const buildFileInput = (fileInputEl) => { | ||
| const setItemsLabel = (fileInputEl) => { |
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 doesn't seem to set anything, maybe we could clarify with getItemsLabel or something more descriptive.
| const setItemsLabel = (fileInputEl) => { | |
| const getItemsLabel = (fileInputEl) => { |
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.
Good catch. Updated in 38cf451
packages/usa-file-input/src/index.js
Outdated
|
|
||
| // Add initial instructions for input usage | ||
| fileInputEl.setAttribute("aria-label", defaultInstructionsText); | ||
| fileInputEl.setAttribute("data-default-aria-label", defaultInstructionsText); |
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.
Could we reuse defaultInstructionsText instead of setting it as a data attribute?
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.
| @@ -184,17 +210,60 @@ const buildFileInput = (fileInputEl) => { | |||
| fileInputParent.querySelector(`.${DRAG_TEXT_CLASS}`).outerHTML = ""; | |||
| } | |||
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.
packages/usa-file-input/src/index.js
Outdated
| } | ||
|
|
||
| // Hides null state content and sets preview heading | ||
| instructions.classList.add(HIDDEN_CLASS); |
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.
Small, but we have this HIDDEN_CLASS defined on line 21. An alternative would be to use the hidden attribute, like accordion. Would be one less const to manage.
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.
Replaced HIDDEN_CLASS in favor of the hidden attribute in 4278f23
Co-authored-by: James Mejia <james.mejia@gsa.gov>
Updated to improve clarity
…o al-file-input-vc
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.
@mejiaj Thanks for the helpful review! I have addressed all comments. Please let me know if you have questions.
|
|
||
| // Adds class names and other attributes | ||
| fileInputEl.classList.remove(DROPZONE_CLASS); | ||
| fileInputEl.classList.add(INPUT_CLASS); | ||
| fileInputParent.classList.add(DROPZONE_CLASS); | ||
| box.classList.add(BOX_CLASS); | ||
| instructions.classList.add(INSTRUCTIONS_CLASS); | ||
| instructions.setAttribute("aria-hidden", "true"); | ||
| dropTarget.classList.add(TARGET_CLASS); |
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.
Updated in e102337
Note: This does reorder usa-file-input__box and usa-file-input__instructions, but it does not seem to have an impact since both elements already had z-index set in their styles.
packages/usa-file-input/src/index.js
Outdated
| if (!disabled) { | ||
| createSROnlyStatus(fileInputEl); | ||
| } |
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.
Moved disabled checks into enhanceFileInput() in cd6ba70
packages/usa-file-input/src/index.js
Outdated
| } | ||
|
|
||
| // Hides null state content and sets preview heading | ||
| instructions.classList.add(HIDDEN_CLASS); |
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.
Replaced HIDDEN_CLASS in favor of the hidden attribute in 4278f23
packages/usa-file-input/src/index.js
Outdated
|
|
||
| // Add initial instructions for input usage | ||
| fileInputEl.setAttribute("aria-label", defaultInstructionsText); | ||
| fileInputEl.setAttribute("data-default-aria-label", defaultInstructionsText); |
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.
packages/usa-file-input/src/index.js
Outdated
| */ | ||
| const buildFileInput = (fileInputEl) => { | ||
| const setItemsLabel = (fileInputEl) => { |
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.
Good catch. Updated in 38cf451
On enhance file input will add a class if the component either has aria-disabled or disabled attribute.
|
@mejiaj I confirmed that your update adds the disabled class as expected when either Will you let me know if you see anything else? |
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.
Code looks good, I just have a comment on addPreviewHeading(). This function adds a description for screen readers for how many files and their names.
On voiceover this only gets read once after upload. We could improve this by having it be read on focus as a fast follow-up.
| * @param {HTMLInputElement} fileInputEl - The input element. | ||
| * @param {Object} fileNames - The selected files found in the fileList object. | ||
| */ | ||
| const addPreviewHeading = (fileInputEl, fileNames) => { |
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 function adds text describing:
- How many files
- Which files have been uploaded
So far I've only gotten it to read after initial upload. If I move to another area and comeback it doesn't get read on VoiceOver Chromium 113.
Possible Future enhancements:
- Add a unique ID to this message and tie it to the component via
aria-labelledby[MDN on aria-labelledby] - Move the preview heading text so it always gets read.
- Dynamically update the aria-label
Move the preview heading text example
file-input-label.mp4
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.
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 work


Summary
Improved the file input experience for voice command and screen reader users. Voice command users can now more easily interact with the component by speaking the visible instructions text. Additionally, screen reader users now have access to both the visible instructions text (in the
aria-label) and the file selection status (in a newly created screen reader-only element).Breaking change
This is not a breaking change.
All changes have been handled dynamically.
Related issue
Closes #5192
As a future enhancement, issue #5238 will create data attributes that will allow users to customize the status and instructions text.
Related pull requests
Changelog update: uswds/uswds-site#2088
Preview link
Preview link: File input
Problem statement
Users of assistive technologies should be able to intuitively access all form elements, understand what action they should take, and receive feedback so that they can confirm that the information they have added is correct. However, the file input component presents problems for both screen reader and voice command users:
Voice command issues
Because of file input's structure, voice command does not work in an intuitive way. A user would expect to vocalize the text shown on screen as part of a "Click" command. For file input, this would mean the user would expect to say something like "Click drag file here or choose from folder" or ""Click choose from folder" to interact with the file input. However, neither of these work because the accessibility tree does not include this information.
Screen readers issues
Screen reader users do not have access to the instructions text ("Drag file here or choose from folder"). Instead, the
aria-labelprovides feedback on the file selection status. This is insufficient information for interaction. The defaultaria-label"no file selected" does not clearly communicate what the input is or what the user should do, which can lead to confusion.Solution
To allow voice command users to easily access the file input component, we should make sure the input
aria-labelmatches the visible text as the status changes.To allow screen reader users to continue to confirm their file selection, we should move the file status from the
aria-labelto a dynamically createdsr-onlyelement.Major changes
buildFileInput()into separate functions:createTargetAreaandcreateVisibleInstructionsPossible future enhancements
Converting the CTA text ("choose from folder", "change file") from a
<span>element to a<button>element might improve consistency in Voice Command software. It could also give an alternative method for voice control users so that they can highlight all interactive elements on the page and select the element they want to click.Testing and review
Accessibility:
Developers:
The following items should be confirmed: