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

core(a11y): add back html/tags/impact #5439

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Conversation

patrickhulce
Copy link
Collaborator

Related Issues/PRs
closes #5402

}
},
{
"node": {
"type": "node",
"selector": "body > a[href$=\"mailto:inbox@email.com\"]",
"path": "3,HTML,1,BODY,29,A",
"snippet": "<a href=\"mailto:inbox@email.com\" target=\"_blank\">"
"snippet": "<a href=\"mailto:inbox@email.com\" target=\"_blank\"></a>",
"axeFailureSummary": "Fix all of the following:\n Element is in tab order and does not have accessible text\n\nFix any of the following:\n Element does not have text that is visible to screen readers\n aria-label attribute does not exist or is empty\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty or not visible\n Element's default semantics were not overridden with role=\"presentation\"\n Element's default semantics were not overridden with role=\"none\""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joelhsmith it seems like in several cases these can be static

Do you have a sense of which cases they're needed per-node? Is it just color-contrast?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through axe-core, I found these:

aria-allowed-attr
aria-required-children
aria-required-parent
aria-valid-attr-value
aria-valid-attr
color-contrast
fieldset
group-labelledby
duplicate-id
non-empty-if-present

I would not be surprised in the future if even more of them became contextual in the future. Plus people can write their own checks which also might be contextual.

@@ -56,7 +59,7 @@ class AxeAudit extends Audit {
extendedInfo: {
value: rule,
},
details: Audit.makeTableDetails(headings, items),
details: {...Audit.makeTableDetails(headings, items), impact, tags},
Copy link
Member

Choose a reason for hiding this comment

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

should impact and tags be nested or something? I don't really have an opinion, this seems fine and nesting might not be any better, but it may be putting us on a path toward details becoming the new grab bag :)

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I like it where it is.

If it were nested, I can't think of what it would be called. On Deque's site "impact" and "tags" meta is extrapolated to create the "Compliance Data & User Impact" column. "complianceUserImpact:" seems a little long anyway ;-)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

seems ok. one question

@@ -42,7 +44,8 @@ class AxeAudit extends Audit {
type: 'node',
selector: Array.isArray(node.target) ? node.target.join(' ') : '',
path: node.path,
snippet: node.snippet,
snippet: node.html || node.snippet,
axeFailureSummary: node.failureSummary,
Copy link
Member

Choose a reason for hiding this comment

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

how about just summary

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 11, 2018

Choose a reason for hiding this comment

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

discussed in person, landed on explanation to match with our audit-level naming 👍

@patrickhulce patrickhulce merged commit 4c47fb0 into master Jun 11, 2018
@patrickhulce patrickhulce deleted the more_a11y_details branch June 11, 2018 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lighthouse 3 Regression in Reporting Data
4 participants