Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

#149 Be able to evaluate page with iframes #150

Merged
merged 6 commits into from
Jul 8, 2015
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 3 additions & 3 deletions dist/js/axs_testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -1545,7 +1545,7 @@ axs.AuditRule.collectMatchingElements = function(a, b, c, d) {
for (e = f.getDistributedNodes(), f = 0;f < e.length;f++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@davidhsv this file needs to be checked out entirely.

axs.AuditRule.collectMatchingElements(e[f], b, c, d);
}
} else {
} else {
console.warn("ShadowRoot not provided for", e);
}
}
Expand Down Expand Up @@ -1590,7 +1590,7 @@ axs.AuditRules = {};
}
if (c.name in a) {
throw Error('Can not add audit rule with same name: "' + c.name + '"');
}
}
a[c.name] = b[c.code] = c;
};
axs.AuditRules.getRule = function(c) {
Expand Down Expand Up @@ -1803,7 +1803,7 @@ axs.AuditRules.addRule({name:"audioWithoutControls", heading:"Audio elements sho
if (a.test(e) && (e = e.replace(a, ""), !axs.constants.ARIA_PROPERTIES.hasOwnProperty(e))) {
return !0;
}
}
}
return !1;
}, code:"AX_ARIA_11"});
})();
Expand Down
3 changes: 2 additions & 1 deletion src/audits/FocusableElementNotVisibleAndNotAriaHidden.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ axs.AuditRules.addRule({
}
// Ignore elements which have a negative tabindex and no text content,
// as they will be skipped by assistive technology
if (axs.properties.findTextAlternatives(element, {}).trim() === '')
var alternative = axs.properties.findTextAlternatives(element, {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a behavior change or just cleanup? It doesn't seem related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a cleanup, I was getting a exception sometimes when findTextAlternatives return null (null.trim()).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I just submitted a separate PR for this same issue, not having seen this, sorry.

if (alternative == null || alternative.trim() === '')
return false;

return true;
Expand Down
8 changes: 8 additions & 0 deletions src/js/AuditRule.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,14 @@ axs.AuditRule.collectMatchingElements = function(node, matcher, collection,
}
}
}

//If it is a iframe, get the contentDocument
if (element && element.localName == 'iframe' && element.contentDocument) {
axs.AuditRule.collectMatchingElements(element.contentDocument,
matcher,
collection,
opt_shadowRoot);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is opt_shadowRoot necessary here? Is it required for collectMatchingElements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, it's a recursive method and I think it's necessary to always pass this atribute forward. For example, if we got a shadow down inside a iframe contentDocument, it will be needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense.

}
// If it is neither the parent of a ShadowRoot, a <content> element, nor
// a <shadow> element recurse normally.
var child = node.firstChild;
Expand Down
10 changes: 10 additions & 0 deletions test/js/audit-rule-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@
equal(matched.length, DIV_COUNT);
});

test("Iframe with simple DOM", function () {
var ifrm = document.createElement("IFRAME");
var container = document.getElementById('qunit-fixture');
container.appendChild(ifrm);
ifrm.contentDocument.body.appendChild(buildTestDom());
var matched = [];
axs.AuditRule.collectMatchingElements(container, matcher, matched);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this method mutating matched in memory? That doesn't seem ideal. Unrelated to this PR, but since we're in the neighborhood I thought I'd point it out. Not a nlocker for merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was justing copying the same behavior in others tests (in this same file). I don't like methods changing parameter's content too (bad smell).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that's fine, thought I'd point it out as something we can improve in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is intended behaviour. Since this is a recursive method, we need an accumulator to keep track of matched elements - otherwise we'd be constantly having to combine partial results.

equal(matched.length, DIV_COUNT);
});

test("With shadow DOM with no content insertion point", function () {
var container = document.getElementById('qunit-fixture');
container.appendChild(buildTestDom());
Expand Down
3 changes: 3 additions & 0 deletions tools/runner/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ var page = require('webpage').create(),
system = require('system'),
url;

// disabling so we can get the document root from iframes (http -> https)
page.settings.webSecurityEnabled = false;

if (system.args.length !== 2) {
console.log('Usage: phantomjs audit.js URL');
phantom.exit();
Expand Down