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): improve speed and accuracy of code blocks with syntax highlighting #2003

Merged
merged 6 commits into from
Jan 31, 2020
Merged

fix(color-contrast): improve speed and accuracy of code blocks with syntax highlighting #2003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e9173b
fix(color-contrast): improve speed and accuracy of code block with sy…
straker Jan 28, 2020
4534eb8
add integration test
straker Jan 28, 2020
872c073
figure out why firefox fails on opacity, but just in circle (works in…
straker Jan 28, 2020
f0abe4e
try again
straker Jan 28, 2020
f4c9b25
fix tests
straker Jan 29, 2020
76c5766
comment
straker Jan 29, 2020
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
10 changes: 5 additions & 5 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 @@ -140,15 +140,15 @@ color.filteredRectStack = function filteredRectStack(elm) {
*/
color.getRectStack = function(elm) {
const boundingStack = axe.commons.dom.getElementStack(elm);
let rects = Array.from(elm.getClientRects());

// Handle inline elements spanning multiple lines to be evaluated
const filteredArr = axe.commons.dom.getTextElementStack(elm);

// If the element does not have multiple rects, like for display:block, return a single stack
if (!rects || rects.length <= 1) {
if (!filteredArr || filteredArr.length <= 1) {
return [boundingStack];
}

// Handle inline elements spanning multiple lines to be evaluated
let filteredArr = axe.commons.dom.getClientElementStack(elm);

if (filteredArr.some(stack => stack === undefined)) {
// Can be happen when one or more of the rects sits outside the viewport
return null;
Expand Down
49 changes: 40 additions & 9 deletions lib/commons/dom/get-element-stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,10 @@ function addNodeToGrid(grid, vNode) {

for (let col = startCol; col <= endCol; col++) {
grid.cells[row][col] = grid.cells[row][col] || [];
grid.cells[row][col].push(vNode);

if (!grid.cells[row][col].includes(vNode)) {
grid.cells[row][col].push(vNode);
}
}
}
});
Expand Down Expand Up @@ -452,10 +455,10 @@ function getRectStack(grid, rect, recursed = false) {
// 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
x <= rectX + clientRect.width &&
x >= rectX &&
y <= rectY + clientRect.height &&
y >= rectY
);
});
});
Expand Down Expand Up @@ -506,13 +509,13 @@ dom.getElementStack = function(node) {
};

/**
* Return all elements that are at the center of each client rect of the passed in node.
* @method getClientElementStack
* Return all elements that are at the center of each text client rect of the passed in node.
* @method getTextElementStack
* @memberof axe.commons.dom
* @param {Node} node
* @return {Array<Node[]>}
*/
dom.getClientElementStack = function(node) {
dom.getTextElementStack = function(node) {
if (!axe._cache.get('gridCreated')) {
createGrid();
axe._cache.set('gridCreated', true);
Expand All @@ -525,7 +528,35 @@ dom.getClientElementStack = function(node) {
return [];
}

const clientRects = vNode.clientRects;
// for code blocks that use syntax highlighting, you can get a ton of client
// rects (See https://github.com/dequelabs/axe-core/issues/1985). they use
// a mixture of text nodes and other nodes (which will contain their own text
// nodes), but all we care about is checking the direct text nodes as the
// other nodes will have their own client rects checked. doing this speeds up
// color contrast significantly for large syntax highlighted code blocks
const clientRects = [];
Array.from(node.childNodes).forEach(elm => {
if (
elm.nodeType === 3 &&
axe.commons.text.sanitize(elm.textContent) !== ''
) {
const range = document.createRange();
range.selectNodeContents(elm);
const rects = range.getClientRects();

for (let i = 0; i < rects.length; i++) {
const rect = rects[i];

// filter out 0 width and height rects (newline characters)
// ie11 has newline characters return 0.00998, so we'll say if the
// line is < 1 it shouldn't be counted
if (rect.width >= 1 && rect.height >= 1) {
clientRects.push(rect);
}
}
}
});

return clientRects.map(rect => {
return getRectStack(grid, rect);
});
Expand Down
18 changes: 9 additions & 9 deletions test/commons/color/get-foreground-color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ describe('color.getForegroundColor', function() {

it('should return the blended color if it has alpha set', function() {
fixture.innerHTML =
'<div style="height: 40px; width: 30px; background-color: #800000;">' +
'<div id="target" style="height: 20px; width: 15px; color: rgba(0, 0, 128, 0.5);' +
'<div style="height: 40px; background-color: #800000;">' +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update these tests as my code changes made them more accurate than before. Before my changes, we would test only the first line of this fixture. However, if we look at the result of the fixture, we can see that there should be 3 background colors and 3 foreground colors, but we only calculate 1.

image

So I updated the tests to just focus on the single color with alpha/opacity.

'<div id="target" style="height: 40px; color: rgba(0, 0, 128, 0.5);' +
' background-color: rgba(0, 128, 0, 0.5);">' +
'This is my text' +
'</div></div>';
Expand All @@ -31,8 +31,8 @@ describe('color.getForegroundColor', function() {

it('should return the blended color if it has opacity set', function() {
fixture.innerHTML =
'<div style="height: 40px; width: 30px; background-color: #800000;">' +
'<div id="target" style="height: 20px; width: 15px; color: #000080;' +
'<div style="height: 40px; background-color: #800000;">' +
'<div id="target" style="height: 40px; color: #000080;' +
' background-color: green; opacity: 0.5;">' +
'This is my text' +
'</div></div>';
Expand All @@ -49,8 +49,8 @@ describe('color.getForegroundColor', function() {
it('should take into account parent opacity tree', function() {
fixture.innerHTML =
'<div style="background-color: #fafafa">' +
'<div style="height: 40px; width: 30px; opacity: 0.6">' +
'<div id="target" style="height: 20px; width: 15px; color: rgba(0, 0, 0, 0.87);">' +
'<div style="height: 40px; opacity: 0.6">' +
'<div id="target" style="height: 40px; color: rgba(0, 0, 0, 0.87);">' +
'This is my text' +
'</div></div></div>';
axe.testUtils.flatTreeSetup(fixture);
Expand All @@ -66,9 +66,9 @@ describe('color.getForegroundColor', function() {
it('should take into account entire parent opacity tree', function() {
fixture.innerHTML =
'<div style="background-color: #fafafa">' +
'<div style="height: 40px; width: 30px; opacity: 0.75">' +
'<div style="height: 40px; width: 30px; opacity: 0.8">' +
'<div id="target" style="height: 20px; width: 15px; color: rgba(0, 0, 0, 0.87);">' +
'<div style="height: 40px; opacity: 0.75">' +
'<div style="height: 40px; opacity: 0.8">' +
'<div id="target" style="height: 40px; color: rgba(0, 0, 0, 0.87);">' +
'This is my text' +
'</div></div></div></div>';
axe.testUtils.flatTreeSetup(fixture);
Expand Down
35 changes: 23 additions & 12 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 @@ -3,7 +3,7 @@ describe('dom.getElementStack', function() {

var fixture = document.getElementById('fixture');
var getElementStack = axe.commons.dom.getElementStack;
var getClientElementStack = axe.commons.dom.getClientElementStack;
var getTextElementStack = axe.commons.dom.getTextElementStack;
var isIE11 = axe.testUtils.isIE11;
var shadowSupported = axe.testUtils.shadowSupport.v1;

Expand Down Expand Up @@ -247,7 +247,7 @@ describe('dom.getElementStack', function() {
it('should not return elements that do not fully cover the target', function() {
fixture.innerHTML =
'<div id="1" style="position:relative;">' +
'<div id="2" style="position:absolute;width:300px;height:20px;"></div>' +
'<div id="2" style="position:absolute;width:300px;height:19px;"></div>' +
'<p id="target" style="position:relative;z-index:1;width:300px;height:40px;">Text oh heyyyy <a href="#" id="target">and here\'s <br>a link</a></p>' +
'</div>';
axe.testUtils.flatTreeSetup(fixture);
Expand Down Expand Up @@ -491,21 +491,32 @@ describe('dom.getElementStack', function() {
});
});

describe('dom.getClientElementStack', function() {
it('should return array of client rects', function() {
describe('dom.getTextElementStack', function() {
it('should return array of client text rects', function() {
fixture.innerHTML =
'<main id="1">' +
'<span id="target">' +
'<span id="2">Hello</span><br/><span id="3">World</span>' +
'</span>' +
'<div id="target">' +
'<span id="2">Hello</span><br/>World' +
'</div>' +
'</main>';
axe.testUtils.flatTreeSetup(fixture);
var target = fixture.querySelector('#target');
var stacks = getTextElementStack(target).map(mapToIDs);
assert.deepEqual(stacks, [['target', '1', 'fixture']]);
});

it('should ignore newline characters', function() {
fixture.innerHTML =
'<main id="1">' +
'<div id="target">' +
'<span id="2">Hello</span><br/>\n' +
'World' +
'</div>' +
'</main>';
axe.testUtils.flatTreeSetup(fixture);
var target = fixture.querySelector('#target');
var stacks = getClientElementStack(target).map(mapToIDs);
assert.deepEqual(stacks, [
['2', 'target', '1', 'fixture'],
['3', 'target', '1', 'fixture']
]);
var stacks = getTextElementStack(target).map(mapToIDs);
assert.deepEqual(stacks, [['target', '1', 'fixture']]);
});
});
});
76 changes: 76 additions & 0 deletions test/integration/full/contrast/code-highlighting.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<!DOCTYPE html>
<html lang="en">
<head>
<title>Test Page</title>
<link
rel="stylesheet"
type="text/css"
href="/node_modules/mocha/mocha.css"
/>
<link
rel="stylesheet"
href="https://cdnjs.cloudflare.com/ajax/libs/prism/1.17.1/themes/prism.min.css"
/>
<link
rel="stylesheet"
href="https://cdnjs.cloudflare.com/ajax/libs/prism/1.17.1/themes/prism-okaidia.min.css"
/>
<script src="/node_modules/mocha/mocha.js"></script>
<script src="/node_modules/chai/chai.js"></script>
<script src="/axe.js"></script>
<script>
mocha.setup({
timeout: 10000,
ui: 'bdd'
});
var assert = chai.assert;
</script>
<script src="/test/integration/no-ui-reporter.js"></script>
<style></style>
</head>

<body>
<div id="fixture">
<pre>
<code class="language-html">&lt;!DOCTYPE html&gt;
&lt;html lang="en"&gt;
&lt;head&gt;
&lt;title&gt;Test Page&lt;/title&gt;
&lt;link
rel="stylesheet"
type="text/css"
href="/node_modules/mocha/mocha.css"
/&gt;
&lt;script src="/node_modules/mocha/mocha.js"&gt;&lt;/script&gt;
&lt;script src="/node_modules/chai/chai.js"&gt;&lt;/script&gt;
&lt;script src="/axe.js"&gt;&lt;/script&gt;
&lt;script&gt;
mocha.setup({
timeout: 10000,
ui: 'bdd'
});
var assert = chai.assert;
&lt;/script&gt;
&lt;script src="/test/integration/no-ui-reporter.js"&gt;&lt;/script&gt;
&lt;style&gt;&lt;/style&gt;
&lt;/head&gt;

&lt;body&gt;
&lt;pre&gt;
&lt;code&gt;

&lt;/code&gt;
&lt;/pre&gt;
&lt;script src="/test/testutils.js"&gt;&lt;/script&gt;
&lt;script src="code-highlighting.js"&gt;&lt;/script&gt;
&lt;script src="/test/integration/adapter.js"&gt;&lt;/script&gt;
&lt;/body&gt;
&lt;/html&gt;</code>
</pre>
</div>
<script src="/test/testutils.js"></script>
<script src="code-highlighting.js"></script>
<script src="/test/integration/adapter.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/prism/1.17.1/prism.min.js"></script>
</body>
</html>
52 changes: 52 additions & 0 deletions test/integration/full/contrast/code-highlighting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
describe('color-contrast code highlighting test', function() {
'use strict';

describe('violations', function() {
it('should find issues', function(done) {
axe.run(
'#fixture',
{ runOnly: { type: 'rule', values: ['color-contrast'] } },
function(err, results) {
assert.isNull(err);
assert.lengthOf(results.violations, 1);
assert.lengthOf(results.violations[0].nodes, 32);
done();
}
);
});
});

describe('passes', function() {
it('should find passes', function(done) {
axe.run(
'#fixture',
{ runOnly: { type: 'rule', values: ['color-contrast'] } },
function(err, results) {
assert.isNull(err);
assert.lengthOf(results.passes, 1);
assert.lengthOf(results.passes[0].nodes, 27);
done();
}
);
});
});

describe('incomplete', function() {
it('should find just the code block', function(done) {
axe.run(
'#fixture',
{ runOnly: { type: 'rule', values: ['color-contrast'] } },
function(err, results) {
assert.isNull(err);
assert.lengthOf(results.incomplete, 1);
assert.lengthOf(results.incomplete[0].nodes, 1);
assert.equal(
results.incomplete[0].nodes[0].html,
'<code class=" language-html">'
);
done();
}
);
});
});
});