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): account for elements that do not fill entire bounding size #3186

Merged
merged 2 commits into from
Oct 6, 2021
Merged

fix(color-contrast): account for elements that do not fill entire bounding size #3186

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df72c7f
fix(color-contrast): account for elements that do not fill entire bou…
straker Oct 5, 2021
6aa57ad
not private
straker Oct 6, 2021
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
45 changes: 30 additions & 15 deletions lib/commons/dom/get-rect-stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,8 @@ function addNodeToGrid(grid, vNode) {
const endRow = ((y + rect.height) / gridSize) | 0;
const endCol = ((x + rect.width) / gridSize) | 0;

grid._numCols = Math.max(grid._numCols ?? 0, endCol);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little off. If _numCols needs to be different, it should be fixed where that value is set. If it doesn't need to be changed, this should probably just be a local variable.

Copy link
Contributor Author

@straker straker Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It needs to be set for every grid, so can't be local. Grid items are added dynamically, so there's no way to know how many rows or columns the grid will have before hand, we can only know as each item is added and changes the grid size.


for (let row = startRow; row <= endRow; row++) {
grid.cells[row] = grid.cells[row] || [];

Expand Down Expand Up @@ -475,21 +477,34 @@ export function getRectStack(grid, rect, recursed = false) {
// went with pixel perfect collision rather than rounding
const row = (y / gridSize) | 0;
const col = (x / gridSize) | 0;
let stack = grid.cells[row][col].filter(gridCellNode => {
return gridCellNode.clientRects.find(clientRect => {
const rectX = clientRect.left;
const rectY = clientRect.top;

// perform an AABB (axis-aligned bounding box) collision check for the
// point inside the rect
return (
x <= rectX + clientRect.width &&
x >= rectX &&
y <= rectY + clientRect.height &&
y >= rectY
);
});
});

// we're making an assumption that there cannot be an element in the
// grid which escapes the grid bounds. For example, if the grid is 4x4 there
// can't be an element whose midpoint is at column 5. If this happens this
// means there's an error in our grid logic that needs to be fixed
if (row > grid.cells.length || col > grid._numCols) {
straker marked this conversation as resolved.
Show resolved Hide resolved
throw new Error('Element midpoint exceeds the grid bounds');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow why you would want this to throw. Crashing axe-core doesn't seem like the right solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I want to know when our custom grid logic fails, and it shows there's an issue that needs to be fixed. Without throwing, color contrast would hide issues that should be fixed.

}

// it is acceptable if a row has empty cells due to client rects not filling
// the entire bounding rect of an element
// @see https://github.com/dequelabs/axe-core/issues/3166
let stack =
grid.cells[row][col]?.filter(gridCellNode => {
return gridCellNode.clientRects.find(clientRect => {
const rectX = clientRect.left;
const rectY = clientRect.top;

// perform an AABB (axis-aligned bounding box) collision check for the
// point inside the rect
return (
x <= rectX + clientRect.width &&
x >= rectX &&
y <= rectY + clientRect.height &&
y >= rectY
);
});
}) ?? [];

const gridContainer = grid.container;
if (gridContainer) {
Expand Down
121 changes: 83 additions & 38 deletions test/checks/color/color-contrast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,13 +358,55 @@ describe('color-contrast', function() {
);
});

describe('with pseudo elements', function () {
it('should not error if client rects do not fill entire bounding rect', function() {
var params = checkSetup(
'<pre style="overflow-x: auto; background-color: #333"><span id="target" style="color: #000">' +
'\nx x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x ' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\nx' +
'\n</span></pre>'
);
assert.doesNotThrow(function() {
contrastEvaluate.apply(checkContext, params);
});
});

describe('with pseudo elements', function() {
it('should return undefined if :before pseudo element has a background color', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 100%; height: 100%; background: red; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isUndefined(contrastEvaluate.apply(checkContext, params));
assert.deepEqual(checkContext._data, {
messageKey: 'pseudoContent'
Expand All @@ -374,13 +416,13 @@ describe('color-contrast', function() {
document.querySelector('#background')
);
});

it('should return undefined if :after pseudo element has a background color', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:after { content: ""; position: absolute; width: 100%; height: 100%; background: red; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isUndefined(contrastEvaluate.apply(checkContext, params));
assert.deepEqual(checkContext._data, {
messageKey: 'pseudoContent'
Expand All @@ -390,21 +432,21 @@ describe('color-contrast', function() {
document.querySelector('#background')
);
});

it('should return undefined if pseudo element has a background image', function() {
var dataURI =
'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/' +
'XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkA' +
'ABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKU' +
'E1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 100%; height: 100%; background: url(' +
dataURI +
') no-repeat left center; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isUndefined(contrastEvaluate.apply(checkContext, params));
assert.deepEqual(checkContext._data, {
messageKey: 'pseudoContent'
Expand All @@ -414,62 +456,62 @@ describe('color-contrast', function() {
document.querySelector('#background')
);
});

it('should not return undefined if pseudo element has no content', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { position: absolute; width: 100%; height: 100%; background: red; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it('should not return undefined if pseudo element is not absolutely positioned no content', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; width: 100%; height: 100%; background: red; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it('should not return undefined if pseudo element is has zero dimension', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 0; height: 100%; background: red; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it("should not return undefined if pseudo element doesn't have a background", function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 100%; height: 100%; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it('should not return undefined if pseudo element has visibility: hidden', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 100%; height: 100%; background-color: red; visibility: hidden; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it('should not return undefined if pseudo element has display: none', function() {
var params = checkSetup(
'<style>.foo { position: relative; } .foo:before { content: ""; position: absolute; width: 100%; height: 100%; background-color: red; display: none; }</style>' +
'<div id="background" class="foo"><p id="target" style="#000">Content</p></div>'
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

it('should return undefined if pseudo element is more than 25% of the element', function () {
it('should return undefined if pseudo element is more than 25% of the element', function() {
var params = checkSetup(
'<style>.foo { position: relative; width: 100px; height: 100px; } ' +
'.foo:before { content: ""; position: absolute; width: 26px; height: 100px; background: red; }</style>' +
Expand All @@ -478,7 +520,7 @@ describe('color-contrast', function() {
assert.isUndefined(contrastEvaluate.apply(checkContext, params));
});

it('should not return undefined if pseudo element is 25% of the element', function () {
it('should not return undefined if pseudo element is 25% of the element', function() {
var params = checkSetup(
'<style>.foo { position: relative; width: 100px; height: 100px; } ' +
'.foo:before { content: ""; position: absolute; width: 25px; height: 100px; background: red; }</style>' +
Expand All @@ -487,17 +529,20 @@ describe('color-contrast', function() {
assert.isTrue(contrastEvaluate.apply(checkContext, params));
});

(isIE11 ? it : xit)('should return undefined if the unit is not in px', function () {
var params = checkSetup(
'<style>.foo { position: relative; } ' +
'.foo:before { content: ""; position: absolute; width: 25%; height: 100%; background: red; }</style>' +
'<p id="target" class="foo">Content</p>'
);
assert.isUndefined(contrastEvaluate.apply(checkContext, params));
});
(isIE11 ? it : xit)(
'should return undefined if the unit is not in px',
function() {
var params = checkSetup(
'<style>.foo { position: relative; } ' +
'.foo:before { content: ""; position: absolute; width: 25%; height: 100%; background: red; }</style>' +
'<p id="target" class="foo">Content</p>'
);
assert.isUndefined(contrastEvaluate.apply(checkContext, params));
}
);
});

describe('with special texts', function () {
describe('with special texts', function() {
it('should return undefined for a single character text with insufficient contrast', function() {
var params = checkSetup(
'<div style="background-color: #FFF;">' +
Expand Down Expand Up @@ -582,7 +627,7 @@ describe('color-contrast', function() {
});
});

describe('options', function () {
describe('options', function() {
it('should support options.boldValue', function() {
var params = checkSetup(
'<div style="color: gray; background-color: white; font-size: 14pt; font-weight: 100" id="target">' +
Expand Down Expand Up @@ -732,24 +777,24 @@ describe('color-contrast', function() {
ignorePseudo: true
}
);

assert.isTrue(contrastEvaluate.apply(checkContext, params));
});
it('should adjust the pseudo element minimum size with the options.pseudoSizeThreshold', function () {

it('should adjust the pseudo element minimum size with the options.pseudoSizeThreshold', function() {
var params = checkSetup(
'<style>.foo { position: relative; width: 100px; height: 100px }' +
'.foo:before { content: ""; position: absolute; width: 22%; height: 100%; background: red; }</style>' +
'<style>.foo { position: relative; width: 100px; height: 100px }' +
'.foo:before { content: ""; position: absolute; width: 22%; height: 100%; background: red; }</style>' +
'<p id="target" class="foo">Content</p>',
{
pseudoSizeThreshold: 0.20
pseudoSizeThreshold: 0.2
}
);
assert.isUndefined(contrastEvaluate.apply(checkContext, params));
});
});

describe('with shadowDOM', function () {
describe('with shadowDOM', function() {
(shadowSupported ? it : xit)(
'returns colors across Shadow DOM boundaries',
function() {
Expand Down
40 changes: 40 additions & 0 deletions test/commons/dom/get-element-stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,46 @@ describe('dom.getElementStack', function() {
assert.deepEqual(stack, []);
});

it('should throw error if element midpoint-x exceeds the grid', function() {
fixture.innerHTML = '<div id="target">Hello World</div>';
axe.testUtils.flatTreeSetup(fixture);
var target = fixture.querySelector('#target');
var vNode = axe.utils.getNodeFromTree(target);
Object.defineProperty(vNode, 'boundingClientRect', {
get: function() {
return {
left: 0,
top: 10,
width: 10000,
height: 10
};
}
});
assert.throws(function() {
getElementStack(target);
}, 'Element midpoint exceeds the grid bounds');
});

it('should throw error if element midpoint-y exceeds the grid', function() {
fixture.innerHTML = '<div id="target">Hello World</div>';
axe.testUtils.flatTreeSetup(fixture);
var target = fixture.querySelector('#target');
var vNode = axe.utils.getNodeFromTree(target);
Object.defineProperty(vNode, 'boundingClientRect', {
get: function() {
return {
left: 0,
top: 10,
width: 10,
height: 10000
};
}
});
assert.throws(function() {
getElementStack(target);
}, 'Element midpoint exceeds the grid bounds');
});

// IE11 either only supports clip paths defined by url() or not at all,
// MDN and caniuse.com give different results...
(isIE11 ? it.skip : it)(
Expand Down