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(color-contrast): ignore overlapping elements that do not interfere with the background color #3583

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 4, 2022

This should drastically decrease the number of overlapping element incompletes axe-core generates, especially for anyone who uses Material Button on their site. I moved findPseudoElement to it's own function so it could be used in both color-contrast-evaluate and calculateObscuringElement.

I also moved getBackgroundStack to the top to match our default export style. The main parts of that file change are the addition of isEmptyElement that goes with the isInlineDescendant check.

Closes issue: #3464

@straker straker requested a review from a team as a code owner August 4, 2022 16:47
@@ -72,7 +71,11 @@ export default function colorContrastEvaluate(node, options, virtualNode) {
}

const bgNodes = [];
const bgColor = getBackgroundColor(node, bgNodes, shadowOutlineEmMax);
const bgColor = getBackgroundColor(node, bgNodes, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update linkInTextBlock, it uses getBackgroundColor too.

*/
export default function findPseudoElement(
vNode,
{ pseudoSizeThreshold = 0.25, ignorePseudo = false, recurse = true } = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

ignorePseudo should definitely not be part of this if we put it in a commons. Doesn't make sense to pass in an option who's only job is to return undefined.

* @param {Element} node
* @param {Object} options
*/
function isEmptyElement(node, { pseudoSizeThreshold, ignorePseudo }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do to our performance?

* @param {Element} node
* @param {Object} options
*/
function isEmptyElement(node, { pseudoSizeThreshold, ignorePseudo }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are at least a few more ways in which something that looks empty may not be; filter, box-shadow, list-style, and overflow:scroll (to create a scrollbar). Don't know if that last one needs to be there but the others, probably. Not sure if that's all of them either. I went over a list of all CSS properties to try and come up with these.

].some(prop => parseInt(vNode.getComputedStylePropertyValue(prop), 10) !== 0);

if (
visibleTextNodes(vNode).length === 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking for text and images, should we just abort if this thing has child nodes? Seems a little safer, and potentially faster too.

!hasPseudoElement &&
!hasBorder
) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid calculating things that we don't end up needing. Now, if there is text, we're still computing styles & pseudo elements. That's not necessary if we returned false as soon as we found text content.

matchPseudoStyle('content', 'none') ||
matchPseudoStyle('display', 'none') ||
matchPseudoStyle('visibility', 'hidden') ||
matchPseudoStyle('position', 'absolute') === false
Copy link
Contributor

Choose a reason for hiding this comment

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

This line works well enough when this was part of the color-contrast-evaluate method, but I'm not so sure about this now that it's in the commons.

const vNode = getNodeFromTree(node);
const style = window.getComputedStyle(node);
const bgColor = getOwnBackgroundColor(style);
const hasPseudoElement = !!findPseudoElement(vNode, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method doesn't seem right here. For example, if the pseudo element isn't position:absolute, hasPsuedoElement would become false, even if there's a background color, or content, or a bunch of other things on that.

I'm not sure how to fix that, except perhaps maybe have this be "true" unless content:none is true for both :before and :after? I'm not sure that findPseudoElement should have been added to commons. There are a bunch of things it does that make sense for inside color-contrast-evaluate, but that create problematic gotchas if used in other places.

var vNode = queryFixture(
'<style>.parent' +
pseudoElm +
'{ content: ""; width: 100%; height: 100px; background: red; position: absolute; }</style><div class="parent"><div><div id="target" style="height: 100px;"></div></div></div>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind putting in a line break into these longer lines?

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