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

Fix: Seperate IDs for prefix/suffix and aria-describedby added for suffix (fixes #143) #144

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

joe-allen-89
Copy link
Contributor

@joe-allen-89 joe-allen-89 commented Jul 10, 2024

#143

Fix

ref:
https://webaim.org/techniques/forms/advanced#describedby

@oliverfoster
Copy link
Member

Could you describe how this reads with the screen readers and why you're using both aria-labelledby and aria-describedby please?

aria-labelledby
aria-describedby

https://benmyers.dev/blog/aria-labels-and-descriptions/

It is possible to have more than one id in aria-labelledby:

aria-labelledby={[prefix && `${_id}-${index}-aria-prefix`, suffix && `${_id}-${index}-aria-suffix`].filter(Boolean).join(" ")}`

@joe-allen-89
Copy link
Contributor Author

Could you describe how this reads with the screen readers and why you're using both aria-labelledby and aria-describedby please?

aria-labelledby aria-describedby

https://benmyers.dev/blog/aria-labels-and-descriptions/

It is possible to have more than one id in aria-labelledby:

aria-labelledby={[prefix && `${_id}-${index}-aria-prefix`, suffix && `${_id}-${index}-aria-suffix`].filter(Boolean).join(" ")}`

@oliverfoster when adding both to aria-labelledby the suffix is read out before the content of the textInput (either placeholder or user answer), by adding describedby it reads out the suffix after the content of the text input which seemed like a more logical order.

@oliverfoster
Copy link
Member

oliverfoster commented Jul 15, 2024

Does it do that with all of the screen readers? (nvda, voiceover and jaws in firefox, safari and chrome?) I can't see it defined that way in any of the specs. Do you have a reference we can pin on the pr?

@joe-allen-89
Copy link
Contributor Author

joe-allen-89 commented Jul 15, 2024

I've tested with Jaws on Chrome, Firefox and Edge. I shall have a look for a reference.

@joe-allen-89
Copy link
Contributor Author

@oliverfoster - https://webaim.org/techniques/forms/advanced#describedby

"Screen readers will typically read the label, then read the input type (text box, checkbox, etc., plus any necessary properties, such as if the field is required), and then read the description."

Whilst this doesn't say 100% will read the description after the input that's the typical behaviour.

Copy link

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

When testing this using VoiceOver on Mac, the placeholder "Enter answer here" aria-label isn't read when a prefix is set. Looks like aria-labelledby replaces aria-label.

aria-labelledby takes precedence over all other methods of providing an accessible name, including aria-label, , and the element's inner text.
ref: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby

I don't have access to test Windows screen readers to see if this is a global issue.

@joe-allen-89
Copy link
Contributor Author

@kirsty-hames I've added the placeholder to the aria-labelledby so it will always be included as aria-describedby doesn't take precedence over aria-label.

I've also updated both to reflect the changes made to the slider to remove the attribute by setting it to null when not in use - adaptlearning/adapt-contrib-slider@41976c6

@kirsty-hames
Copy link
Contributor

Thanks for the updates @joe-allen-89. On retest, the placeholder text doesn't read. aria-describedby requires the id of an element, rather than passing in the placeholder text. We may have to create a separate label containing the placeholder text instead and reference this by id. For example:

            <div className="textinput-item__textbox-container">
              <input
                className="textinput-item__textbox js-textinput-textbox"
                type="text"
                placeholder={placeholder}
                data-id={`${input}-${index}`}
                id={`${_id}-${index}`}
                aria-labelledby={(prefix) ? `${_id}-${index}-aria-prefix ${_id}-${index}-aria-placeholder` : null}
                aria-describedby={(suffix) ? `${_id}-${index}-aria-suffix` : null}
                aria-label={placeholder}
                defaultValue={userAnswer}
                disabled={!_isEnabled}
              />
              <div
                className='textinput-item__placeholder aria-label'
                id={`${_id}-${index}-aria-placeholder`}
                aria-hidden='true'>{placeholder}
              </div>
              <div className="textinput-item__state">
                <div className="textinput-item__icon textinput-item__correct-icon" aria-label={_globals._accessibility._ariaLabels.correct}>
                  <div className="icon" aria-hidden="true"/>
                </div>
                <div className="textinput-item__icon textinput-item__incorrect-icon" aria-label={_globals._accessibility._ariaLabels.incorrect}>
                  <div className="icon" aria-hidden="true" />
                </div>
              </div>
            </div>

Note, I've set .textinput-item__placeholder to aria-hidden to prevent this from reading twice.

@kirsty-hames
Copy link
Contributor

Hey @joe-allen-89, thanks for implementing the suggestions I made in 0a87b52.

I've tested this in both NVDA (Chrome on Windows) and VoiceOver (Safari and Mac) and experimented with some alternative approaches. As a general note, I have found that there's some duplication in reading, more so with Voiceover but I think this will be unavoidable with any solution. I think this is down to whether the individual screen reader picks up on the following attributes: placeholder, value, htmlFor labels, whether it provides it's own instruction for input forms e.g. "edit text", alongside any ARIA we provide.

With that in mind, I wonder whether we can simplify the ARIA into a single aria-label and remove the aria-describedby, aria-labelledby and .textinput-item__placeholder?

For example: ${prefix && prefix + ' '}${!userAnswer ? placeholder : userAnswer}${suffix && ' ' + suffix}

As noted above, some repetition still exists but it's a simpler implementation.

@joe-allen-89
Copy link
Contributor Author

@kirsty-hames as the element we're referencing is visible I think it's best practice to use aria-labelledby / describedby rather than having it all in aria-label. Happy to be overruled on that though - https://w3c.github.io/aria/#aria-label

The repition seems to come from it reading out the placeholder text within the input from my testing. Possibly an argument for removing placeholder text for all but screen reader users as the instruction text should be sufficient, not sure if that's ideal though.

@kirsty-hames
Copy link
Contributor

@kirsty-hames as the element we're referencing is visible I think it's best practice to use aria-labelledby / describedby rather than having it all in aria-label. Happy to be overruled on that though - https://w3c.github.io/aria/#aria-label

Thanks @joe-allen-89, That's fine and I agree to keep it as aria-labelledby / describedby.

The repition seems to come from it reading out the placeholder text within the input from my testing. Possibly an argument for removing placeholder text for all but screen reader users as the instruction text should be sufficient, not sure if that's ideal though.

I think we should avoid removing the placeholder text and may have to settle with repetition. At least all information is read even if repetition is a bit of a nuisance. I'll go ahead and approve.

@joe-allen-89 joe-allen-89 merged commit 34acf35 into master Oct 4, 2024
@joe-allen-89 joe-allen-89 deleted the issue/143 branch October 4, 2024 08:18
github-actions bot pushed a commit that referenced this pull request Oct 4, 2024
## [7.2.13](v7.2.12...v7.2.13) (2024-10-04)

### Fix

* Seperate IDs for prefix/suffix and aria-describedby added for suffix (fixes #143)  (#144) ([34acf35](34acf35)), closes [#143](#143) [#144](#144) [#143](#143)
Copy link

github-actions bot commented Oct 4, 2024

🎉 This PR is included in version 7.2.13 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefix and Suffix Labels Not Being Read by Accessibility Tools
4 participants