fix: Include body as part of background color checks when element does not intersect #1520
Conversation
There was a problem hiding this comment.
I tested the code changes in a sandbox. The changes fix the false positive error. However when changing the color of the link to have a color-contrast problem (such as color: #333) an error is not reported.
<html>
<head>
<style>
body {
/* If 0px - 8px, color contrast problem identified; no problem when >= 9px */
height: 5px;
background: #161616;
}
a {
color: #333;
}
</style>
</head>
<body>
<a href="https://link/">This is a link - test 38</a>
</body>
</html>
straker
dismissed
their stale review
I had an older version of the code that didn't work. works with most up-to-date version
|
Should we add a test to verify that we don't break the check in either direction? So works when there is a color-contrast problem (test we don't have) and when there isn't (test we have now). |
|
I believe in this case, I'm just testing that an element returns the correct background stack. An element that visually intersects with I suppose we could have a test for the scenario of when an element does not intersect with the body and |
|
|
||
| if ( | ||
| // Check that the body background is the page's background | ||
| bodyIndex > 1 && // only if there are negative z-index elements | ||
| sortBodyElement && |
There was a problem hiding this comment.
Shouldn't this just be bodyIndex !== 0, since bodyIndex can never be less than -1.
There was a problem hiding this comment.
bodyIndex can be -1 in some specific scenarios. See https://jsfiddle.net/o3hjdsy8/1/
| assert.closeTo(actual.red, 0, 0); | ||
| assert.closeTo(actual.green, 0, 0); | ||
| assert.closeTo(actual.blue, 0, 0); | ||
| assert.closeTo(actual.alpha, 1, 0); |
There was a problem hiding this comment.
I don't think this should end up black (see my comment above). It looks to me like even with the body height changed, the background color is still the background color of the body.
There was a problem hiding this comment.
I'm not sure I understand here. See above comments for clarification.
scurker
dismissed
WilcoFiers’s stale review
@WilcoFiers Added some comments, so this will likely need a re-review.
scurker
dismissed
WilcoFiers’s stale review
@wilco Swapped around the logic and added some general comments. Hopefully that will help give context for future viewers.
WilcoFiers
changed the title
sortPageBackgrounddid not account for instances where an element did not visually intersect withdocument.bodybut should inherit the background color from the canvas background (<html>).According to the CSS3 Background Spec, the document canvas should propagate the background color of the
<body>element when all of the following are true:<html>element hasbackground-image: none<html>element hasbackground-color: transparentThe following CodePen provides a case where axe shows a false positive due to the above missing check.
bodyas part of the element stack when element does not intersect with body#fixtureelement in test template as it was interfering with the layout of elements due to to the Mocha results being dynamic.#fixture, the static display of#mocha-statswas interfering with a few tests.I opted to go the route of moving the
#fixtureand resolving the breaking tests, as the alternatives I explored seemed dirty and hacky.Closes issue: #1456
Reviewer checks
Required fields, to be filled out by PR reviewer(s)