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.

Sign up for GitHub

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
Draft

fix(color-contrast): ignore overlapping elements that do not interfere with the background color #3583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5492053
fix(color-contrast): ignore overlapping elements that do not interfer…
straker Aug 4, 2022
3532700
revert getElementSpec change
straker Aug 4, 2022
66a098f
fix ie11
straker Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 6 additions & 63 deletions lib/checks/color/color-contrast-evaluate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@ import {
getForegroundColor,
incompleteData,
getContrast,
getOwnBackgroundColor,
getTextShadowColors,
flattenShadowColors
} from '../../commons/color';
import { memoize } from '../../core/utils';
import { findPseudoElement } from '../../commons/dom';

export default function colorContrastEvaluate(node, options, virtualNode) {
const {
Expand Down Expand Up @@ -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.

shadowOutlineEmMax,
ignorePseudo,
pseudoSizeThreshold
});
const fgColor = getForegroundColor(node, false, bgColor);
// Thin shadows only. Thicker shadows are included in the background instead
const shadowColors = getTextShadowColors(node, {
Expand Down Expand Up @@ -162,70 +165,10 @@ export default function colorContrastEvaluate(node, options, virtualNode) {
return isValid;
}

function findPseudoElement(
vNode,
{ pseudoSizeThreshold = 0.25, ignorePseudo = false }
) {
if (ignorePseudo) {
return;
}
const rect = vNode.boundingClientRect;
const minimumSize = rect.width * rect.height * pseudoSizeThreshold;
do {
const beforeSize = getPseudoElementArea(vNode.actualNode, ':before');
const afterSize = getPseudoElementArea(vNode.actualNode, ':after');
if (beforeSize + afterSize > minimumSize) {
return vNode; // Combined area of before and after exceeds the minimum size
}
} while ((vNode = vNode.parent));
}

const getPseudoElementArea = memoize(function getPseudoElementArea(
node,
pseudo
) {
const style = window.getComputedStyle(node, pseudo);
const matchPseudoStyle = (prop, value) =>
style.getPropertyValue(prop) === value;
if (
matchPseudoStyle('content', 'none') ||
matchPseudoStyle('display', 'none') ||
matchPseudoStyle('visibility', 'hidden') ||
matchPseudoStyle('position', 'absolute') === false
) {
return 0; // The pseudo element isn't visible
}

if (
getOwnBackgroundColor(style).alpha === 0 &&
matchPseudoStyle('background-image', 'none')
) {
return 0; // There is no background
}

// Find the size of the pseudo element;
const pseudoWidth = parseUnit(style.getPropertyValue('width'));
const pseudoHeight = parseUnit(style.getPropertyValue('height'));
if (pseudoWidth.unit !== 'px' || pseudoHeight.unit !== 'px') {
// IE doesn't normalize to px. Infinity gets everything to undefined
return pseudoWidth.value === 0 || pseudoHeight.value === 0 ? 0 : Infinity;
}
return pseudoWidth.value * pseudoHeight.value;
});

function textIsEmojis(visibleText) {
const options = { nonBmp: true };
const hasUnicodeChars = hasUnicode(visibleText, options);
const hasNonUnicodeChars =
sanitize(removeUnicode(visibleText, options)) === '';
return hasUnicodeChars && hasNonUnicodeChars;
}

function parseUnit(str) {
const unitRegex = /^([0-9.]+)([a-z]+)$/i;
const [, value = '', unit = ''] = str.match(unitRegex) || [];
return {
value: parseFloat(value),
unit: unit.toLowerCase()
};
}
12 changes: 10 additions & 2 deletions lib/commons/color/get-background-color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,27 @@ import visuallyContains from '../dom/visually-contains';
* @param {Element} elm Element to determine background color
* @param {Array} [bgElms=[]] elements to inspect
* @param {Number} shadowOutlineEmMax Thickness of `text-shadow` at which it becomes a background color
* @param {Object} options
* @deprecated shadowOutlineEmMax parameter. Pass an object instead with shadowOutlineEmMax as property.
* @returns {Color}
*/
export default function getBackgroundColor(
elm,
bgElms = [],
shadowOutlineEmMax = 0.1
shadowOutlineEmMax,
options
) {
if (['undefined', 'object'].includes(typeof shadowOutlineEmMax)) {
options = shadowOutlineEmMax ?? {};
shadowOutlineEmMax = options.shadowOutlineEmMax ?? 0.1;
}

let bgColors = getTextShadowColors(elm, { minRatio: shadowOutlineEmMax });
if (bgColors.length) {
bgColors = [{ color: bgColors.reduce(flattenShadowColors) }];
}

const elmStack = getBackgroundStack(elm);
const elmStack = getBackgroundStack(elm, options);

// Search the stack until we have an alpha === 1 background
(elmStack || []).some(bgElm => {
Expand Down
165 changes: 105 additions & 60 deletions lib/commons/color/get-background-stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,50 +3,37 @@ import elementHasImage from './element-has-image';
import getOwnBackgroundColor from './get-own-background-color';
import incompleteData from './incomplete-data';
import reduceToElementsBelowFloating from '../dom/reduce-to-elements-below-floating';
import visibleTextNodes from '../text/visible-text-nodes';
import findPseudoElement from '../dom/find-pseudo-element';
import { getNodeFromTree } from '../../core/utils';

/**
* Determine if element B is an inline descendant of A
* @private
* @param {Element} node
* @param {Element} descendant
* @return {Boolean}
* Get all elements rendered underneath the current element,
* In the order they are displayed (front to back)
*
* @method getBackgroundStack
* @memberof axe.commons.color
* @param {Element} elm
* @param {Object} options
* @return {Array}
*/
function isInlineDescendant(node, descendant) {
const CONTAINED_BY = Node.DOCUMENT_POSITION_CONTAINED_BY;
// eslint-disable-next-line no-bitwise
if (!(node.compareDocumentPosition(descendant) & CONTAINED_BY)) {
return false;
}
const style = window.getComputedStyle(descendant);
const display = style.getPropertyValue('display');
if (!display.includes('inline')) {
return false;
export default function getBackgroundStack(elm, options) {
let elmStack = filteredRectStack(elm);

if (elmStack === null) {
return null;
}
// IE needs this; It doesn't set display:block when position is set
const position = style.getPropertyValue('position')
return position === 'static';
}
elmStack = reduceToElementsBelowFloating(elmStack, elm);
elmStack = sortPageBackground(elmStack);

/**
* Determine if the element obscures / overlaps with the text
* @private
* @param {Number} elmIndex
* @param {Array} elmStack
* @param {Element} originalElm
* @return {Number|undefined}
*/
function calculateObscuringElement(elmIndex, elmStack, originalElm) {
// Reverse order, so that we can safely splice
for (let i = elmIndex - 1; i >= 0; i--) {
if (!isInlineDescendant(originalElm, elmStack[i])) {
return true;
}
// Ignore inline descendants, for example:
// <p>text <img></p>; We don't care about the <img> element,
// since it does not overlap the text inside of <p>
elmStack.splice(i, 1);
// Return all elements BELOW the current element, null if the element is undefined
const elmIndex = elmStack.indexOf(elm);
if (calculateObscuringElement(elmIndex, elmStack, elm, options)) {
// if the total of the elements above our element results in total obscuring, return null
incompleteData.set('bgColor', 'bgOverlap');
return null;
}
return false;
return elmIndex !== -1 ? elmStack : null;
}

/**
Expand Down Expand Up @@ -95,31 +82,89 @@ function sortPageBackground(elmStack) {
}

/**
* Get all elements rendered underneath the current element,
* In the order they are displayed (front to back)
*
* @method getBackgroundStack
* @memberof axe.commons.color
* @param {Element} elm
* @return {Array}
* Determine if the element obscures / overlaps with the text
* @private
* @param {Number} elmIndex
* @param {Array} elmStack
* @param {Element} originalElm
* @param {Object} options
* @return {Number|undefined}
*/
function getBackgroundStack(elm) {
let elmStack = filteredRectStack(elm);

if (elmStack === null) {
return null;
function calculateObscuringElement(elmIndex, elmStack, originalElm, options) {
// Reverse order, so that we can safely splice
for (let i = elmIndex - 1; i >= 0; i--) {
if (
!isInlineDescendant(originalElm, elmStack[i]) &&
!isEmptyElement(elmStack[i], options)
) {
return true;
}
// Ignore inline descendants, for example:
// <p>text <img></p>; We don't care about the <img> element,
// since it does not overlap the text inside of <p>
elmStack.splice(i, 1);
}
elmStack = reduceToElementsBelowFloating(elmStack, elm);
elmStack = sortPageBackground(elmStack);
return false;
}

// Return all elements BELOW the current element, null if the element is undefined
const elmIndex = elmStack.indexOf(elm);
if (calculateObscuringElement(elmIndex, elmStack, elm)) {
// if the total of the elements above our element results in total obscuring, return null
incompleteData.set('bgColor', 'bgOverlap');
return null;
/**
* Determine if element B is an inline descendant of A
* @private
* @param {Element} node
* @param {Element} descendant
* @return {Boolean}
*/
function isInlineDescendant(node, descendant) {
const CONTAINED_BY = Node.DOCUMENT_POSITION_CONTAINED_BY;
// eslint-disable-next-line no-bitwise
if (!(node.compareDocumentPosition(descendant) & CONTAINED_BY)) {
return false;
}
return elmIndex !== -1 ? elmStack : null;
const style = window.getComputedStyle(descendant);
const display = style.getPropertyValue('display');
if (!display.includes('inline')) {
return false;
}
// IE needs this; It doesn't set display:block when position is set
const position = style.getPropertyValue('position');
return position === 'static';
}

export default getBackgroundStack;
/**
* Determine if an element is empty and would not contribute to the background color
* @see https://github.com/dequelabs/axe-core/issues/3464
* @private
* @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?

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.

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.

pseudoSizeThreshold,
ignorePseudo,
recurse: false
});
// IE11 does not support border, border-width, or
// outline computed shorthand properties
const hasBorder = [
'border-bottom-width',
'border-top-width',
'border-left-width',
'border-right-width',
'outline-width'
].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.

bgColor.alpha === 0 &&
!elementHasImage(node, style) &&
!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.

}

return false;
}
71 changes: 71 additions & 0 deletions lib/commons/dom/find-pseudo-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import memoize from '../../core/utils/memoize';
import getOwnBackgroundColor from '../color/get-own-background-color';

/**
* Find the pseudo element of node that meets the minimum size threshold.
* @method findPseudoElement
* @memberof axe.commons.dom
* @instance
* @param {VirtualNode} vNode The VirtualNode to find pseudo elements for
* @param {Object} options
* @returns {VirtualNode|undefined} The VirtualNode which has matching pseudo elements.
*/
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.

) {
if (ignorePseudo) {
return;
}
const rect = vNode.boundingClientRect;
const minimumSize = rect.width * rect.height * pseudoSizeThreshold;
do {
const beforeSize = getPseudoElementArea(vNode.actualNode, ':before');
const afterSize = getPseudoElementArea(vNode.actualNode, ':after');
if (beforeSize + afterSize > minimumSize) {
return vNode; // Combined area of before and after exceeds the minimum size
}
} while (recurse && (vNode = vNode.parent));
}

const getPseudoElementArea = memoize(function getPseudoElementArea(
node,
pseudo
) {
const style = window.getComputedStyle(node, pseudo);
const matchPseudoStyle = (prop, value) =>
style.getPropertyValue(prop) === value;
if (
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.

) {
return 0; // The pseudo element isn't visible
}

if (
getOwnBackgroundColor(style).alpha === 0 &&
matchPseudoStyle('background-image', 'none')
) {
return 0; // There is no background
}

// Find the size of the pseudo element;
const pseudoWidth = parseUnit(style.getPropertyValue('width'));
const pseudoHeight = parseUnit(style.getPropertyValue('height'));
if (pseudoWidth.unit !== 'px' || pseudoHeight.unit !== 'px') {
// IE doesn't normalize to px. Infinity gets everything to undefined
return pseudoWidth.value === 0 || pseudoHeight.value === 0 ? 0 : Infinity;
}
return pseudoWidth.value * pseudoHeight.value;
});

function parseUnit(str) {
const unitRegex = /^([0-9.]+)([a-z]+)$/i;
const [, value = '', unit = ''] = str.match(unitRegex) || [];
return {
value: parseFloat(value),
unit: unit.toLowerCase()
};
}
1 change: 1 addition & 0 deletions lib/commons/dom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* @memberof axe.commons
*/
export { default as findElmsInContext } from './find-elms-in-context';
export { default as findPseudoElement } from './find-pseudo-element';
export { default as findUpVirtual } from './find-up-virtual';
export { default as findUp } from './find-up';
export { default as getComposedParent } from './get-composed-parent';
Expand Down
Loading