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: Minor accessibility warnings #141

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix: Minor accessibility warnings #141

wants to merge 9 commits into from

Conversation

filipewl
Copy link
Contributor

@filipewl filipewl commented Jun 22, 2022

What's the purpose of this pull request?

Fix some minor accessibility warnings reported by axe DevTools:

  1. The nav elements that are used in the footer's links are considered landmarks and all landmarks must have a unique accessible name.
  2. Our Hero component makes use of a header element with an aria-labelledby attribute, but that's not a valid combination because the header has no role when it's a descendant of an article.
  3. Sometimes the axe's accessibility check run on the Home page via the integration tests fails due to insufficient color contrast. It's unclear why this fails nondeterministically, or why it's always only on the Home page considering the nav bar is shared between the PLP and PDP pages well. Nor why I could not reproduce it by running the test locally a lot of times. The logs don't report which element is associated with it so I added more information to them. I was able to get a hint it could be the mobile's search input by running axe DevTools with the mobile's viewport. After some investigation, It could be because the input is not completely hidden just with 0 width. Setting the opacity to 0 seems to fix the issue and it has no visual or functional drawback.
1 2 3
CleanShot 2022-06-22 at 14 48 10 CleanShot 2022-06-22 at 14 48 58 CleanShot 2022-06-23 at 12 37 05

There are still going to be 2 warnings left being reported by axe:

  1. Needs to be patched via the PaymentMethods component inside @faststore/ui.
  2. Seems more tricky and is associated with the Incentives component.
1 2
CleanShot 2022-06-23 at 12 06 18 CleanShot 2022-06-23 at 16 18 19

References

@filipewl filipewl added the Bug Fixes Something isn't working label Jun 22, 2022
@filipewl filipewl self-assigned this Jun 22, 2022
@vercel
Copy link

vercel bot commented Jun 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview Nov 18, 2022 at 8:25AM (UTC)

@vercel vercel bot temporarily deployed to preview June 22, 2022 18:06 Inactive
@vtex-sites
Copy link

vtex-sites bot commented Jun 22, 2022

Preview is ready

This pull request generated a Preview

👀   Preview: https://sfj-64fb2f8--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 64fb2f8

@vtex-sites
Copy link

vtex-sites bot commented Jun 22, 2022

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 38b5f7f

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse/p
📎   /office

We already print the description of the violation, but it's not immediately clear which element is causing it.

e.g. in https://github.com/vtex-sites/nextjs.store/pull/141/checks?check_run_id=7010437942 we had a color contrast violation but no pointer which element is causing it.
@vercel vercel bot temporarily deployed to preview June 23, 2022 01:40 Inactive
@vercel vercel bot temporarily deployed to preview June 23, 2022 13:07 Inactive
@vercel vercel bot temporarily deployed to preview June 23, 2022 15:13 Inactive
@vercel vercel bot temporarily deployed to preview June 23, 2022 18:28 Inactive
Attempt to fix [this][1] flaky axe's accessibility issue of color contrast.

Apparently because the input is not totally hidden, it's getting picked up by axe and failing the color contrast check because there's no text and due the small dimension (0 width). Setting the opacity to 0 seems to fix and there seems to be no visual/functional drawback.

dequelabs/axe-core#3514

[1]: https://github.com/vtex-sites/nextjs.store/runs/7015698254#:~:text=description%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%E2%94%82%20nodes%20%E2%94%82%0A%E2%94%9C%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%BC%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%BC%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%BC%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%BC%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%80%E2%94%A4%0A%E2%94%82%20%20%20%200%20%20%20%20%E2%94%82%20%27-,color%2Dcontrast,-%27%20%E2%94%82%20%27serious%27%20%E2%94%82%20%27Ensures%20the
@vercel vercel bot temporarily deployed to preview June 23, 2022 18:42 Inactive
@filipewl filipewl marked this pull request as ready for review June 23, 2022 19:12
@@ -37,6 +37,15 @@ function logToTerminal(violations) {
nodes: nodes.length,
}))
)
for (const violation of violations) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of this new log. Hoping it can help us to better pinpoint the elements with the issues.
Screen Shot 2022-06-21 at 15 38 01

Copy link
Contributor

Choose a reason for hiding this comment

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

😍 thank youu!

@filipewl filipewl requested review from a team and removed request for a team June 23, 2022 19:22
Copy link
Contributor

@icazevedo icazevedo left a comment

Choose a reason for hiding this comment

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

Terrific work, Filipe. I tried really hard to read the table you linked on item 2, but I couldn't understand it. I tried reading the description of the table above it, but still, I couldn't figure out what each column means and how this affects what you did here. Maybe we can meet to discuss this later on?

Our Hero component makes use of a header element with an aria-labelledby attribute, but that's not a valid combination because the header has no role when it's a descendant of an article.

@hellofanny
Copy link
Contributor

Welll doneee! This will be so helpful @filipewl 😍😍😍

Regarding the 2 warning left:

  1. I was wondering if we could just remove that aria-label 🤔
  2. I remember that we need a little upgrade on this component 🙈 In the ideal version, we should add a carousel to the Incentives component. Maybe we can prioritize it or use a workaround for now. WTDT?

Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

Good Job! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants