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(td-has-headers): greatly improve performance of td-has-headers rule #1887

Merged
merged 4 commits into from
Nov 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions lib/checks/tables/td-has-header.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var tableUtils = axe.commons.table;
var badCells = [];
var cells = tableUtils.getAllCells(node);
const tableUtils = axe.commons.table;
const badCells = [];
const cells = tableUtils.getAllCells(node);
const tableGrid = tableUtils.toGrid(node);

cells.forEach(cell => {
// For each non-empty data cell that doesn't have an aria label
Expand All @@ -10,7 +11,7 @@ cells.forEach(cell => {
!axe.commons.aria.label(cell)
) {
// Check if it has any headers
const hasHeaders = tableUtils.getHeaders(cell).some(header => {
const hasHeaders = tableUtils.getHeaders(cell, tableGrid).some(header => {
return header !== null && !!axe.commons.dom.hasContent(header);
});

Expand Down
4 changes: 2 additions & 2 deletions lib/commons/table/get-cell-position.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* @param {HTMLTableCellElement} cell The table cell of which to get the position
* @return {Object} Object with `x` and `y` properties of the coordinates
*/
table.getCellPosition = function(cell, tableGrid) {
table.getCellPosition = axe.utils.memoize(function(cell, tableGrid) {
var rowIndex, index;
if (!tableGrid) {
tableGrid = table.toGrid(dom.findUp(cell, 'table'));
Expand All @@ -25,4 +25,4 @@ table.getCellPosition = function(cell, tableGrid) {
}
}
}
};
});
66 changes: 55 additions & 11 deletions lib/commons/table/get-headers.js
Original file line number Diff line number Diff line change
@@ -1,29 +1,73 @@
/* global table */

/**
* Loop through the table grid looking for headers and caching the result.
* @param {String} headerType The type of header to look for ("row" or "col")
* @param {Object} position The position of the cell to start looking
* @param {Array} tablegrid A matrix of the table obtained using axe.commons.table.toGrid
* @return {Array<HTMLTableCellElement>} Array of HTMLTableCellElements that are headers
*/
function traverseForHeaders(headerType, position, tableGrid) {
const property = headerType === 'row' ? '_rowHeaders' : '_colHeaders';
const predicate =
headerType === 'row' ? table.isRowHeader : table.isColumnHeader;
const rowEnd = headerType === 'row' ? position.y : 0;
const colEnd = headerType === 'row' ? 0 : position.x;

let headers;
const cells = [];
for (let row = position.y; row >= rowEnd && !headers; row--) {
for (let col = position.x; col >= colEnd; col--) {
const cell = tableGrid[row] ? tableGrid[row][col] : undefined;

if (!cell) {
continue;
}

// stop traversing once we've found a cache
const vNode = axe.utils.getNodeFromTree(cell);
if (vNode[property]) {
headers = vNode[property];
break;
}

cells.push(cell);
}
}

// need to check that the cells we've traversed are headers
headers = (headers || []).concat(cells.filter(predicate));

// cache results
cells.forEach(tableCell => {
const vNode = axe.utils.getNodeFromTree(tableCell);
vNode[property] = headers;
});

return headers;
}

/**
* Get any associated table headers for a `HTMLTableCellElement`
* @method getHeaders
* @memberof axe.commons.table
* @instance
* @param {HTMLTableCellElement} cell The cell of which to get headers
* @param {Array} [tablegrid] A matrix of the table obtained using axe.commons.table.toGrid
* @return {Array<HTMLTableCellElement>} Array of headers associated to the table cell
*/
table.getHeaders = function(cell) {
table.getHeaders = function(cell, tableGrid) {
straker marked this conversation as resolved.
Show resolved Hide resolved
if (cell.hasAttribute('headers')) {
return commons.dom.idrefs(cell, 'headers');
}

var tableGrid = commons.table.toGrid(commons.dom.findUp(cell, 'table'));
var position = commons.table.getCellPosition(cell, tableGrid);
if (!tableGrid) {
tableGrid = commons.table.toGrid(commons.dom.findUp(cell, 'table'));
}
const position = commons.table.getCellPosition(cell, tableGrid);

// TODO: RTL text
var rowHeaders = table
.traverse('left', position, tableGrid)
.filter(cell => table.isRowHeader(cell));

var colHeaders = table
.traverse('up', position, tableGrid)
.filter(cell => table.isColumnHeader(cell));
const rowHeaders = traverseForHeaders('row', position, tableGrid);
const colHeaders = traverseForHeaders('col', position, tableGrid);

return [].concat(rowHeaders, colHeaders).reverse();
};
2 changes: 1 addition & 1 deletion lib/commons/table/get-scope.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ table.getScope = function(cell) {
return false;
}
var tableGrid = table.toGrid(dom.findUp(cell, 'table'));
var pos = table.getCellPosition(cell);
var pos = table.getCellPosition(cell, tableGrid);

// The element is in a row with all th elements, that makes it a column header
var headerRow = tableGrid[pos.y].reduce((headerRow, cell) => {
Expand Down
4 changes: 2 additions & 2 deletions lib/commons/table/to-grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* @param {HTMLTableElement} node The table to convert
* @return {Array<HTMLTableCellElement>} Array of HTMLTableCellElements
*/
table.toGrid = function(node) {
table.toGrid = axe.utils.memoize(function(node) {
var table = [];
var rows = node.rows;
for (var i = 0, rowLength = rows.length; i < rowLength; i++) {
Expand All @@ -32,7 +32,7 @@ table.toGrid = function(node) {
}

return table;
};
});

// This was the old name
table.toArray = table.toGrid;
3 changes: 2 additions & 1 deletion lib/core/imports/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,6 @@ axe.imports = {
axios: require('axios'),
CssSelectorParser: require('css-selector-parser').CssSelectorParser,
doT: require('@deque/dot'),
emojiRegexText: require('emoji-regex')
emojiRegexText: require('emoji-regex'),
memoize: require('memoizee')
};
1 change: 1 addition & 0 deletions lib/core/public/run-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

// Clean up after resolve / reject
function cleanup() {
axe._memoizedFns.forEach(fn => fn.clear());
straker marked this conversation as resolved.
Show resolved Hide resolved
axe._cache.clear();
axe._tree = undefined;
axe._selectorData = undefined;
Expand Down
17 changes: 17 additions & 0 deletions lib/core/utils/memoize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Memoize a function.
* @method memoize
* @memberof axe.utils
* @param {Function} fn Function to memoize
* @return {Function}
*/
axe._memoizedFns = [];
axe.utils.memoize = function(fn) {
straker marked this conversation as resolved.
Show resolved Hide resolved
// keep track of each function that is memoized so it can be cleared at
// the end of a run. each memoized function has its own cache, so there is
// no method to clear all memoized caches. instead, we have to clear each
// individual memoized function ourselves.
const memoized = axe.imports.memoize(fn);
axe._memoizedFns.push(memoized);
return memoized;
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
"lint-staged": "^9.2.1",
"make-dir": "^3.0.0",
"markdown-table": "^1.1.2",
"memoizee": "^0.4.14",
"minami": "^1.2.3",
"mkdirp": "^0.5.1",
"mocha": "^6.1.2",
Expand Down
38 changes: 38 additions & 0 deletions test/commons/table/get-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ describe('table.getHeaders', function() {
}

var fixture = $id('fixture');
var origToGrid = axe.commons.table.toGrid;
var origCellPosition = axe.commons.table.getCellPosition;

afterEach(function() {
fixture.innerHTML = '';
axe._tree = undefined;
axe.commons.table.toGrid = origToGrid;
axe.commons.table.getCellPosition = origCellPosition;
});

it('should work with scope=auto', function() {
Expand Down Expand Up @@ -136,4 +140,38 @@ describe('table.getHeaders', function() {
axe.testUtils.flatTreeSetup(fixture.firstChild);
assert.deepEqual(axe.commons.table.getHeaders(target), []);
});

it('should not call toGrid if a tableGrid is passed in', function() {
fixture.innerHTML =
'<table id="table">' +
'<tr><td></td><th id="t1">1</th><th>2</th></tr>' +
'<tr><th id="t2">2</th><td id="target">ok</td><td></td></tr>' +
'</table>';

var target = $id('target');
var table = $id('table');

axe.testUtils.flatTreeSetup(fixture.firstChild);
var tableGrid = axe.commons.table.toGrid(table);

// getCellPosition will eventually call into toGrid when checking for
// scope but we're only interested if the toGrid call was called before
// cellPosition
var called = false;
axe.commons.table.toGrid = function(table) {
called = true;
return origToGrid(table);
};
axe.commons.table.getCellPosition = function(cell, tableGrid) {
axe.commons.table.toGrid = origToGrid;
return origCellPosition(cell, tableGrid);
};

axe.commons.table.getHeaders(target, tableGrid);
assert.isFalse(called);
assert.deepEqual(axe.commons.table.getHeaders(target), [
$id('t1'),
$id('t2')
]);
});
});
45 changes: 45 additions & 0 deletions test/core/public/run-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@ describe('runRules', function() {
}

var fixture = document.getElementById('fixture');
var memoizedFns;

var isNotCalled;
beforeEach(function() {
memoizedFns = axe._memoizedFns.slice();
isNotCalled = function(err) {
throw err || new Error('Reject should not be called');
};
Expand All @@ -60,6 +62,7 @@ describe('runRules', function() {
fixture.innerHTML = '';
axe._audit = null;
axe._tree = undefined;
axe._memoizedFns = memoizedFns;
});

it('should work', function(done) {
Expand Down Expand Up @@ -914,4 +917,46 @@ describe('runRules', function() {
}, 100);
});
});

it('should clear the memoized cache for each function', function(done) {
axe._load({
rules: [
{
id: 'html',
selector: 'html',
any: ['html']
}
],
checks: [
{
id: 'html',
evaluate: function() {
return true;
}
}
],
messages: {}
});

runRules(
document,
{},
function resolve(out, cleanup) {
var called = false;
axe._memoizedFns = [
{
clear: function() {
called = true;
}
}
];

cleanup();
assert.isTrue(called);

done();
},
isNotCalled
);
});
});
32 changes: 32 additions & 0 deletions test/core/utils/memoize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
describe('axe.utils.memoize', function() {
'use strict';

var orig = axe.imports.memoize;
var memoizedFns;

beforeEach(function() {
memoizedFns = axe._memoizedFns.slice();
});

afterEach(function() {
axe.imports.memoize = orig;
axe._memoizedFns = memoizedFns;
});

it('should call imports.memoize', function() {
var called = false;
axe.imports.memoize = function() {
called = true;
};

axe.utils.memoize(function() {});
assert.isTrue(called);
});

it('should add the function to axe._memoizedFns', function() {
axe._memoizedFns.length = 0;

axe.utils.memoize(function myFn() {});
assert.equal(axe._memoizedFns.length, 1);
});
});