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(region): contents in iframes should pass the region rule if the iframe itself is in a region #2949

Merged
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
37 changes: 10 additions & 27 deletions lib/checks/navigation/heading-order-after.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { matchAncestry } from '../../core/utils';

export default function headingOrderAfter(results) {
// Construct a map of all headings on the page
const headingOrder = getHeadingOrder(results);
results.forEach(result => {
result.result = getHeadingOrderOutcome(result, headingOrder)
result.result = getHeadingOrderOutcome(result, headingOrder);
});
return results;
}
Expand All @@ -11,20 +13,20 @@ export default function headingOrderAfter(results) {
* Determine check outcome, based on the position of the result in the headingOrder
*/
function getHeadingOrderOutcome(result, headingOrder) {
const index = findHeadingOrderIndex(headingOrder, result.node.ancestry)
const index = findHeadingOrderIndex(headingOrder, result.node.ancestry);
const currLevel = headingOrder[index]?.level ?? -1;
const prevLevel = headingOrder[index - 1]?.level ?? -1;

// First heading always passes
if (index === 0) {
return true
};
return true;
}
// Heading not in the map
if (currLevel === -1) {
return undefined;
return undefined;
}
// Check if a heading is skipped
return (currLevel - prevLevel <= 1)
return currLevel - prevLevel <= 1;
}

/**
Expand Down Expand Up @@ -73,7 +75,7 @@ function mergeHeadingOrder(mergedHeadingOrder, result) {

/**
* Determine where the iframe results fit into the top-level heading order
*
*
* If a frame has no headings, but it does have iframes we might not have a result.
* We can account for this by finding the closest ancestor we do know about.
*/
Expand All @@ -83,7 +85,7 @@ function getFrameIndex(headingOrder, frameAncestry) {
if (index !== -1) {
return index;
}
frameAncestry = shortenArray(frameAncestry, 1)
frameAncestry = shortenArray(frameAncestry, 1);
}
return -1;
}
Expand All @@ -105,25 +107,6 @@ function addFrameToHeadingAncestry(heading, frameAncestry) {
return { ...heading, ancestry };
}

/**
* Check if two ancestries are identical
*/
function matchAncestry(ancestryA, ancestryB) {
if (ancestryA.length !== ancestryB.length) {
return false;
}
return ancestryA.every((selectorA, index) => {
const selectorB = ancestryB[index];
if (!Array.isArray(selectorA)) {
return selectorA === selectorB;
}
if (selectorA.length !== selectorB.length) {
return false;
}
return selectorA.every((str, index) => selectorB[index] === str);
});
}

/**
* Shorten an array by some number of items
*/
Expand Down
31 changes: 31 additions & 0 deletions lib/checks/navigation/region-after.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { matchAncestry } from '../../core/utils';

function regionAfter(results) {
clottman marked this conversation as resolved.
Show resolved Hide resolved
const iframeResults = results.filter(r => r.data.isIframe);

results.forEach(r => {
// continue if the element passed the check or is not in a frame
if (r.result || r.node.ancestry.length === 1) {
return;
}

const frameAncestry = r.node.ancestry.slice(0, -1);
for (const iframeResult of iframeResults) {
// if the container frame passed the check, this element should also pass
if (matchAncestry(frameAncestry, iframeResult.node.ancestry)) {
r.result = iframeResult.result;
break;
}
}
});

// iframe elements should always pass
iframeResults.forEach(r => {
if (!r.result) {
r.result = true;
}
});
return results;
}

export default regionAfter;
13 changes: 12 additions & 1 deletion lib/checks/navigation/region-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function findRegionlessElms(virtualNode, options) {
// End recursion if the element is a landmark, skiplink, or hidden content
if (
isRegion(virtualNode, options) ||
['iframe', 'frame'].includes(virtualNode.props.nodeName) ||
(dom.isSkipLink(virtualNode.actualNode) &&
dom.getElementByReference(virtualNode.actualNode, 'href')) ||
!dom.isVisible(node, true)
Expand All @@ -52,7 +53,12 @@ function findRegionlessElms(virtualNode, options) {
vNode._hasRegionDescendant = true;
vNode = vNode.parent;
}

// iframes are counted as regionless here, but parent tree should treat them as having content
// after method makes iframes count as regions
// this means siblings to iframes will fail, not the parent that contains an iframe and other children
if (['iframe', 'frame'].includes(virtualNode.props.nodeName)) {
return [virtualNode];
}
return [];

// Return the node is a content element. Ignore any direct text children
Expand All @@ -75,6 +81,11 @@ function findRegionlessElms(virtualNode, options) {

function regionEvaluate(node, options, virtualNode) {
let regionlessNodes = cache.get('regionlessNodes');

this.data({
WilcoFiers marked this conversation as resolved.
Show resolved Hide resolved
isIframe: ['iframe', 'frame'].includes(virtualNode.props.nodeName)
});

if (regionlessNodes) {
return !regionlessNodes.includes(virtualNode);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/checks/navigation/region.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
{
"id": "region",
"evaluate": "region-evaluate",
"after": "region-after",
"options": {
"regionMatcher": "dialog, [role=dialog], [role=alertdialog], svg, iframe"
"regionMatcher": "dialog, [role=dialog], [role=alertdialog], svg"
},
"metadata": {
"impact": "moderate",
Expand Down
2 changes: 2 additions & 0 deletions lib/core/base/metadata-function-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import internalLinkPresentEvaluate from '../../checks/navigation/internal-link-p
import metaRefreshEvaluate from '../../checks/navigation/meta-refresh-evaluate';
import pAsHeadingEvaluate from '../../checks/navigation/p-as-heading-evaluate';
import regionEvaluate from '../../checks/navigation/region-evaluate';
import regionAfter from '../../checks/navigation/region-after';
import skipLinkEvaluate from '../../checks/navigation/skip-link-evaluate';
import uniqueFrameTitleAfter from '../../checks/navigation/unique-frame-title-after';
import uniqueFrameTitleEvaluate from '../../checks/navigation/unique-frame-title-evaluate';
Expand Down Expand Up @@ -234,6 +235,7 @@ const metadataFunctionMap = {
'meta-refresh-evaluate': metaRefreshEvaluate,
'p-as-heading-evaluate': pAsHeadingEvaluate,
'region-evaluate': regionEvaluate,
'region-after': regionAfter,
'skip-link-evaluate': skipLinkEvaluate,
'unique-frame-title-after': uniqueFrameTitleAfter,
'unique-frame-title-evaluate': uniqueFrameTitleEvaluate,
Expand Down
1 change: 1 addition & 0 deletions lib/core/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export {
matchesExpression,
convertSelector
} from './matches';
export { default as matchAncestry } from './match-ancestry';
export { default as memoize } from './memoize';
export { default as mergeResults } from './merge-results';
export { default as nodeSorter } from './node-sorter';
Expand Down
20 changes: 20 additions & 0 deletions lib/core/utils/match-ancestry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Check if two ancestries are identical
*/
function matchAncestry(ancestryA, ancestryB) {
clottman marked this conversation as resolved.
Show resolved Hide resolved
if (ancestryA.length !== ancestryB.length) {
return false;
}
return ancestryA.every((selectorA, index) => {
const selectorB = ancestryB[index];
if (!Array.isArray(selectorA)) {
return selectorA === selectorB;
}
if (selectorA.length !== selectorB.length) {
return false;
}
return selectorA.every((str, index) => selectorB[index] === str);
});
}

export default matchAncestry;
153 changes: 153 additions & 0 deletions test/checks/navigation/region-after.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
describe('region-after', function() {
'use strict';

var checkContext = axe.testUtils.MockCheckContext();

afterEach(function() {
checkContext.reset();
});

it('should always pass iframes', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: false
},
{
data: { isIframe: false },
node: {
ancestry: ['html > body > iframe', 'html > body > p']
},
result: false
}
]);
assert.equal(results[0].result, true);
assert.equal(results[1].result, false);
});

it('should pass children of iframes if the iframe contained in it is in a region', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
data: { isIframe: false },
node: {
ancestry: ['html > body > iframe', 'html > body > p']
},
result: false
}
]);

assert.equal(results[0].result, true);
assert.equal(results[1].result, true);
});

it('should pass nested iframes', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: false
},
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe', 'html > body > iframe']
},
result: false
},
{
data: { isIframe: false },
node: {
ancestry: [
'html > body > iframe',
'html > body > iframe',
'html > body > p'
]
},
result: false
}
]);

assert.equal(results[0].result, true);
assert.equal(results[1].result, true);
assert.equal(results[2].result, false);
});

it('should pass children of nested iframes if the nested iframe is in a region', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: false
},
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe', 'html > body > iframe']
},
result: true
},
{
data: { isIframe: false },
node: {
ancestry: [
'html > body > iframe',
'html > body > iframe',
'html > body > p'
]
},
result: false
}
]);

assert.equal(results[0].result, true);
assert.equal(results[1].result, true);
assert.equal(results[2].result, true);
});

it('should pass content if a grandparent frame passes', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe', 'html > body > iframe']
},
result: false
},
{
data: { isIframe: false },
node: {
ancestry: [
'html > body > iframe',
'html > body > iframe',
'html > body > p'
]
},
result: false
}
]);
assert.equal(results[0].result, true);
assert.equal(results[1].result, true);
assert.equal(results[2].result, true);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a test missing from this, one that I strongly suspect fails.

Suggested change
});
});
it('should pass content if an grandparent frame passes', function() {
var results = checks.region.after([
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe', 'html > body > iframe']
},
result: false
},
{
data: { isIframe: false },
node: {
ancestry: [
'html > body > iframe',
'html > body > iframe',
'html > body > p'
]
},
result: false
}
]);
assert.equal(results[0].result, true);
assert.equal(results[1].result, true);
assert.equal(results[2].result, true);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's actually an integration test for this already - region-pass-nested-iframe.html. It's the reason I had to slightly modify your suggested code when I made region-after easier to understand.

8 changes: 0 additions & 8 deletions test/checks/navigation/region.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,14 +290,6 @@ describe('region', function() {
assert.isTrue(checkEvaluate.apply(checkContext, checkArgs));
});

it('treats iframe elements as regions', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now done in the after method, so this test won't pass any more.

var checkArgs = checkSetup(
'<iframe id="target"></iframe><div role="main">Content</div>'
);

assert.isTrue(checkEvaluate.apply(checkContext, checkArgs));
});

it('returns the outermost element as the error', function() {
var checkArgs = checkSetup(
'<div id="target"><p>This is random content.</p></div><div role="main"><h1 id="mainheader" tabindex="0">Introduction</h1></div>'
Expand Down
Loading