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(toolbar): add anchor element to interactive elements #10343

Merged
merged 3 commits into from Mar 7, 2024

Conversation

ematipico
Copy link
Member

Changes

This PR adds a to the list of element that can be interactive.

Interactive elements are usually marked with the "widget" role. link role is a command role, which is a superclass that includes "widget".

Testing

I am happy to add a test with some guidance

Docs

N/A

Copy link

changeset-bot bot commented Mar 6, 2024

🦋 Changeset detected

Latest commit: 16537ef

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 6, 2024
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

LGTM, just needs a changeset! You can add a test in e2e/dev-toolbar-audits.test.js if you want to, but I'm totally okay with not having one right now. We can make a full test suite for audits later

@HiDeoo
Copy link
Member

HiDeoo commented Mar 6, 2024

Following the discussion from earlier today, I opened #10346.

According to this MDN reference, an anchor element is only considered interactive content if it has an href attribute:

Some elements belong to this category only under specific conditions:

<a>, if the href attribute is present
<audio>, if the controls attribute is present
<img>, if the usemap attribute is present
<input>, if the type attribute is not in the hidden state
<object>, if the usemap attribute is present
<video>, if the controls attribute is present

If my understanding is correct, the rules should check for the presence of the href attribute before considering the anchor element as interactive content (and same for the other elements with specific conditions). Any thoughts on this?

@Princesseuh
Copy link
Member

Following the discussion from earlier today, I opened #10346.

According to this MDN reference, an anchor element is only considered interactive content if it has an href attribute:

Some elements belong to this category only under specific conditions:
<a>, if the href attribute is present
<audio>, if the controls attribute is present
<img>, if the usemap attribute is present
<input>, if the type attribute is not in the hidden state
<object>, if the usemap attribute is present
<video>, if the controls attribute is present

If my understanding is correct, the rules should check for the presence of the href attribute before considering the anchor element as interactive content (and same for the other elements with specific conditions). Any thoughts on this?

Ah yeah, the optimal solution would be to check in the match property. An helper could be made to make that easier to re-use.

@ematipico
Copy link
Member Author

Changeset added. I suppose we can create the helper in another PR?

@Princesseuh
Copy link
Member

Changeset added. I suppose we can create the helper in another PR?

Well it's not just the helper, the behaviour in this PR is incorrect since a a is only interactable under conditions

@ematipico
Copy link
Member Author

All right, I'll add the logic tomorrow

@ematipico ematipico marked this pull request as draft March 7, 2024 08:56
@ematipico ematipico marked this pull request as ready for review March 7, 2024 09:24
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Seems fine!

@ematipico ematipico merged commit f973aa9 into main Mar 7, 2024
13 checks passed
@ematipico ematipico deleted the fix/toolbar-a11y-audit branch March 7, 2024 11:18
@astrobot-houston astrobot-houston mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants