Skip to content
This repository was archived by the owner on Jul 12, 2020. It is now read-only.

Improve auditing for webapps #26

Open
addyosmani opened this issue Jan 1, 2015 · 11 comments
Open

Improve auditing for webapps #26

addyosmani opened this issue Jan 1, 2015 · 11 comments

Comments

@addyosmani
Copy link
Owner

When using the Chrome Accessibility DevTools extension, pages such as this Angular Todo app will display severity issues that are not reported by A11y when conducting an audit.

Example:

vs:

My current theory is that we're not giving the page sufficient time to complete JS execution so not all content is present and in an auditable state.

After experimenting with adding a custom timeout (similar to this) and playing with timeouts of differing values, this doesn't appear to improve our ability to report back on such issues.

These issues aren't specific to this module (as far as I can tell). I've been able to repro the same with https://github.com/GoogleChrome/accessibility-developer-tools. Looking into this further.

@addyosmani
Copy link
Owner Author

Testing against a manually scraped output from the app after a few seconds:

screen shot 2015-01-02 at 22 21 23

Looks like further search into why timeout support isn't solving this will be needed.

@addyosmani
Copy link
Owner Author

@sindresorhus The approach other projects take here is either doing what screenshot-stream does (simple timeout) or waiting for a specific condition to be met before indicating the page is really complete (e.g a ng-complete or similar class is present).

This would take the form of something like this:
https://github.com/ariya/phantomjs/blob/master/examples/waitfor.js

I propose we consider supporting both a waitFor condition (for webapps) and a timeout for more general cases that need more time to finish loading. The latter is pretty straight-forward to add in. The former could reuse portions of the Phantom sample if we think it's worth adding in.

Thoughts? Overkill or no? I'm also thinking about how waitsFor might manifest at a CLI level (if it does at all).

@sindresorhus
Copy link
Contributor

Yeah, I don't see how we would implement that nicely on the CLI. Recommending users to just set a longer timeout might be easier.

Let's implement the timeout thing first and see if it works for people.

@addyosmani
Copy link
Owner Author

We've landed support for delay in 6bc3fe5.

Given there isn't a clean way to implement waitsFor in the CLI, should we add it to the API at all or wait until it's asked for by a few more people?

@sindresorhus
Copy link
Contributor

I would wait. No point in adding stuff people might need. Let's get some external use-cases first that can't be solved with --delay.

@addyosmani
Copy link
Owner Author

Hmm. My experiments with delay against Angular apps with delays of 5, 10, 20 and 30s haven't been able to make much of a difference. I'm a little concerned if we ship without basic support for webapp auditing.

@sindresorhus
Copy link
Contributor

@addyosmani Do you have some public URLs that are failing which I can test against?

@addyosmani
Copy link
Owner Author

http://addyosmani.com/apps/accessibility/

Should fail with [Severe] Controls and media elements should have labels (1)

only raises: Text elements should have a reasonable contrast ratio

http://goodfil.ms/

Expected:

[Severe] ARIA attributes which refer to other elements by ID should refer to elements which exist in the DOM (1)
[Severe] ARIA state and property values must be valid (1)
[Severe] Controls and media elements should have labels (1)
[Warning] Images should have an alt attribute (4)
[Warning] Meaningful images should not be used in element backgrounds (3)
[Warning] Text elements should have a reasonable contrast ratio (89)
[Warning] The purpose of each link should be clear from the link text (41)

Received:

ARIA state and property values must be valid
These elements are focusable but either invisible or obscured by another element
Images should have an alt attribute
Text elements should have a reasonable contrast ratio

http://www.5by.com/

Expected:

[Severe] Controls and media elements should have labels (6)
[Warning] Images should have an alt attribute (3)
[Warning] Meaningful images should not be used in element backgrounds (23)
[Warning] Text elements should have a reasonable contrast ratio (3)
[Warning] The purpose of each link should be clear from the link text (5)

Received:

Images should have an alt attribute
Meaningful images should not be used in element backgrounds

http://www.framefish.com/

Expected:

[Severe] Controls and media elements should have labels (1)
[Warning] Images should have an alt attribute (7)
[Warning] Meaningful images should not be used in element backgrounds (2)
[Warning] Text elements should have a reasonable contrast ratio (37)
[Warning] The purpose of each link should be clear from the link text (25)

Received:

Images should have an alt attribute
Text elements should have a reasonable contrast ratio
Meaningful images should not be used in element backgrounds

@addyosmani
Copy link
Owner Author

@sindresorhus Interested in your opinion: is it worth delaying release until this is supported or should we hold off until we've solved this problem? I'm still planning on meeting with the team behind accessibility-developer-tools to figure out if this is a limitation of their module regardless.

@sindresorhus
Copy link
Contributor

Oh, I thought I commented on this. Must have not come through (such bad internet here...).

Releases are cheap. So no point in holding back.

As for the issue. I have a strong feeling it's because PhantomJS has a >2yo webkit version.

@koenpunt
Copy link

What's the WebKit version in PhantomJs 2?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants
@addyosmani @sindresorhus @koenpunt and others