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

Add label to network list search input #4887

Closed
wants to merge 1 commit into from

Conversation

mven
Copy link

@mven mven commented Jul 16, 2024

resolves #4863
resolves #4864

@@ -22,7 +22,7 @@
:data-name="channel.name"
:data-type="channel.type"
:aria-controls="'#chan-' + channel.id"
:aria-selected="active"
:aria-selected="true"
Copy link
Author

@mven mven Jul 16, 2024

Choose a reason for hiding this comment

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

Drive-by fix here. active is not a valid value. Valid values are: true, false, or undefined (blank)

Copy link
Member

Choose a reason for hiding this comment

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

NACK, active is a prop, passed via the element by vue.

Copy link
Author

Choose a reason for hiding this comment

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

Ah right. I'm new to Vue and I missed that this was a dynamic attribute. I'll revert this change shortly.

@keypress.enter.exact.prevent="onSubmit"
@blur="onBlur"
/>
<label :aria-label="getInputPlaceholder(channel)">
Copy link
Author

Choose a reason for hiding this comment

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

added parent label and moved aria-label to it, same as below.

@brunnre8
Copy link
Member

Can you explain to me how this fixes #4863?
You populate the label with the same content as is already there, so that doesn't seem to do anything at a glance for screen readers?

As for #4864 this has the aria-label already on there as well, normal users don't need a label as the placeholder text serves the same purpose, so what's the rational for doing it?

@brunnre8
Copy link
Member

brunnre8 commented Jul 16, 2024

as is this also borks the CSS as far as I can tell, for instance the chat input:

image

@brunnre8 brunnre8 added the Status: changes-requested Review happened but some changes need to be made label Jul 16, 2024
@mven
Copy link
Author

mven commented Jul 17, 2024

Can you explain to me how this fixes #4863? You populate the label with the same content as is already there, so that doesn't seem to do anything at a glance for screen readers?

One distinction is that the label associated with the input provides more of a semantic structure than what's currently there. Though, I've run a few tests on my end and realized that this code change isn't best practice taking the intentional lack of a visual label into consideration.

It might not cover all cases and there's a fallback such as wrapping the label around the input much like this code change, but to visually hide a span that contains the label name. This requires some additional code and care, so it might not the best solution, despite it covering some edge cases.

What's currently in the source of these corresponding changes currently meets WCAG 2.1 AA guidelines, so I think I'll close this PR.

@mven
Copy link
Author

mven commented Jul 17, 2024

As for #4864 this has the aria-label already on there as well, normal users don't need a label as the placeholder text serves the same purpose, so what's the rational for doing it?

placeholder text isn't sufficient since they're mostly inaccessible and folks with vision impairments can have problems with it. An improvement could be made to add aria-placeholder, but this should only be used as a supplement to an actual label (or aria-label).

@mven
Copy link
Author

mven commented Jul 17, 2024

Closing out for now! Thanks for the review @brunnre8

@mven mven closed this Jul 17, 2024
@mven mven deleted the mven/searchlabel branch July 17, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: changes-requested Review happened but some changes need to be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input to search channels lacks label Textarea for entering messages lacks label
2 participants