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

function regionAfter(results) {
clottman marked this conversation as resolved.
Show resolved Hide resolved
results.forEach((r, index) => {
// this element failed the check
if (!r.result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should prefer the "exit early" code style.

Suggested change
if (!r.result) {
if (r.result) {
return
}

// we need to see if it's actually contained in an iframe that passed the check
const ancestryMinusLast = r.node.ancestry.slice(0, -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ancestryMinusLast = r.node.ancestry.slice(0, -1);
const frameAncestry = r.node.ancestry.slice(0, -1);


// we only need to check up to index - 1 because the iframe this element might be contained in will have been found before this element
for (const earlierResult of results.slice(0, index)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels unnecessarily complex. I suggest you create a list of frames with result:true, and then try to match against that. You have no business running matchAncestry against anything other than iframes. You don't need to do it if ancestry has a length of 1 either.

That also lets you use clearer variable names. It took me a while to figure out what earlierResult was even for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That strategy won't work because we need to be able to evaluate nested iframes correctly - when an iframe is contained in another iframe that should pass, but the inner iframe does not pass on its own, we don't know until we've iterated sequentially through that the inner iframe should pass and thus be considered part of the set that should be matched against.

Consider this (which is represented in the full integration tests as region-pass-nested-iframe.html):

<main>
  <iframe id='outer' srcdoc="<iframe id='inner' srcdoc='<p>hello</p>'></iframe></iframe>
</main>

Here's the code I was working on, which has the problem described above, if you want to play around with it yourself.

function regionAfter(results) {
  const passingFrameAncestries = results.filter(
    r => r.result && r.data.isIframe
  ).map(r => r.node.ancestry);

  results.forEach((r, index) => {
    // this element failed the check, and it's in an iframe
    if (!r.result && r.node.ancestry.length > 1) {
      // we need to see if it's actually contained in an iframe that passed the check
      const frameAncestry = r.node.ancestry.slice(0, -1);

      for (const frame of passingFrameAncestries) {
        if (matchAncestry(frameAncestry, frame)) {
          r.result = true;
          break;
        }
      }
    }
  });

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

// matches if the current result came from the iframe that's represented by earlierResult
if (matchAncestry(ancestryMinusLast, earlierResult.node.ancestry)) {
r.result = earlierResult.result;
break;
}
}
}
});

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

export default regionAfter;
5 changes: 5 additions & 0 deletions lib/checks/navigation/region-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,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;
197 changes: 197 additions & 0 deletions test/checks/navigation/region-after.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
describe('region-after', function() {
'use strict';

var checkContext = axe.testUtils.MockCheckContext();

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

it('should always pass iframes', function() {
var result = 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.deepEqual(result, [
clottman marked this conversation as resolved.
Show resolved Hide resolved
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
data: { isIframe: false },
node: {
ancestry: ['html > body > iframe', 'html > body > p']
},
result: false
}
]);
});

it('should pass children of iframes if the iframe contained in it is in a region', function() {
var result = 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.deepEqual(result, [
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
data: { isIframe: false },
node: {
ancestry: ['html > body > iframe', 'html > body > p']
},
result: true
}
]);
});

it('should pass nested iframes', function() {
var result = 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.deepEqual(result, [
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
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
}
]);
});

it('should pass children of nested iframes if the nested iframe is in a region', function() {
var result = 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.deepEqual(result, [
{
data: { isIframe: true },
node: {
ancestry: ['html > body > iframe']
},
result: true
},
{
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: 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