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: Address validationError tooltip console for outputs #5088

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

KearseTrevor
Copy link
Contributor

@KearseTrevor KearseTrevor commented Oct 21, 2024

https://desire2learn.atlassian.net/browse/GAUD-7125

After reviewing all inputs that contain validationError and FormElementMixin

  • All applicable elements made use of the same pattern (using announced attribute)
  • Going to go with parity with other elements

Action: Add for attribute to input-text, leave announced for now

I'm open to have a quick conversation about removing the use of announced from the elements that make use of it in their validationError tooltips

  • When I removed it, and trialed it out - the behaviour appeared equivalent, but I'm unsure if it is our desired behaviour in either case for Safari + VO
    --- Edit: Early attempts that spun off this PR to address this discussion point were unsuccessful

@KearseTrevor KearseTrevor requested a review from a team as a code owner October 21, 2024 14:49
Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-5088/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@@ -482,8 +482,8 @@ class InputText extends InputInlineHelpMixin(PropertyRequiredMixin(FocusMixin(La
let tooltip = nothing;
if (!this.skeleton) {
if (this.validationError && !this.noValidate) {
// this tooltip is using "announced" since we don't want aria-describedby wire-up which would bury the message in VoiceOver's More Content Available menu
tooltip = html`<d2l-tooltip state="error" announced align="start" class="vdiff-target">${this.validationError} <span class="d2l-offscreen">${this.description}</span></d2l-tooltip>`;
tooltip = html`<d2l-tooltip state="error" announced for="${this._inputId}" align="start" class="vdiff-target">${this.validationError} <span class="d2l-offscreen">${this.description}</span></d2l-tooltip>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want this to be:

<d2l-tooltip state="error" for="${this._inputId}" align="start" class="vdiff-target">${this.validationError}</d2l-tooltip>
  • By using for and pointing at the input element, we should get the desired aria-describedby wire-up (by d2l-tooltip), making announced unnecessary

  • The input will potentially already have aria-describedby if the consumer provided a description or content in the inline-help slot (see line 432 above). The d2l-tooltip's for logic will append the tooltip id to those here. So, the offscreen description inside the original tooltip should no longer be necessary. I suspect that might have been rendered to handle the case where the announce(...) helper could clobber the normal aria-describedby announcement - that would no longer be an issue.

  • Previously, the announcement order was: validation message, then the offscreen description, omitting the inline-help. If we still want to omit the inline-help, then we'll need another adjustment above on line 432 if there is a validation error. Also note: the helper appends the id to the end of aria-describedby here in elemIdListAdd. The order of the ids in aria-describedby might dictate the order those messages are announced. I think I checked that previously but I don't totally recall what I found. If so, and if we still want the validation message announced first, we might need to add an option to that helper so the caller can indicate whether the id should be appended to the beginning or end.

@KearseTrevor
Copy link
Contributor Author

KearseTrevor commented Oct 29, 2024

This current iteration is lacking proper accessibility when using VO+Safari
required invalid data edit text is all that's provided. - as in, it's not even buried underneath the more menus

For reference, the behaviour that is displayed in daylight right now via VO+Safari is also awkward but at least it say the error message eventually.

  • it is currently buried - and it repeats itself
    • along the lines of "required, invalid data edit text, "you are on a ...", Password is Required, Password is Required"

Using announced doesn't seem to fix this, which is odd because that is the only difference between what is here vs daylight in some cases?

@KearseTrevor
Copy link
Contributor Author

Pruned scope creep since attempts to address Safari + VO behaviour in middle and low verbosity were unsuccessful. Leaving it as a note here that the experience in Safari + VO for validation tooltips is just not ideal in general but would need an additional specific ticket to address as it's not a straightforward solve.

@dbatiste
Copy link
Contributor

Hmmm. That's unfortunate.

I did a quick test with Safari+VO (using d2l-input-text required demo) with the current code and it doesn't seem to announce the tooltip message. The only configuration that seems to announce the tooltip message is the original one. 😞 Just to confirm, is that what you observe also?

@KearseTrevor
Copy link
Contributor Author

Yes that's the same observation I've gotten - the only working version I was able to come up with was modifying the following the title attribute as follows - which I was unsure if that was kosher:

title="${ifDefined(this.validationError ? `${this.validationError} ${this.title ? this.title : ''}` : this.title)}"

@dbatiste
Copy link
Contributor

dbatiste commented Nov 6, 2024

Yes that's the same observation I've gotten - the only working version I was able to come up with was modifying the following the title attribute as follows - which I was unsure if that was kosher:

title="${ifDefined(this.validationError ? `${this.validationError} ${this.title ? this.title : ''}` : this.title)}"

Ok. We can run it by Jeff but I don't think we want that, since it would cause the browser's tooltip to display as well as our own.

@KearseTrevor
Copy link
Contributor Author

KearseTrevor commented Nov 7, 2024

Circling back, had a conversation with Jeff and it was decided that since this is specific to Safari + VO AND is relevant to error tooltips and not all input tooltips that we guide to - Rework our console warnings to remove noise on this specific case/configuration so we don't confuse consumers since they are using the components as expected

  • I'm fine to leave a comment in with whatever console rework is baked so that we know we're doing it because for attribute usage had a collision with Safari/VO environments that were outside our control

@dbatiste
Copy link
Contributor

dbatiste commented Nov 7, 2024

Ok, so if I understand correctly, we're going to leave the input code as-is. That is, not change require to d2l-input-text. But we update d2l-tooltip's warnings. Ya?

@dbatiste
Copy link
Contributor

Oh, one minor thing that would be good to fix up - the comment in d2l-input-text:

// this tooltip is using "announced" since we don't want aria-describedby wire-up which would bury the message in VoiceOver's More Content Available menu

It sounds like Apple has fixed that "More Content Available" piece. I think I actually found the discussion thread on that last week... turns out some other people were upset about it too. Anyway, I suggest we change this comment to:

// this tooltip is using "announced" since we don't want aria-describedby wire-up - VoiceOver ignores our message when the input is invalid

@KearseTrevor KearseTrevor merged commit 907c171 into main Nov 12, 2024
6 checks passed
@KearseTrevor KearseTrevor deleted the GAUD-7125/validation-error-tooltip branch November 12, 2024 16:57
Copy link

🎉 This PR is included in version 3.66.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants