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

refactor: make element spec processing more cosistent #4093

Merged
merged 23 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
6 changes: 4 additions & 2 deletions lib/core/base/rule.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ Rule.prototype.run = function run(context, options = {}, resolve, reject) {
.then(results => {
const result = getResult(results);
if (result) {
result.node = new DqElement(node, options);
result.node = new DqElement(node, options).toJSON();
ruleResult.nodes.push(result);

// mark rule as incomplete rather than failure for rules with reviewOnFail
Expand Down Expand Up @@ -327,7 +327,9 @@ Rule.prototype.runSync = function runSync(context, options = {}) {

const result = getResult(results);
if (result) {
result.node = node.actualNode ? new DqElement(node, options) : null;
result.node = node.actualNode
? new DqElement(node, options).toJSON()
: null;
ruleResult.nodes.push(result);

// mark rule as incomplete rather than failure for rules with reviewOnFail
Expand Down
20 changes: 3 additions & 17 deletions lib/core/public/run-partial.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,14 @@ export default function runPartial(...args) {
axe._selectorData = getSelectorData(contextObj.flatTree);
axe._running = true;

// Even in the top frame, we don't support this with runPartial
options.elementRef = false;
straker marked this conversation as resolved.
Show resolved Hide resolved

return (
new Promise((res, rej) => {
axe._audit.run(contextObj, options, res, rej);
})
.then(results => {
results = results.map(({ nodes, ...result }) => ({
nodes: nodes.map(serializeNode),
...result
}));
const frames = contextObj.frames.map(({ node }) => {
return new DqElement(node, options).toJSON();
});
Expand All @@ -50,16 +49,3 @@ export default function runPartial(...args) {
})
);
}

function serializeNode({ node, ...nodeResult }) {
nodeResult.node = node.toJSON();
for (const type of ['any', 'all', 'none']) {
nodeResult[type] = nodeResult[type].map(
({ relatedNodes, ...checkResult }) => ({
...checkResult,
relatedNodes: relatedNodes.map(node => node.toJSON())
})
);
}
return nodeResult;
}
4 changes: 2 additions & 2 deletions lib/core/utils/check-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ function checkHelper(checkResult, options, resolve, reject) {
node = node.actualNode;
}
if (node instanceof window.Node) {
const dqElm = new DqElement(node, options);
checkResult.relatedNodes.push(dqElm);
const nodeSpec = new DqElement(node, options).toJSON();
checkResult.relatedNodes.push(nodeSpec);
}
});
}
Expand Down
3 changes: 3 additions & 0 deletions lib/core/utils/collect-results-from-frames.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ export default function collectResultsFromFrames(
resolve,
reject
) {
// elementRefs can't be passed across frame boundaries
options = { ...options, elementRef: false };

var q = queue();
var frames = parentContent.frames;

Expand Down
78 changes: 61 additions & 17 deletions lib/core/utils/dq-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function getSource(element) {
* "Serialized" `HTMLElement`. It will calculate the CSS selector,
* grab the source (outerHTML) and offer an array for storing frame paths
* @param {HTMLElement} element The element to serialize
* @param {Object} options Propagated from axe.run/etc
* @param {Object} spec Properties to use in place of the element when instantiated on Elements from other frames
*/
function DqElement(elm, options = {}, spec = {}) {
Expand All @@ -48,6 +49,8 @@ function DqElement(elm, options = {}, spec = {}) {
*/
this.fromFrame = this.spec.selector?.length > 1;

this._includeElementInJson = options.elementRef;

if (options.absolutePaths) {
this._options = { toRoot: true };
}
Expand Down Expand Up @@ -106,30 +109,71 @@ DqElement.prototype = {
return this._element;
},

/**
* Converts to a "spec", a form suitable for use with JSON.stringify
* (*not* to pre-stringified JSON)
* @returns {Object}
*/
toJSON() {
const spec = serializer.toSpec(this);
if (this._includeElementInJson) {
spec.element = this._element;
}
return spec;
}
};

DqElement.mergeSpecs = function mergeSpec(node, frame) {
// Parameter order reversed for backcompat
return serializer.mergeSpecs(frame, node);
};

/**
* The default handlers for DqElement.setSerializer. Custom serializers should
* produce output which at least the same properties that this would, usually
* by delegating to these defaults and then adding annotations on top.
*/
DqElement.defaultSerializer = {
toSpec: function toSpec(dqElm) {
return {
selector: dqElm.selector,
source: dqElm.source,
xpath: dqElm.xpath,
ancestry: dqElm.ancestry,
nodeIndexes: dqElm.nodeIndexes,
fromFrame: false
};
},
mergeSpecs: function mergeSpecs(parentFrame, child) {
return {
selector: this.selector,
source: this.source,
xpath: this.xpath,
ancestry: this.ancestry,
nodeIndexes: this.nodeIndexes
...child,
selector: [...parentFrame.selector, ...child.selector],
ancestry: [...parentFrame.ancestry, ...child.ancestry],
xpath: [...parentFrame.xpath, ...child.xpath],
nodeIndexes: [...parentFrame.nodeIndexes, ...child.nodeIndexes],
fromFrame: true
};
}
};

DqElement.fromFrame = function fromFrame(node, options, frame) {
const spec = DqElement.mergeSpecs(node, frame);
return new DqElement(frame.element, options, spec);
};
let serializer = DqElement.defaultSerializer;

DqElement.mergeSpecs = function mergeSpec(node, frame) {
return {
...node,
selector: [...frame.selector, ...node.selector],
ancestry: [...frame.ancestry, ...node.ancestry],
xpath: [...frame.xpath, ...node.xpath],
nodeIndexes: [...frame.nodeIndexes, ...node.nodeIndexes]
};
/**
* @param {Object} newSerializer
* @property {Function} toSpec Converts a DqElement to a "spec", a form
* suitable for JSON.stringify to consume. Output must include all properites
* that DqElement.defaultSerializer would have. Will always be invoked from the
* input element's original page context.
* @property {Function} mergeSpecs Merges two specs (produced by toSpec) which
* represent element's parent frame and an element, respectively. Will
* *not* necessarily be invoked from *either* node's original page context.
* This operation must be associative, that is, these two expressions must
* produce the same result:
* - mergeSpecs(a, mergeSpecs(b, c))
* - mergeSpecs(mergeSpecs(a, b), c)
*/
DqElement.setSerializer = function setSerializer(newSerializer) {
serializer = { ...DqElement.defaultSerializer, ...newSerializer };
};

export default DqElement;
16 changes: 9 additions & 7 deletions lib/core/utils/merge-results.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@ import findBy from './find-by';
* Adds the owning frame's CSS selector onto each instance of DqElement
* @private
* @param {Array} resultSet `nodes` array on a `RuleResult`
* @param {HTMLElement} frameElement The frame element
* @param {String} frameSelector Unique CSS selector for the frame
* @param {Object} options Propagated from axe.run/etc
* @param {Object} frameSpec The spec describing the owning frame (see DqElement.toJSON())
*/
function pushFrame(resultSet, options, frameSpec) {
resultSet.forEach(res => {
res.node = DqElement.fromFrame(res.node, options, frameSpec);
res.node = DqElement.mergeSpecs(res.node, frameSpec);
const checks = getAllChecks(res);

checks.forEach(check => {
check.relatedNodes = check.relatedNodes.map(node =>
DqElement.fromFrame(node, options, frameSpec)
DqElement.mergeSpecs(node, frameSpec)
);
});
});
Expand Down Expand Up @@ -68,8 +68,10 @@ function normalizeResult(result) {
/**
* Merges one or more RuleResults (possibly from different frames) into one RuleResult
* @private
* @param {Array} frameResults Array of objects including the RuleResults as `results` and frame as `frame`
* @return {Array} The merged RuleResults; should only have one result per rule
* @param {Array} frameResults Array of objects including the RuleResults as `results` and
* owning frame as either an Element `frameElement` or a spec `frameSpec` (see DqElement.toJSON)
* @param {Object} options Propagated from axe.run/etc
* @return {Array} The merged RuleResults; should only have one result per rule
*/
function mergeResults(frameResults, options) {
const mergedResult = [];
Expand Down Expand Up @@ -130,7 +132,7 @@ export default mergeResults;

function getFrameSpec(frameResult, options) {
if (frameResult.frameElement) {
return new DqElement(frameResult.frameElement, options);
return new DqElement(frameResult.frameElement, options).toJSON();
} else if (frameResult.frameSpec) {
return frameResult.frameSpec;
}
Expand Down
3 changes: 3 additions & 0 deletions test/core/base/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,8 @@ describe('Audit', function () {
'<input aria-labelledby="monkeys">' +
'<blink>FAIL ME</blink>';

axe.setup();

a.run(
{ include: [axe.utils.getFlattenedTree(fixture)[0]] },
{},
Expand Down Expand Up @@ -971,6 +973,7 @@ describe('Audit', function () {
return true;
}
});
axe.setup();

var preloadOptions = {
preload: {
Expand Down
49 changes: 46 additions & 3 deletions test/core/utils/dq-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ describe('DqElement', function () {
});

describe('toJSON', function () {
afterEach(() => {
DqElement.setSerializer(null);
});

it('should only stringify selector and source', function () {
var spec = {
selector: 'foo > bar > joe',
Expand All @@ -171,8 +175,31 @@ describe('DqElement', function () {
};

var div = document.createElement('div');
var result = new DqElement(div, {}, spec);
assert.deepEqual(result.toJSON(), spec);
var dqElm = new DqElement(div, {}, spec);
assert.deepEqual(dqElm.toJSON(), spec);
});

it('can be hooked by setSerializer', function () {
var serializeNodeHook = sinon.spy(node => ({
...DqElement.defaultSerializer.serializeNode(node),
hookAddition: 'new value'
}));
DqElement.setSerializer({ serializeNode: serializeNodeHook });

var spec = {
selector: 'foo > bar > joe',
source: '<joe aria-required="true">',
xpath: '/foo/bar/joe',
ancestry: 'foo > bar > joe',
nodeIndexes: [123, 456]
};
var div = document.createElement('div');
var dqElm = new DqElement(div, {}, spec);

var output = dqElm.toJSON();

assert.isTrue(serializeNodeHook.calledOnceWith(dqElm));
assert.propertyVal(output, 'hookAddition', 'new value');
});
});

Expand All @@ -199,12 +226,15 @@ describe('DqElement', function () {
dqIframe = new DqElement(iframe, {}, iframeSpec);
});

describe('.mergeSpecs', function () {
describe('DqElement.mergeSpecs', function () {
var mainSpec, iframeSpec;
beforeEach(function () {
mainSpec = dqMain.toJSON();
iframeSpec = dqIframe.toJSON();
});
afterEach(function () {
DqElement.setSerializer(null);
});

it('merges node and frame selectors', function () {
var mergedSpec = DqElement.mergeSpecs(mainSpec, iframeSpec);
Expand All @@ -229,6 +259,19 @@ describe('DqElement', function () {
mainSpec.nodeIndexes[0]
]);
});

it('can be hooked by setSerializer', function () {
var mergeSpecsHook = sinon.spy((nodeSpec, frameSpec) => ({
...DqElement.defaultSerializer.mergeSpecs(nodeSpec, frameSpec),
hookAddition: 'new value'
}));
DqElement.setSerializer({ mergeSpecs: mergeSpecsHook });

var mergedSpec = DqElement.mergeSpecs(mainSpec, iframeSpec);

assert.isTrue(mergeSpecsHook.calledOnceWith(mainSpec, iframeSpec));
assert.propertyVal(mergedSpec, 'hookAddition', 'new value');
});
});

describe('DqElement.fromFrame', function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ describe('Configure Options', function () {
};

var iframe = document.createElement('iframe');
iframe.src = '/test/mock/frames/context.html';
iframe.src = '/test/mock/frames/noHtml-config.html';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original test was configureing in the top frame but not the child frame. This isn't supported and doesn't provide the protection noHtml is intended to (the thing that the test after this one is verifying). However, previously, the original test worked despite testing an unsupported configuration because there was an extra round trip through DqElement in the top frame involved that no longer exists.

I think it's preferable to break the old pattern and demand that configure must be called consistently in each frame, so I changed the test rather than forcing a top-frame-only noHtml configure to suppress html properties from child frame results, but if that feels too concerning for backcompat reasons you could address it by adding an audit.noHtml test into the process-aggregate.js code that's filling in html from node.source.

iframe.onload = function () {
axe.configure(config);

Expand Down
13 changes: 13 additions & 0 deletions test/integration/full/serializer/custom-source-serializer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const base = axe.utils.DqElement.defaultSerializer;
axe.utils.DqElement.setSerializer({
toSpec: function toSpec(dqElm) {
const result = base.toSpec(dqElm);
result.source = dqElm.element.id;
return result;
},
mergeSpecs: function mergeSpecs(parentSpec, childSpec) {
const result = base.mergeSpecs(parentSpec, childSpec);
result.source = `${parentSpec.source} > ${childSpec.source}`;
return result;
}
});
12 changes: 12 additions & 0 deletions test/integration/full/serializer/frames/level1.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<html id="level1" lang="INVALID">
<head>
<meta charset="utf8" />
<script src="/axe.js"></script>
<script src="../custom-source-serializer.js"></script>
</head>
<body>
<iframe id="frame2-a" src="level2-a.html"></iframe>
<iframe id="frame2-b" src="level2-b.html"></iframe>
</body>
</html>
9 changes: 9 additions & 0 deletions test/integration/full/serializer/frames/level2-a.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html id="level2-a" lang="INVALID">
<head>
<meta charset="utf8" />
<script src="/axe.js"></script>
<script src="../custom-source-serializer.js"></script>
</head>
<body></body>
</html>
9 changes: 9 additions & 0 deletions test/integration/full/serializer/frames/level2-b.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html id="level2-b" xml:lang="INVALID">
<head>
<meta charset="utf8" />
<script src="/axe.js"></script>
<script src="../custom-source-serializer.js"></script>
</head>
<body></body>
</html>
Loading