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

CI: implement htmlcs runner for pa11y-ci #1677

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

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Dec 5, 2022

Related issues

#938

Description

Add a runner to pa11y-ci since it could detect some errors that aXe doesn't detect.
The remaining errors may be fixed from twbs/bootstrap#38219.

Motivation & Context

Have a better ci. It's not much work compared to what it can detect and avoid. Seems to have a better algorithm to detect color issues. Following #938 to ensure that it won't break.

According to a11y expert, it's better to have as much test as possible since the algorithm are different and can be a bit more accurate to some points. Furthermore, it doesn't add much work to make it work.

Types of change

  • New feature (non-breaking change which adds functionality)

Live preview

https://deploy-preview-1677--boosted.netlify.app/docs/examples/cards/

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 8ec6f59
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/65968f5fc0a31a0008bf7061
😎 Deploy Preview https://deploy-preview-1677--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

louismaximepiton

This comment was marked as outdated.

@julien-deramond

This comment was marked as resolved.

@julien-deramond

This comment was marked as outdated.

@sonarcloud
Copy link

sonarcloud bot commented Dec 16, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
51.9% 51.9% Duplication

@louismaximepiton louismaximepiton marked this pull request as ready for review December 16, 2022 14:54
@sonarcloud
Copy link

sonarcloud bot commented Jan 18, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
48.8% 48.8% Duplication

` • This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 3.66:1. Recommendation:  change text colour to #757575.

   (html > body > div:nth-child(6) > main > div:nth-child(3) > div:nth-child(25) > div:nth-child(1) > nav > ul > li:nth-child(1) > a)

   <a class="page-link">Previous</a>`
@louismaximepiton
Copy link
Member Author

louismaximepiton commented Aug 1, 2023

Here is a tracker for all the hidden elements.

  • iframe: We don't control iframes so it'll be hard to maintain. It doesn't bring much to maintain this imo,
  • .table-dark .table-active, caption.visually-hidden: Due to box-shadow used to change the background-color, it raises a false positive,
  • .table-responsive: Well documented by Scott O'hara, it seems like it's fine not to force the focus on scrollable areas since people can still access it if they aren't using keyboard only (which is mostly the case).
  • :disabled + label, .is-disabled: Those come from forms and are associated to disabled inputs. It raises false positive,
  • .exclude-from-pa11y-analysis: A class to help remove some non working samples due to design mostly.

What was removed and need to be backported to Bs maybe:

  • #offcanvas, #offcanvasDark, #offcanvasResponsive, #bdSidebar by changing <div> with <section>.
  • Using an alert instead of .text-warning in the grid example

Things that should be discussed before merging:

  • All .exclude-from-pa11y-analysis
  • Including a submit button inside search bar in navbar.

@@ -521,7 +521,7 @@ Responsive tables make use of `overflow-y: hidden`, which clips off any content
Across every breakpoint, use `.table-responsive` for horizontally scrolling tables.

<div class="bd-example">
<div class="table-responsive">
<div class="table-responsive" tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about adding tabindex="0" everywhere.
It seems not yet ok on our side (#1711) and on Boostrap's side (twbs/bootstrap#38971).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll remove this rule and hide some more elements.

Copy link
Contributor

@MewenLeHo MewenLeHo Aug 7, 2023

Choose a reason for hiding this comment

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

I thinks it's a safer option for now.
I just saw that the linked PR on Bootstrap side has been validated. We may now consider to follow their footsteps.

@@ -32,10 +32,10 @@
<small class="font-monospace text-body-secondary text-uppercase">{{- $lang -}}</small>
<div class="d-flex ms-auto">
<button type="button" class="btn-edit text-nowrap"{{ with $stackblitz_add_js }} data-sb-js-snippet="{{ $stackblitz_add_js }}"{{ end }} title="Try it on StackBlitz">
<svg class="bi" aria-hidden="true"><use xlink:href="#lightning-charge-fill"/></svg>
<svg class="bi" aria-hidden="true" focusable="false"><use xlink:href="#lightning-charge-fill"/></svg>
Copy link
Contributor

@MewenLeHo MewenLeHo Aug 3, 2023

Choose a reason for hiding this comment

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

Was needed for IE but we are no longer supporting IE so we may not need it anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it for the consistency of the doc + I think it's fine having it atm since Bootstrap have it too. Feel free to open a PR on Bootstrap side if you want to remove all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

And what about adding focusable="false" to bootstrap directly? 🤔
https://github.com/twbs/bootstrap/blob/main/site/layouts/shortcodes/example.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll propose a PR on this subject on Bs directly!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. I will resolve this comment once this change will be validated by their core team.

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

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

Some points to clarify and it will be ok.

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

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

Remaining subjects:

  • adding focusable="false" to svg in shortcodes/example.html or not depending if it's possible to do directly on Bootstrap's side or not.
  • reevaluate the need to add tabindex=0 since Bootstrap's core team reach an agreement on this subject and are ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Dev Review In Progress
Development

Successfully merging this pull request may close these issues.

3 participants