#149 Be able to evaluate page with iframes #150
Conversation
…Security in phantomJs -> so we can get the document root from frames.
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
| container.appendChild(ifrm); | ||
| ifrm.contentDocument.body.appendChild(buildTestDom()); | ||
| var matched = []; | ||
| axs.AuditRule.collectMatchingElements(container, matcher, matched); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Yep, that's fine, thought I'd point it out as something we can improve in the future.
There was a problem hiding this comment.
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.
|
Check out my comment on the |
|
Actually, forgot to also mention that I don't think we need to rebuild |
|
The axs_testing.js was generated by the grunt build. Should I revert it? |
Conflicts: dist/js/axs_testing.js - reverted to master branch file
|
Reverted file ax_testing.js to match remote master. |
| @@ -1545,7 +1545,7 @@ axs.AuditRule.collectMatchingElements = function(a, b, c, d) { | |||
| for (e = f.getDistributedNodes(), f = 0;f < e.length;f++) { | |||
There was a problem hiding this comment.
@davidhsv this file needs to be checked out entirely.
|
@davidhsv looks like there's a merge conflict. Once that's resolved and you remove |
|
I've fixed the Gruntfile so that it won't create spurious changes to |
|
Apologies for coming late to this. This LGTM 👍 once the merge issues are fixed. |
|
@davidhsv bump. Can you resolve the merge conflicts and push again? Thanks! |
Conflicts: dist/js/axs_testing.js src/audits/FocusableElementNotVisibleAndNotAriaHidden.js
|
@ckundo Done 👍 |
#149 Be able to evaluate page with iframes
Disabling webSecurity in phantomJs -> so we can get the document root from frames.