From 7677a6a46247b4d96a55a0ac536ae9f60afcfaea Mon Sep 17 00:00:00 2001 From: Dylan Barrell Date: Mon, 5 Feb 2018 10:19:15 -0500 Subject: [PATCH] fix(perf): remove need for node sorting from select completely --- lib/core/base/context.js | 5 +++++ lib/core/utils/select.js | 30 +++++++++++------------------- test/core/base/context.js | 13 +++++++++++++ test/core/base/rule.js | 22 ++++++++++++++++++++-- test/core/utils/select.js | 23 ++++++----------------- 5 files changed, 55 insertions(+), 38 deletions(-) diff --git a/lib/core/base/context.js b/lib/core/base/context.js index 3224c88c45..48ad016a5e 100644 --- a/lib/core/base/context.js +++ b/lib/core/base/context.js @@ -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; @@ -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 } diff --git a/lib/core/utils/select.js b/lib/core/utils/select.js index 9ce44e125c..a8e7aa192c 100644 --- a/lib/core/utils/select.js +++ b/lib/core/utils/select.js @@ -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; + }, []); } /** @@ -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 @@ -112,8 +106,9 @@ 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)) { @@ -121,9 +116,6 @@ axe.utils.select = function select(selector, context) { } 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, diff --git a/test/core/base/context.js b/test/core/base/context.js index 86f253616d..e15bc8de0f 100644 --- a/test/core/base/context.js +++ b/test/core/base/context.js @@ -98,6 +98,19 @@ describe('Context', function() { [$id('foo'), $id('bar')]); }); + it('should sort the include nodes in document order', function() { + fixture.innerHTML = '
'; + + 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 = '
'; diff --git a/test/core/base/rule.js b/test/core/base/rule.js index d1b58b4fa2..290e29b4f2 100644 --- a/test/core/base/rule.js +++ b/test/core/base/rule.js @@ -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); @@ -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); diff --git a/test/core/utils/select.js b/test/core/utils/select.js index aef282f264..6629024aae 100644 --- a/test/core/utils/select.js +++ b/test/core/utils/select.js @@ -125,30 +125,19 @@ describe('axe.utils.select', function () { }); - it('should sort by DOM order', function () { - fixture.innerHTML = '
' + - '
'; - - 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 = '
' + '
'; - 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 = '
' + '
';