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

When title is missing, should aria-hidden be "false" or default? #294

Closed
skylerberg opened this issue Oct 18, 2023 · 5 comments · Fixed by #297
Closed

When title is missing, should aria-hidden be "false" or default? #294

skylerberg opened this issue Oct 18, 2023 · 5 comments · Fixed by #297

Comments

@skylerberg
Copy link

Currently, this library sets the aria-hidden attribute based on whether or not title is falsy.

:aria-hidden="!title"
:aria-label="title"

In the change between Vue2 and Vue3, the default behavior for v-bind attributes with the value false changed. Specifically:

BREAKING: No longer removes attribute if the value is boolean false. Instead, it's set as attr="false". To remove the attribute, use null or undefined.

This leads to a subtle change in behavior in this package. Instead of omitting aria-hidden when title is unset, it will include aria-hidden="false". According to MDN, here are the different settings:

false
The element is exposed to the accessibility API as if it was rendered.

true
The element is hidden from the accessibility API.

undefined (default)
The element's hidden state is determined by the user agent based on whether it is rendered.

I'm no expert in accessibility, but it seems like the default behavior would be more desirable. That way non-rendered icons won't be read by screen readers.

I'll put up a PR in case you also think we should change this.

P.S., I found this because it was causing a lot of noise as warnings on a migration build of Vue, so changing this will improve the migration experience a bit.

@robcresswell
Copy link
Owner

robcresswell commented Oct 23, 2023

Hey, thanks for opening this. Definitely happy to improve this, but I'm unsure what the problem is.

That way non-rendered icons won't be read by screen readers

They shouldn't be read by screen readers now. If there's a title, then hidden will be false and the label will be announced I believe, and if the title is falsy, then hidden will be true, which seems correct. Have I misunderstood?

I'm a little confused by the migration issue too, why does setting false cause a migration warning? Isn't false a valid setting here? I don't actually use Vue any more, and I don't recall the migration process.

@skylerberg
Copy link
Author

Thanks for taking a look at this even though you are not using vue anymore!

If there's a title, then hidden will be false and the label will be announced

This is the scenario I think we can have undesirable behavior. If hidden is false, then it will announce it unconditionally. If hidden is undefined, it will announce it if it is rendered. The behavior I am concerned about is announcing the title for icons that are not rendered.

When migrating from vue 2 to vue 3, you can use a migration build that will issue warnings for behavior that has changed. The behavior that changed that is relevant here is this: https://v3-migration.vuejs.org/breaking-changes/attribute-coercion.html

Previously, if you bound an attribute to a false value, the attribute would be removed. In vue 3, it literally sets it to false. Thus, we used to set hidden to undefined rather than false.

@robcresswell
Copy link
Owner

Previously, if you bound an attribute to a false value, the attribute would be removed. In vue 3, it literally sets it to false. Thus, we used to set hidden to undefined rather than false.

I understand the difference

The behavior I am concerned about is announcing the title for icons that are not rendered.

What I don't understand is why / how this would occur. I think your change probably makes sense anyway, but I'm still not following if its a real issue or just a theoretical one. Sorry for my brain slowness, I am not a clever man.

@skylerberg
Copy link
Author

What I don't understand is why / how this would occur. I think your change probably makes sense anyway, but I'm still not following if its a real issue or just a theoretical one.

I don't have a screen reader and am not an accessibility expert, so it's theoretical for me. But I have a theory of a practical scenario it might matter in.

Suppose a page has several modals with icons. The modals are in the DOM but hidden while not active with something like display: none in CSS. My theory is that by setting aria-hidden="false" we would cause a screen reader to announce the icons in the modals even though they are not rendered.

Sorry for my brain slowness, I am not a clever man.

That makes two of us, so thanks for joining me in bumbling through this :)

@robcresswell
Copy link
Owner

Ah, I see what you mean now, thank you. Yes, I think it would be wise to merge your PR. I should be able to get to it Friday evening / Saturday morning; I'll do a release of updated icons etc at the same time

robcresswell added a commit that referenced this issue Jan 28, 2024
Fixes: #294

Co-Authored-By: skylerberg
robcresswell added a commit that referenced this issue Jan 28, 2024
Fixes: #294

Co-Authored-By: skylerberg
Signed-off-by: Rob Cresswell <robcresswell@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants