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: Added a check for a hardcoded aria 'role' value in the same element. #493

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JMac118
Copy link

@JMac118 JMac118 commented Mar 28, 2024

This will stop it from "guessing" if an element is a navigation or main element if the element has the aria role of 'navigation' or 'main'. It won't need to guess it if it is specifically outlined to be that.

This will stop it from "guessing" if an element is a navigation or main element if the element has the aria role of 'navigation' or 'main'. It won't need to guess it if it is specifically outlined to be that.
Copy link
Author

@JMac118 JMac118 left a comment

Choose a reason for hiding this comment

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

When a website has a aria value of 'navigation' or 'main' the extension can still add "guessed" values to the list when it really doesn't need to, which will cause duplicate landmarks in the extension (one actual value, one guessed). This change should stop that.

@JMac118 JMac118 changed the title Added a check for a hardcoded aria 'role' value in the same element. fix: Added a check for a hardcoded aria 'role' value in the same element. Mar 28, 2024
@matatk
Copy link
Owner

matatk commented Nov 9, 2024

Hi @JMac118. I’m sorry I missed your PR until now, thank you for your contribution! This was an oversight on my part, and needs to be fixed. I have actually for some time been working on a big re-write behind the scenes, which I’m releasing in stages (I just merged the code into main, and submitted the extension to Chrome for review - I will follow up with the other browsers later, though there aren’t any real changes for users in browsers other than Chrome, for now).

Your PR won’t be able to be merged automatically due to the changes I’ve been making - apologies - but I can see how it works, and ensure that these changes are adopted.

Ideally it should be possible for me to add to the test suite to highlight some/all of the this oversight in the code.

@JMac118
Copy link
Author

JMac118 commented Nov 11, 2024

No problem @matatk

Thank you for acknowledging my PR and for the work you are doing on the re-write. I appreciate your efforts to improve the extension.

I was working on implementing more accessibility features for a website with an admittedly non-standard way of handling ARIA roles and other accessibility elements, when I came across this issue and couldn't have the extension suppress the "guesses". Since the EU accessibility mandate is being enforced midway through next year, I figured this change would be a good quality of life improvement for the user experience.

Would you like me to take a look at matching my changes to the ones you merged into main?

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