chore(VirtualRule): add nodeIndex property (#2955)

* chore(VirtualRule): add nodeIndex property * chore(DqElement): add nodeIndexes * chore(DqElement): fix up fromFrame * chore(dqelement): fix for IE * chore(mergeResult): merge using nodeIndex * chore: formatting * chore: fix iframe order sorting * chore: resolve feedback * chore(merge-results): deal with invalid nodeIndexes * chore: more tests * chore: consistent node sorting * chore: more fun with sorting * fix(merge-results): handle invalid nodeIndexes * fix: nodeIndex can be 0 * Update lib/core/utils/dq-element.js
dequelabs · Jun 22, 2021 · 8d4ca3c · 8d4ca3c
1 parent b2663eb
commit 8d4ca3c
Show file tree

Hide file tree

Showing 11 changed files with 601 additions and 234 deletions.
diff --git a/lib/core/base/check.js b/lib/core/base/check.js
@@ -108,7 +108,7 @@ Check.prototype.run = function run(node, options, context, resolve, reject) {
  // possible reference error.
  if (node && node.actualNode) {
  // Save a reference to the node we errored on for futher debugging.
- e.errorNode = new DqElement(node.actualNode).toJSON();
+ e.errorNode = new DqElement(node).toJSON();
  }
  reject(e);
  return;
@@ -162,7 +162,7 @@ Check.prototype.runSync = function runSync(node, options, context) {
  // possible reference error.
  if (node && node.actualNode) {
  // Save a reference to the node we errored on for futher debugging.
- e.errorNode = new DqElement(node.actualNode).toJSON();
+ e.errorNode = new DqElement(node).toJSON();
  }
  throw e;
  }

diff --git a/lib/core/base/rule.js b/lib/core/base/rule.js
@@ -252,7 +252,7 @@ Rule.prototype.run = function run(context, options = {}, resolve, reject) {
  .then(results => {
  const result = getResult(results);
  if (result) {
- result.node = new DqElement(node.actualNode, options);
+ result.node = new DqElement(node, options);
  ruleResult.nodes.push(result);
 
  // mark rule as incomplete rather than failure for rules with reviewOnFail
@@ -321,9 +321,7 @@ Rule.prototype.runSync = function runSync(context, options = {}) {
 
  const result = getResult(results);
  if (result) {
- result.node = node.actualNode
- ? new DqElement(node.actualNode, options)
- : null;
+ result.node = node.actualNode ? new DqElement(node, options) : null;
  ruleResult.nodes.push(result);
 
  // mark rule as incomplete rather than failure for rules with reviewOnFail

diff --git a/lib/core/base/virtual-node/virtual-node.js b/lib/core/base/virtual-node/virtual-node.js
@@ -4,6 +4,7 @@ import { isFocusable, getTabbableElements } from '../../../commons/dom';
 import cache from '../cache';
 
 let isXHTMLGlobal;
+let nodeIndex = 0;
 
 class VirtualNode extends AbstractVirtualNode {
  /**
@@ -19,6 +20,11 @@ class VirtualNode extends AbstractVirtualNode {
  this.actualNode = node;
  this.parent = parent;
 
+ if (!parent) {
+ nodeIndex = 0;
+ }
+ this.nodeIndex = nodeIndex++;
+
  this._isHidden = null; // will be populated by axe.utils.isHidden
  this._cache = {};
 

diff --git a/lib/core/utils/dq-element.js b/lib/core/utils/dq-element.js
@@ -1,6 +1,8 @@
 import getSelector from './get-selector';
 import getAncestry from './get-ancestry';
 import getXpath from './get-xpath';
+import getNodeFromTree from './get-node-from-tree';
+import AbstractVirtualNode from '../base/virtual-node/abstract-virtual-node';
 
 function truncate(str, maxLength) {
  maxLength = maxLength || 300;
@@ -14,6 +16,9 @@ function truncate(str, maxLength) {
 }
 
 function getSource(element) {
+ if (!element?.outerHTML) {
+ return '';
+ }
  var source = element.outerHTML;
  if (!source && typeof XMLSerializer === 'function') {
  source = new XMLSerializer().serializeToString(element);
@@ -27,33 +32,46 @@ function getSource(element) {
  * @param {HTMLElement} element The element to serialize
  * @param {Object} spec Properties to use in place of the element when instantiated on Elements from other frames
  */
-function DqElement(element, options, spec) {
- this._fromFrame = !!spec;
+function DqElement(elm, options = {}, spec = {}) {
+ this.spec = spec;
+ if (elm instanceof AbstractVirtualNode) {
+ this._virtualNode = elm;
+ this._element = elm.actualNode;
+ } else {
+ this._element = elm;
+ this._virtualNode = getNodeFromTree(elm);
+ }
 
- this.spec = spec || {};
- if (options && options.absolutePaths) {
+ /**
+ * Whether DqElement was created from an iframe
+ * @type {boolean}
+ */
+ this.fromFrame = this.spec.selector?.length > 1;
+
+ if (options.absolutePaths) {
  this._options = { toRoot: true };
  }
 
  /**
- * The generated HTML source code of the element
- * @type {String}
+ * Number by which nodes in the flat tree can be sorted
+ * @type {Number}
  */
- // TODO: es-modules_audit
- if (axe._audit.noHtml) {
- this.source = null;
- } else if (this.spec.source !== undefined) {
- this.source = this.spec.source;
- } else {
- this.source = getSource(element);
+ this.nodeIndexes = [];
+ if (Array.isArray(this.spec.nodeIndexes)) {
+ this.nodeIndexes = this.spec.nodeIndexes;
+ } else if (typeof this._virtualNode?.nodeIndex === 'number') {
+ this.nodeIndexes = [this._virtualNode.nodeIndex];
  }
 
  /**
- * The element which this object is based off or the containing frame, used for sorting.
- * Excluded in toJSON method.
- * @type {HTMLElement}
+ * The generated HTML source code of the element
+ * @type {String|null}
  */
- this._element = element;
+ this.source = null;
+ // TODO: es-modules_audit
+ if (!axe._audit.noHtml) {
+ this.source = this.spec.source ?? getSource(this._element);
+ }
 }
 
 DqElement.prototype = {
@@ -88,16 +106,13 @@ DqElement.prototype = {
  return this._element;
  },
 
- get fromFrame() {
- return this._fromFrame;
- },
-
  toJSON() {
  return {
  selector: this.selector,
  source: this.source,
  xpath: this.xpath,
- ancestry: this.ancestry
+ ancestry: this.ancestry,
+ nodeIndexes: this.nodeIndexes
  };
  }
 };
@@ -107,7 +122,8 @@ DqElement.fromFrame = function fromFrame(node, options, frame) {
  ...node,
  selector: [...frame.selector, ...node.selector],
  ancestry: [...frame.ancestry, ...node.ancestry],
- xpath: [...frame.xpath, ...node.xpath]
+ xpath: [...frame.xpath, ...node.xpath],
+ nodeIndexes: [...frame.nodeIndexes, ...node.nodeIndexes]
  };
  return new DqElement(frame.element, options, spec);
 };

diff --git a/lib/core/utils/get-standards.js b/lib/core/utils/get-standards.js
@@ -1,5 +1,5 @@
-import standards from '../../standards'
-import clone from './clone'
+import standards from '../../standards';
+import clone from './clone';
 
 export default function getStandards() {
  return clone(standards);

diff --git a/lib/core/utils/merge-results.js b/lib/core/utils/merge-results.js
@@ -1,6 +1,5 @@
 import DqElement from './dq-element';
 import getAllChecks from './get-all-checks';
-import nodeSorter from './node-sorter';
 import findBy from './find-by';
 
 /**
@@ -32,25 +31,22 @@ function pushFrame(resultSet, dqFrame, options) {
  */
 function spliceNodes(target, to) {
  const firstFromFrame = to[0].node;
-
  for (let i = 0; i < target.length; i++) {
  const node = target[i].node;
- const sorterResult = nodeSorter(
- { actualNode: node.element },
- { actualNode: firstFromFrame.element }
+ const resultSort = nodeIndexSort(
+ node.nodeIndexes,
+ firstFromFrame.nodeIndexes
  );
-
  if (
- sorterResult > 0 ||
- (sorterResult === 0 &&
+ resultSort > 0 ||
+ (resultSort === 0 &&
  firstFromFrame.selector.length < node.selector.length)
  ) {
- target.splice.apply(target, [i, 0].concat(to));
+ target.splice(i, 0, ...to);
  return;
  }
  }
-
- target.push.apply(target, to);
+ target.push(...to);
 }
 
 function normalizeResult(result) {
@@ -105,28 +101,34 @@ function mergeResults(frameResults, options) {
  });
  });
 
- // Only sort results if we have the ability to run
- // document position (such as serial node context) and if
- // we have more than 1 result
- if (frameResults.length > 1 && window && window.Node) {
- mergedResult.forEach(result => {
- if (result.nodes) {
- result.nodes.sort((a, b) => {
- const aNode = a.node.element;
- const bNode = b.node.element;
-
- // only sort if the nodes are from different frames
- if (aNode !== bNode && (a.node._fromFrame || b.node._fromFrame)) {
- return nodeSorter(aNode, bNode);
- }
+ // Sort results in DOM order
+ mergedResult.forEach(result => {
+ if (result.nodes) {
+ result.nodes.sort((nodeA, nodeB) => {
+ return nodeIndexSort(nodeA.node.nodeIndexes, nodeB.node.nodeIndexes);
+ });
+ }
+ });
+ return mergedResult;
+}
 
- return 0;
- });
- }
- });
+function nodeIndexSort(nodeIndexesA = [], nodeIndexesB = []) {
+ const length = Math.max(nodeIndexesA?.length, nodeIndexesB?.length);
+ for (let i = 0; i < length; i++) {
+ const indexA = nodeIndexesA?.[i];
+ const indexB = nodeIndexesB?.[i];
+ if (typeof indexA !== 'number' || isNaN(indexA)) {
+ // Empty arrays go at the end, otherwise shortest array first
+ return i === 0 ? 1 : -1;
+ }
+ if (typeof indexB !== 'number' || isNaN(indexB)) {
+ return i === 0 ? -1 : 1;
+ }
+ if (indexA !== indexB) {
+ return indexA - indexB;
+ }
  }
-
- return mergedResult;
+ return 0;
 }
 
 export default mergeResults;
diff --git a/test/core/base/virtual-node/virtual-node.js b/test/core/base/virtual-node/virtual-node.js
@@ -152,6 +152,32 @@ describe('VirtualNode', function() {
  });
  });
 
+ describe('nodeIndex', function() {
+ it('increments nodeIndex when a parent is passed', function() {
+ var vHtml = new VirtualNode({ nodeName: 'html' });
+ var vHead = new VirtualNode({ nodeName: 'head' }, vHtml);
+ var vTitle = new VirtualNode({ nodeName: 'title' }, vHead);
+ var vBody = new VirtualNode({ nodeName: 'body' }, vHtml);
+
+ assert.equal(vHtml.nodeIndex, 0);
+ assert.equal(vHead.nodeIndex, 1);
+ assert.equal(vTitle.nodeIndex, 2);
+ assert.equal(vBody.nodeIndex, 3);
+ });
+
+ it('resets nodeIndex when no parent is passed', function() {
+ var vHtml = new VirtualNode({ nodeName: 'html' });
+ var vHead = new VirtualNode({ nodeName: 'head' }, vHtml);
+ assert.equal(vHtml.nodeIndex, 0);
+ assert.equal(vHead.nodeIndex, 1);
+
+ vHtml = new VirtualNode({ nodeName: 'html' });
+ vHead = new VirtualNode({ nodeName: 'head' }, vHtml);
+ assert.equal(vHtml.nodeIndex, 0);
+ assert.equal(vHead.nodeIndex, 1);
+ });
+ });
+
  describe.skip('isFocusable', function() {
  var commons;
 

diff --git a/test/core/public/run-rules.js b/test/core/public/run-rules.js
@@ -134,14 +134,17 @@ describe('runRules', function() {
  var nodes = r[0].passes.map(function(detail) {
  return detail.node.selector;
  });
-
- assert.deepEqual(nodes, [
- ['#level0'],
- ['#level0', '#level1'],
- ['#level0', '#level1', '#level2a'],
- ['#level0', '#level1', '#level2b']
- ]);
- done();
+ try {
+ assert.deepEqual(nodes, [
+ ['#level0'],
+ ['#level0', '#level1'],
+ ['#level0', '#level1', '#level2a'],
+ ['#level0', '#level1', '#level2b']
+ ]);
+ done();
+ } catch (e) {
+ done(e);
+ }
  },
  isNotCalled
  );
@@ -229,7 +232,8 @@ describe('runRules', function() {
  "/iframe[@id='context-test']",
  "/div[@id='target']"
  ],
- source: '<div id="target"></div>'
+ source: '<div id="target"></div>',
+ nodeIndexes: [12, 14]
  },
  any: [
  {
@@ -271,7 +275,8 @@ describe('runRules', function() {
  "/div[@id='foo']"
  ],
  source:
- '<div id="foo">\n <div id="bar"></div>\n </div>'
+ '<div id="foo">\n <div id="bar"></div>\n </div>',
+ nodeIndexes: [12, 9]
  },
  any: [
  {
@@ -289,7 +294,8 @@ describe('runRules', function() {
  "/div[@id='foo']"
  ],
  source:
- '<div id="foo">\n <div id="bar"></div>\n </div>'
+ '<div id="foo">\n <div id="bar"></div>\n </div>',
+ nodeIndexes: [12, 9]
  }
  ]
  }
@@ -536,7 +542,8 @@ describe('runRules', function() {
  'html > body > div:nth-child(1) > div:nth-child(1)'
  ],
  xpath: ["/div[@id='target']"],
- source: '<div id="target">Target!</div>'
+ source: '<div id="target">Target!</div>',
+ nodeIndexes: [12]
  },
  impact: 'moderate',
  any: [
@@ -579,7 +586,8 @@ describe('runRules', function() {
  ancestry: [
  'html > body > div:nth-child(1) > div:nth-child(1)'
  ],
- source: '<div id="target">Target!</div>'
+ source: '<div id="target">Target!</div>',
+ nodeIndexes: [12]
  },
  any: [
  {
@@ -595,7 +603,8 @@ describe('runRules', function() {
  'html > body > div:nth-child(1) > div:nth-child(1)'
  ],
  xpath: ["/div[@id='target']"],
- source: '<div id="target">Target!</div>'
+ source: '<div id="target">Target!</div>',
+ nodeIndexes: [12]
  }
  ]
  }