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

deps: update axe-core to latest #7020

Merged
merged 5 commits into from
Jan 15, 2019
Merged

deps: update axe-core to latest #7020

merged 5 commits into from
Jan 15, 2019

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Jan 15, 2019

Taking over from #5565.

The PR is so big mostly because all the Learn more links now point to the 3.1 docs instead of the 2.2 ones and I updated the Accessibility sample artifact.

fixes #5940

@brendankenny
Copy link
Member Author

addressed problems in #5565 (comment):

  • accesskeys - moved to a manual audit
  • layout-table updated the smoke test so it wasn't being marked as notApplicable, but try as I might (even running through the official test cases) I couldn't get it to stay failing (it was either notApplicable or passing)

also fixes #5940

@@ -42,7 +42,7 @@ test_script:
# FIXME: Exclude Appveyor from running `perf` smoketest until we fix the flake.
# https://github.com/GoogleChrome/lighthouse/issues/5053
# - yarn smoke
- yarn smoke ally pwa pwa2 pwa3 dbw redirects seo offline byte metrics
- yarn smoke a11y pwa pwa2 pwa3 dbw redirects seo offline byte metrics
Copy link
Member Author

Choose a reason for hiding this comment

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

no idea why this was ally, but that's terrible :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

some artifact weirdness, but I'm not sure it's anything blocking we could fix quickly

LGTM!

"nodes": [
{
"impact": "serious",
"html": "<h2>Do better web tester page</h2>",
"target": [
"div > h2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

regression or good thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

regression or good thing?

I mean, I guess it's accurate, so that's enough? It is the only h2 in the page. I'm not sure if they recently optimized their selector selection for length or something. Agree they're worse for humans, though.

"nodes": [
{
"impact": "critical",
"html": "<img src=\"lighthouse-480x318.jpg\" width=\"480\" height=\"57\">",
"target": [
"body > img[src$=\"lighthouse-480x318.jpg\"]:nth-child(5)"
"img[height=\"\\35 7\"]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this definitely seems worse :(

"snippet": "<input type=\"password\" onpaste=\"event.preventDefault();\">"
},
{
"impact": "critical",
"html": "<input type=\"password\">",
"target": [
"body > input[type=\"password\"]:nth-child(18)"
"input:nth-child(25)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah a lot of these seem less descriptive

"snippet": "<input type=\"password\">"
},
{
"impact": "critical",
"html": "<input type=\"password\" onpaste=\"return false;\">",
"target": [
"body > input[type=\"password\"]:nth-child(19)"
"input[onpaste=\"return\\ false\\;\"]"
Copy link
Collaborator

@patrickhulce patrickhulce Jan 15, 2019

Choose a reason for hiding this comment

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

but some are betterish.... strange

}
},
{
"node": {
"type": "node",
"selector": "body > img[src$=\"lighthouse-480x318.jpg\"]:nth-child(6)",
"path": "3,HTML,1,BODY,7,IMG",
"selector": "img[height=\"\\33 18\"]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah IMO these bad boys are real regressions :/

I guess we could file an upstream issue and hope they improve it soon?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could file an upstream issue and hope they improve it soon?

Looks like it might have been dequelabs/axe-core#731. One of the comments:

I'm also not a big fan of dropping the parent selector part. One of the selectors i saw because of that was b. Nice and short, but it seems unlikely the user will just be able to guess where that is.

but looks like they were ok with that.

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.

Accessibility false reports regarding <dl>, <dd>, and <dt>
3 participants