addyosmani/a11y

Audit skips over React components/Phantom JS woes

Closed this issue · 4 comments

We ran the audit against this page: http://reddizen.com/r/aww/3e7xu3/domain/imgur.com

The cat image in the right panel - which does not have an alt tag - is added via React. If we run the ADT runner on this page in the browser, it catches the lack of alt tag on the cat image. However, when we run a11y, it doesn't catch this. We tried similar tests on other react sites, and most of the time a11y didn't catch errors on (non-server-side-rendered) React elements.

We figured out that the issue is PhantomJS. We modified our local version of a11y to use the system version of Phantom JS (version 2.0.0) instead of the node_module version (version 1.9.x) and then a11y caught all accessibility errors on React components. 😒

Unfortunately, since there is no node_module for Phantom 2, we can't really provide a fix for all users. However, as a (hopefully) temporary solution, we would like to be able to customize the headless browser that a11y runs on. Ideally, we'd like to run a11y with SlimerJS. SlimerJS caught all errors for our React components.

Does this sound good? Open to a PR?

cc/ @kennyw12 @ctaymor @matt-royal @stubbornella

Unfortunately poorer support for SPAs has been something on my radar for a while, but I definitely want to see us improve it.

We modified our local version of a11y to use the system version of Phantom JS (version 2.0.0) instead of the node_module version (version 1.9.x) and then a11y caught all accessibility errors on React components

That's really weird. I remember manually testing this against both Polymer, Ember and Angular apps and didn't see significant improvements with 2.0.0. If you're seeing otherwise, I think it makes sense to work towards bumping and verifying everything works as expected. Is https://www.npmjs.com/package/phantomjs2 a viable option for us?

Ideally, we'd like to run a11y with SlimerJS. SlimerJS caught all errors for our React components.

What would this look like? :) I'm open to a PR. We might need to figure out how well this continues to look for CI integrations like the one @stubbornella is using, but I'm interested for sure.

@stubbornella is our Product Manager :-)

Unfortunately, PhantomJS2 wouldn't work for us because it has proven unstable on TravisCI when we've tried to use it.

How would you feel about adding a CLI option that would switch over to slimerjs? This would likely require that we also include slimerjs as a dependency.

If we were to submit a pull request, how would you want us to test this? Would it be enough to add a single integration test proving that a11y runs and gives proper output using slimerjs?

cc/ @gpleiss @ctaymor @matt-royal

We just realized that we did not try the phantomjs2 npm package when we had our trouble with phantomjs2 on TravisCI. We'll give it a try and get back to you tonight or tomorrow.

EDIT:
The phantomjs2 npm package also gives us trouble on TravisCI

Turns out we were able to solve our issues by polyfilling bind. (See PR). So if that is an acceptable solution for y'all, that would fix this issue for us!

The example we posted prior is no longer working for unrelated reasons. We used this page as an example.