Skip to content

Invalid <label> tags in quick links menu #1287

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

Closed
vkbo opened this issue Apr 11, 2023 · 7 comments · Fixed by #1300
Closed

Invalid <label> tags in quick links menu #1287

vkbo opened this issue Apr 11, 2023 · 7 comments · Fixed by #1300
Labels
tag: accessibility Issues related to accessibility issues or efforts

Comments

@vkbo
Copy link
Contributor

vkbo commented Apr 11, 2023

Inside the anchor tag in the list elements that make up the quick links menu, there is a label tag after the span that loads the fontawesome icon. I'm not sure why those label tags were added, but this is an incorrect use of label tags. They are intended for use with form input elements, not in anchor tags.

I'm happy to submit a fix, but I was wondering what the reason for them were first. They were added in #293.

<a {{ attributeString }}>
{%- if type == "fontawesome" -%}
<span><i class="{{ icon }}"></i></span>
<label class="sr-only">{{ name }}</label>
{%- elif type == "local" -%}
<img src="{{ pathto(icon, 1) }}" class="icon-link-image" alt="{{ name }}"/>
{%- elif type == "url" -%}
<img src="{{ icon }}" class="icon-link-image" alt="{{ name }}"/>
{%- else %}
<span>Incorrectly configured icon link. Type must be `fontawesome`, `url` or `local`.</span>
{%- endif -%}
</a>

@drammock
Copy link
Collaborator

<label class="sr-only">{{ name }}</label>

The class sr-only means "screen reader only" I think. I'm pretty sure the label value is there to provide something intelligible for screen readers to say about those icon links.

I also think that this is not a proscribed use of the label tag; the relevant MDN page says

The HTML element represents a caption for an item in a user interface.

...so it's not just limited to form elements IIUC. Feel free to re-open if you think I'm mistaken or misinterpreting.

@vkbo
Copy link
Contributor Author

vkbo commented Apr 11, 2023

I do think you are misinterpreting. The examples you show are all for input elements, and the html standard referenced also only uses it in the context of forms. In addition, the w3school page lists the elements for which it is valid, and last, but not least, the w3 validator explicitly states it's an error to use it in this way:

image

I cannot re-open the issue, only comment on it.

@vkbo
Copy link
Contributor Author

vkbo commented Apr 11, 2023

As for accessibility aspect, I think the correct solution is to add an aria-label attribute to the anchor tag instead. I ran my website through an accessibility tool, and those label tags were listed as orphaned labels, so the tool didn't seem to know what they were for, which defeats the entire purpose.

@drammock
Copy link
Collaborator

thanks for the thorough explanation. Indeed it seems I was misunderstanding the MDN page (because I didn't read it carefully enough, which I now have). Sorry about that. Please go ahead with the PR fix!

@drammock drammock reopened this Apr 11, 2023
@trallard
Copy link
Collaborator

Chiming in here to add more context from an accessibility POV in case @vkbo finds it useful for the PR

Indeed <label> is misplaced in this situation
and a more appropriate approach would be to adopt something like this:

<a {{ attributeString }}>
	{%- if type == "fontawesome" -%}
		<span><i class="{{ icon }}" aria-hidden="true"></i></span>
		<span class="sr-only">{{ name }}</span>
        ...
</a>

Where:

  • the aria-hidden attribute is used to indicate whether an element is exposed to screen readers or not (it removes the markup from the accessibility tree, we have ariadescribedby somewhere else, so it is safe to use within focusable links)
  • the sr-only class hides the element (visually) but keeps it accessible to assistive tech (I think that was the initial intent of the misused label)

Some references:

@trallard trallard added the tag: accessibility Issues related to accessibility issues or efforts label Apr 13, 2023
@vkbo
Copy link
Contributor Author

vkbo commented Apr 13, 2023

Thanks!

I'll make the PR this weekend. I'm a bit overloaded at work at the moment. But as I understand it then, there's no need for the aria-label to describe the a, as it would instead be handled by the sr-only <span> that it wraps?

@trallard
Copy link
Collaborator

Correct that way the link remains focusable and visible for screen readers (and not the icon itself).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants