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

Fixed broken aria-label for disabled links in Foundation #685

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

daniel-rikowski
Copy link
Contributor

aria-label=" was missing for disabled links created by the Foundation helpers. Still, the attribute value itself was added, resulting in invalid HTML.

Before:

<li class="prev disabled" role="link" aria-disabled="true" Previous>

After:

<li class="prev disabled" role="link" aria-disabled="true" aria-label="Previous">

@ddnexus
Copy link
Owner

ddnexus commented Apr 10, 2024

@daniel-rikowski good catch and good patch ... missing only the e2e test, but I could add that, because it might not be so simple to setup and it's just trivial to fix if it's already setup. Please, let me know.

Thank you!

@ddnexus
Copy link
Owner

ddnexus commented Apr 10, 2024

Wondering why the e2e test didn't catch the HTML problem when I introduced it. 🤔

@daniel-rikowski
Copy link
Contributor Author

Wondering why the e2e test didn't catch the HTML problem when I introduced it. 🤔

It looks they are tailored to the broken code, too.

In shapshots.js at line 456 and following:

previous=\"\">&lt;</li>

@daniel-rikowski
Copy link
Contributor Author

@daniel-rikowski good catch and good patch ... missing only the e2e test, but I could add that, because it might not be so simple to setup and it's just trivial to fix if it's already setup. Please, let me know.

Yes, please. I'm currently on a Windows workstation, I guess it would be even harder... 🙄

@ddnexus
Copy link
Owner

ddnexus commented Apr 10, 2024

Well... you can add any arbitrary attribute and HTML-validate does not complain.
I didn't even find any rules about unknown attributes, so I researched it, and it looks like adding custom attributes to HTML is not invalid HTML!

The previous=\"\">&lt;</li> snapshot normalization of a valid custom attribute happens because Previous is treated as a custom empty attribute (which is valid when it omits the ="" part) on parsing. Then it's probably just normalized as lowercase and dumped with the AST tree in a file as the extended version with the ="".

So, I have no more complaints about HTML-validate not complaining about perfectly valid HTML 🙃

I'm currently on a Windows workstation

😱 You have my sympathy. I will run the tests myself with a deep sense of compassion 😇

Thank you for your help!

@ddnexus ddnexus changed the base branch from master to dev April 10, 2024 11:54
@ddnexus ddnexus merged commit c8e81b7 into ddnexus:dev Apr 10, 2024
6 checks passed
@daniel-rikowski
Copy link
Contributor Author

Interesting... I always assumed, custom attributes must be data- attributes. ¯\(ツ)

Glad to be of help 😄

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 this pull request may close these issues.

2 participants