[MWPW-171403][Preflight][Due diligence] Integrate full accessibility report in Preflight #3968
Conversation
7cbba1d to
4502b2c
Compare
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
| // Filter DOM elements based on include/exclude | ||
| const elements = getFilteredElements(config.include, config.exclude); | ||
|
|
||
| if (!elements.length) { |
There was a problem hiding this comment.
nit: this can be written in one line without curly braces
|
|
||
| const violations = checkFunctions.flatMap((checkFn) => checkFn(elements, config)); | ||
|
|
||
| return violations; |
There was a problem hiding this comment.
you can return violations directly without creating the constant
| try { | ||
| // Filter DOM elements based on include/exclude | ||
| const elements = getFilteredElements(config.include, config.exclude); | ||
|
|
There was a problem hiding this comment.
this applies to multiple files - let's remove the empty lines between const declarations, const declaration / if, etc
| // Skip this check if 'altText' isn't enabled in the config | ||
| if (!checks.includes('altText')) return []; | ||
| const violations = []; | ||
| const images = elements.filter((el) => el.tagName?.toLowerCase() === 'img'); |
There was a problem hiding this comment.
NIT:
| const images = elements.filter((el) => el.tagName?.toLowerCase() === 'img'); | |
| const images = elements.filter((el) => el.tagName === 'IMG'); |
libs/blocks/preflight/accessibility/check-keyboard-navigation.js
Outdated
Show resolved
Hide resolved
| focusableElements.forEach((el) => { | ||
| const styles = window.getComputedStyle(el); | ||
| const hasVisibleOutline = styles.outlineStyle !== 'none' && parseFloat(styles.outlineWidth) > 0; | ||
| const hasBoxShadow = styles.boxShadow !== 'none' && styles.boxShadow !== ''; |
There was a problem hiding this comment.
NIT
| const hasBoxShadow = styles.boxShadow !== 'none' && styles.boxShadow !== ''; | |
| const hasBoxShadow = styles.boxShadow !== 'none' && styles.boxShadow; |
libs/blocks/preflight/accessibility/check-keyboard-navigation.js
Outdated
Show resolved
Hide resolved
| export function getFilteredElements(includeSelectors = ['body'], excludeSelectors = []) { | ||
| const includedElements = includeSelectors.flatMap((selector) => Array.from(document.querySelectorAll(`${selector}, ${selector} *`))); | ||
|
|
||
| if (excludeSelectors.length === 0) return includedElements; |
There was a problem hiding this comment.
NIT:
| if (excludeSelectors.length === 0) return includedElements; | |
| if (!excludeSelectors.length) return includedElements; |
There was a problem hiding this comment.
I really like this implementation. It's effectively combining axe with some solid custom checks.
I’ve left a few comments on a few things that I've found.
In addition, adding unit tests would help ensure future changes don’t break anything, especially as preflight usage grows.
For the UI, the layout feels cramped for me when scrolling through multiple violations, as it only uses half the screen. Maybe we could consider moving violations directly under the failing accessibility test and using the full width to improve readability. What do you think?
| const styles = window.getComputedStyle(el); | ||
| const isVisible = styles.display !== 'none' && styles.visibility !== 'hidden' && parseFloat(styles.opacity) > 0; | ||
| const hasText = el.textContent.trim().length > 0; | ||
| const tagWhitelist = ['p', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'a', 'button']; |
There was a problem hiding this comment.
I'm a bit concerned that certain elements might get missed here, such as a span tag, or list element <li><ul>, etc.
Have you considered instead filtering for elements that are visible and have direct text content? I think that could be more comprehensive.
There was a problem hiding this comment.
I initially included <span>, but later removed it to avoid false positives — similar to the concern with relying on direct text content. While direct text is more comprehensive, it can also give false positives. I've added <ul> and <li>, and will continue refining the tag list based on further exploration.
Please advise — WDYT?
In milo blocks typically i din't find standalone text inside <span> it’s usually nested within above whitelisted tags ( i might be missing the some cases ).
| // Traverses the DOM to find the effective background color. | ||
| function getEffectiveBackgroundColor(element) { | ||
| let el = element; | ||
| while (el) { |
There was a problem hiding this comment.
this is clever, but I don't think this will work for important cases like a marquee. The background is not in the parents elements there.
https://main--cc--adobecom.aem.live/products/photoshop-lightroom
When I set the background to blue for example, the text background should recognize that the background is blue, but in this case it does not because the background is not in the direct parent chain.
There was a problem hiding this comment.
@vhargrave: You're right—this won't cover the above case, layered backgrounds from siblings, or positioned elements. I think we can create stories/Jiras for the same and work on follow-up PRs.
| }; | ||
|
|
||
| export const CUSTOM_CHECKS_CONFIG = { | ||
| checks: ['altText', 'color-contrast'], |
There was a problem hiding this comment.
Is is deliberate that you're not adding the keyboard checks as well?
There was a problem hiding this comment.
Is is deliberate that you're not adding the keyboard checks as well?
@vhargrave: Yes, it’s deliberate. Still exploring the use cases where axe-core doesn’t fully capture keyboard navigation issues when triggerred from browser/dom, will enable in subsequent pr's
|
Hi @vhargrave @zagi25 @narcis-radu , please check the review updates/responses |
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
|
Closing this PR due to inactivity. |
|
Heya, since this PR has been open since a while - could you leave some feedback on https://github.com/orgs/adobecom/discussions/4407 why/what is blocking this from proceeding? |
@robert-bogos : updated this one , merged/added earlier alt text feature into this pr accessibility tab |
There was a problem hiding this comment.
@skumar09 I see some unit tests are failing. Are they related to the code changes?
@robert-bogos : They are not related to code change, i have updated the branch with latest code base from stage, now they are passing, please review |
|
Skipped merging 3968: [MWPW-171403][Preflight][Due diligence] Integrate full accessibility report in Preflight due to failing or running checks |
3 similar comments
|
Skipped merging 3968: [MWPW-171403][Preflight][Due diligence] Integrate full accessibility report in Preflight due to failing or running checks |
|
Skipped merging 3968: [MWPW-171403][Preflight][Due diligence] Integrate full accessibility report in Preflight due to failing or running checks |
|
Skipped merging 3968: [MWPW-171403][Preflight][Due diligence] Integrate full accessibility report in Preflight due to failing or running checks |
Resolves: MWPW-171403
Test URLs:
Test Scenarios:
Accessibility PASS
URL: https://accessibility-preflight--milo--skumar09.aem.page/drafts/nala/preflight/media-light
Accessibility FAIL ( Image - Alt Text is not correct)
URL: https://accessibility-preflight--milo--skumar09.hlx.page/drafts/nala/preflight/media-image
Accessibility FAIL ( Color Contrast Issue)
URL: https://accessibility-preflight--milo--skumar09.aem.page/drafts/nala/preflight/media-small