Skip to content

Commit

Permalink
fix(perf): remove need for node sorting from select completely
Browse files Browse the repository at this point in the history
  • Loading branch information
dylanb committed Feb 5, 2018
1 parent 189c165 commit 7677a6a
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 38 deletions.
5 changes: 5 additions & 0 deletions lib/core/base/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ function validateContext(context) {
* @param {Object} spec Configuration or "specification" object
*/
function Context(spec) {
//jshint maxstatements:18
'use strict';
var self = this;

Expand Down Expand Up @@ -229,4 +230,8 @@ function Context(spec) {
if (err instanceof Error) {
throw err;
}
if (!Array.isArray(this.include)) {
this.include = Array.from(this.include);
}
this.include.sort(axe.utils.nodeSorter); // ensure that the order of the include nodes is document order
}
30 changes: 11 additions & 19 deletions lib/core/utils/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,17 @@ function pushNode(result, nodes) {
}

/**
* returns true if any of the nodes in the list is a parent of another node in the list
* reduces the includes list to only the outermost includes
* @param {Array} the array of include nodes
* @return {Boolean}
* @return {Array} the modified array of nodes
*/
function hasOverlappingIncludes(includes) {
let list = includes.slice();
while (list.length > 1) {
let last = list.pop();
if (list[list.length - 1].actualNode.contains(last.actualNode)) {
return true;
function reduceIncludes(includes) {
return includes.reduce((res, el) => {
if (!res.length || !res[res.length - 1].actualNode.contains(el.actualNode)) {
res.push(el);
}
}
return false;
return res;
}, []);
}

/**
Expand All @@ -94,10 +92,6 @@ axe.utils.select = function select(selector, context) {
'use strict';

var result = [], candidate;
if (!Array.isArray(context.include)) {
context.include = Array.from(context.include);
}
context.include.sort(axe.utils.nodeSorter); // ensure that the order of the include nodes is document order
if (axe._selectCache) { // if used outside of run, it will still work
for (var j = 0, l = axe._selectCache.length; j < l; j++) {
// First see whether the item exists in the cache
Expand All @@ -112,18 +106,16 @@ axe.utils.select = function select(selector, context) {
return isNodeInContext(node, context);
};
})(context);
for (var i = 0; i < context.include.length; i++) {
candidate = context.include[i];
var reducedIncludes = reduceIncludes(context.include);
for (var i = 0; i < reducedIncludes.length; i++) {
candidate = reducedIncludes[i];
if (candidate.actualNode.nodeType === candidate.actualNode.ELEMENT_NODE &&
axe.utils.matchesSelector(candidate.actualNode, selector) &&
curried(candidate)) {
result = pushNode(result, [candidate]);
}
result = pushNode(result, axe.utils.querySelectorAllFilter(candidate, selector, curried));
}
if (context.include.length > 1 && hasOverlappingIncludes(context.include)) {
result.sort(axe.utils.nodeSorter);
}
if (axe._selectCache) {
axe._selectCache.push({
selector: selector,
Expand Down
13 changes: 13 additions & 0 deletions test/core/base/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,19 @@ describe('Context', function() {
[$id('foo'), $id('bar')]);
});

it('should sort the include nodes in document order', function() {
fixture.innerHTML = '<div id="foo"><div id="bar"></div></div><div id="baz"></div>';

var result = new Context([
['#foo'],
['#baz'],
['#bar']
]);

assert.deepEqual(result.include.map(function (n) { return n.actualNode; }),
[$id('foo'), $id('bar'), $id('baz')]);
});

it('should remove any null reference', function() {
fixture.innerHTML = '<div id="foo"><div id="bar"></div></div>';

Expand Down
22 changes: 20 additions & 2 deletions test/core/base/rule.js
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,18 @@ describe('Rule', function() {
evaluate: function() {},
id: 'cats'
}]
}, {
checks: {
cats: {
run: function (node, options, resolve) {
success = true;
resolve(true);
}
}
}
});
rule.run({
include: axe.utils.getFlattenedTree(document)[0]
include: [axe.utils.getFlattenedTree(document)[0]]
}, {}, noop, isNotCalled);
assert.isTrue(success);

Expand All @@ -538,9 +547,18 @@ describe('Rule', function() {
evaluate: function() {},
id: 'cats'
}]
}, {
checks: {
cats: {
run: function (node, options, resolve) {
success = true;
resolve(true);
}
}
}
});
rule.run({
include: axe.utils.getFlattenedTree(document)[0]
include: [axe.utils.getFlattenedTree(document)[0]]
}, {}, function() {
success = true;
}, isNotCalled);
Expand Down
23 changes: 6 additions & 17 deletions test/core/utils/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,30 +125,19 @@ describe('axe.utils.select', function () {

});

it('should sort by DOM order', function () {
fixture.innerHTML = '<div id="one"><div id="target1" class="bananas"></div></div>' +
'<div id="two"><div id="target2" class="bananas"></div></div>';

var result = axe.utils.select('.bananas', { include: [axe.utils.getFlattenedTree($id('two'))[0],
axe.utils.getFlattenedTree($id('one'))[0]] });

assert.deepEqual(result.map(function (n) { return n.actualNode; }),
[$id('target1'), $id('target2')]);

});

it('should sort by DOM order on overlapping elements', function () {
it('should not return duplicates on overlapping includes', function () {
fixture.innerHTML = '<div id="zero"><div id="one"><div id="target1" class="bananas"></div></div>' +
'<div id="two"><div id="target2" class="bananas"></div></div></div>';

var result = axe.utils.select('.bananas', { include: [axe.utils.getFlattenedTree($id('one'))[0],
axe.utils.getFlattenedTree($id('zero'))[0]] });
var result = axe.utils.select('.bananas', { include: [axe.utils.getFlattenedTree($id('zero'))[0],
axe.utils.getFlattenedTree($id('one'))[0]] });

assert.deepEqual(result.map(function (n) { return n.actualNode; }),
[$id('target1'), $id('target1'), $id('target2')]);
assert.equal(result.length, 3);
[$id('target1'), $id('target2')]);
assert.equal(result.length, 2);

});

it ('should return the cached result if one exists', function () {
fixture.innerHTML = '<div id="zero"><div id="one"><div id="target1" class="bananas"></div></div>' +
'<div id="two"><div id="target2" class="bananas"></div></div></div>';
Expand Down

0 comments on commit 7677a6a

Please sign in to comment.