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

new_audit: table-fake-caption, html-xml-lang-mismatch, input-button-name #15098

Merged
merged 20 commits into from
May 24, 2023

Conversation

jazyan
Copy link
Collaborator

@jazyan jazyan commented May 19, 2023

See http://go/prcpg for the current list of audits to be enabled.

Audits added:

  • table-fake-caption (docs)
  • html-xml-lang-mismatch (docs)
  • input-button-name (docs)

Updated tests and also alphabetized most of the audits in gatherers/accessibility.js. I'll check which enabled ones are wcag2a or wcag2aa as I enable more audits.

@jazyan jazyan requested a review from a team as a code owner May 19, 2023 17:33
@jazyan jazyan requested review from adamraine and removed request for a team May 19, 2023 17:33
@jazyan jazyan changed the title new_audits: table-fake-caption, html-xml-lang-mismatch, frame-focusable-content new_audit: table-fake-caption, html-xml-lang-mismatch, frame-focusable-content May 19, 2023
@@ -503,6 +503,11 @@ const expectations = {
],
},
},
// TODO(jasmineyan): why is this not applicable?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a test case to a11y_tester.html for frame-focusable-content that should be a failing case based on the axe docs (https://dequeuniversity.com/rules/axe/4.7/frame-focusable-content#:~:text=Incorrect%20markup%20solutions).

But strangely it's registering as not applicable... I tried using src instead of srcdoc, and checked if removing <section> did anything, but no success there. I also verified that the button in the iframe is indeed not focusable when I brought up http://localhost:10503/a11y/a11y_tester.html, and is focusable by default without tabindex=-1 🤔. Any ideas appreciated here, maybe I'm missing something totally obvious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hit this too: #12536 (comment) src=path/to/index.html should work, srcdoc=<inline html> didn't (perhaps I was mistaken and neither worked)

Later in the thread brendan mentions that he couldn't get anything to fail this audit. I'm pasting the axe snippet we use and am seeing the same. First glance, it looks like axe is trying to send a "command" to an instance of axe it expects to be inside an iframe... but we only inject axe into the main document! https://github.com/dequelabs/axe-core/blob/1d1af0936144e342d231ff9fba348066632cf9e1/lib/core/utils/collect-results-from-frames.js#L26

Copy link
Member

@adamraine adamraine May 19, 2023

Choose a reason for hiding this comment

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

I just tested with the axe extension and it called this out as expected 😕

So somehow we're not surfacing it.

Edit: didn't refresh to see connor's comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll work on adding support for this 👍

Copy link
Member

Choose a reason for hiding this comment

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

Support for what?

Copy link
Collaborator

@connorjclark connorjclark May 19, 2023

Choose a reason for hiding this comment

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

Adding axe lib to iframes in the main document, so axe has a way to gather the info it needs for frame-focusable-content.

Copy link
Collaborator

@connorjclark connorjclark May 19, 2023

Choose a reason for hiding this comment

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

So it's definitely possible to change our gatherer to allow frame-focusable-content to work. It's pretty hairy, though.

branch: https://github.com/GoogleChrome/lighthouse/tree/hacky-axe-frames

  • gotta inject the axe source into every iframe target. For this, I just iterate over all the execution contexts in target manager and do runtime.evaluate. We need to introduce a new api to target manager to "Give me the iframe execution contexts" - right now I just do all of them. Also should probably limit to just iframes from the main document (no nested iframes)
  • axe configuration application name must be the default to work, otherwise postMessages are just ignored
  • since results from an iframe go through postMessage, we lost info on node details. To work around that I added a hack to call getNodeDetails within the axe code, and grab the resulting object when we construction the artifact
  • we don't want to surface every a11y issue in iframes (very likely to be unfixable by user...), so I hacked the message that axe sends to iframes to run itself - usually it sends the whole options object. Instead the hacked version sends a config for just frame-focusable-content. (aside: it could be useful to include these results, but somehow mark them as 3p based on the iframe origin)
  • can confirm things work in the branch by running on a11y_tester_framed.html and looking at the Accessibility artifact. You'll see:
image

any solution here is going to require rather hacky modifications to the axe source code, so if we really want this audit we'll have to maintain a minor soft-fork. See the axe-tmp.js in my branch.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't know if the ROI is there. You'd need not just iframe execution contexts, but a canonical context so you aren't accidentally running multiple times per frame, iframes bring new CSP and same-origin script execution issues, devtoolsNodePaths don't currently work across iframe boundaries (though if the node was just the iframe that could work), even limiting to the single audit we'd have at least the perf overhead of building the axe virtual DOM per frame, etc.

Seems like there are three options:

  • continue to not include the audit
  • hack time: to add another level to the hackiness, we could even Runtime.evaluate just a focusable content check per iframe and then dress it up to look like a postMessaged result that the main frame's axe-core could merge together
  • heavier weight: the axe team maintains libraries for major testing frameworks themselves: https://github.com/dequelabs/axe-core-npm/. We could just use @axe-core/puppeteer, which already handles iframes. Still have to deal with all the non-execution-context issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks all for looking into this! Decided to not include frame-focusable-content given how involved it would be.

@jazyan jazyan changed the title new_audit: table-fake-caption, html-xml-lang-mismatch, frame-focusable-content new_audit: table-fake-caption, html-xml-lang-mismatch, input-button-name May 22, 2023
'html-xml-lang-mismatch': {
score: null,
scoreDisplayMode: 'notApplicable',
},
Copy link
Member

@adamraine adamraine May 24, 2023

Choose a reason for hiding this comment

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

In order to test this, we would need to add lang and xml:lang attributes to the <html> tag? And that would force the existing html-has-lang audit to pass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! I figured that this is still useful because I wasn't sure if html-xml-lang-mismatch also checked for the existence + validity of the lang attribute -- as in, whether this was html-has-lang + checking xml:lang, which the docs seemed to imply. But given that this is notApplicable, seems like that's not the case, and this solely focuses on xml:lang.

core/audits/accessibility/html-xml-lang-mismatch.js Outdated Show resolved Hide resolved
core/audits/accessibility/input-button-name.js Outdated Show resolved Hide resolved
Comment on lines 21 to 22
failureTitle: '`<html>` element does not have an `[xml:lang]` attribute with the same value ' +
'as the `[lang]` attribute.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
failureTitle: '`<html>` element does not have an `[xml:lang]` attribute with the same value ' +
'as the `[lang]` attribute.',
failureTitle: '`<html>` element does not have an `[xml:lang]` attribute with the same base language ' +
'as the `[lang]` attribute.',

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

Successfully merging this pull request may close these issues.

5 participants