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 3.5.1 #10344

Merged
merged 9 commits into from
Mar 4, 2020
Merged

deps: update axe-core to 3.5.1 #10344

merged 9 commits into from
Mar 4, 2020

Conversation

jayaddison
Copy link
Contributor

Summary
Updates axe-core==3.5.1 following its release on 2020-02-13 (release tag).

@patrickhulce
Copy link
Collaborator

Thanks very much @jayaddison! Looks like we have a few nice changes in that 3.5.0 update (though meta-viewport as best-practice appears to be a breaking change for us)

  • failureSummary for incomplete results now hurray! 🎉 Did your earlier PR already use those results or do we need to update now?
  • meta-viewport is now a best practice instead of WCAG failure, so we might need to manually run it.
  • A few new rules we might want to disable until we have audits identical-links-same-purpose, no-autoplay-audio, svg-img-alt

@jayaddison

This comment has been minimized.

@GoogleChrome GoogleChrome deleted a comment from shyprology Feb 16, 2020
@jayaddison
Copy link
Contributor Author

I'm not really sure why yet, but following a more successful attempt at git bisect diagnostics, the merge of dequelabs/axe-core#1980 seems to be related to the failing test case in this axe-core version bump.

@jayaddison

This comment has been minimized.

@jayaddison
Copy link
Contributor Author

Ok, I'm gradually gaining more context here.

The failing test is an accessibility self-test over the HTML output produced by the lighthouse report generator.

The axe-core 3.5.1 rule which flags the failure is designed to ensure that perceivable elements in a rendered page belong to accessibility landmarks, as documented in ARIA's authoring practices.

From examining the situation locally, it looks like the updated CSS selector for the regions rule leads to the test failure.

I've not yet been able to work out why the new selector changes the test behaviour -- it still looks valid in theory against the lighthouse HTML report (the updated selector is designed to select all elements contained within the body tag). My best guess at the moment is that the trailing wildcard isn't working as expected.

NB: A wildcard selector is only used in one other axe-core 3.5.1 rule - the hidden-content rule (ref).

@jayaddison
Copy link
Contributor Author

In dequelabs/axe-core#2047, @straker helped determine that the test failures here appeared valid.

Commit 14f0454 modifies the rendering test to ensure that content is rendered inside a main element, which resolves a majority of the errors.

My working theory is that the test in question may not have been working as expected in the past, and that the axe-core 3.5.1 version bump may have exposed some valid issues to investigate.

@jayaddison

This comment has been minimized.

@patrickhulce
Copy link
Collaborator

You can add the new audits to

'tabindex': {enabled: true},
'accesskeys': {enabled: true},
'heading-order': {enabled: true},
'duplicate-id': {enabled: false},
'table-fake-caption': {enabled: false},
'td-has-header': {enabled: false},
'marquee': {enabled: false},
'area-alt': {enabled: false},
'aria-dpub-role-fallback': {enabled: false},
'html-xml-lang-mismatch': {enabled: false},
'blink': {enabled: false},
'server-side-image-map': {enabled: false},

with enabled: false and a little comment just saying not implemented yet :)

@jayaddison

This comment has been minimized.

Comment on lines +290 to +291
'duplicate-id-aria': {enabled: false},
'landmark-no-duplicate-contentinfo': {enabled: false},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not completely certain whether it's fair to associate both of these disabled tests with #9432. duplicate-id-aria seems likely to be related; I'm not 100% sure on whether landmark-no-duplicate-contentinfo is.

@jayaddison

This comment has been minimized.

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.

sorry for the delay @jayaddison, paul is traveling so I'll take over this PR

I think the diff will become much, much smaller after these changes, might be easier to start fresh with a git reset origin/master && git add -A && git commit -m 'reset' and cherry-pick back in your accessibility gatherer changes

@jayaddison

This comment has been minimized.

package.json Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

Yes, diff looks beautiful now! 👍

Not sure what is up with that travis failure, looks like it was caused by a previous failure of firefox extension build crashing and we didn't get a chance to build all assets? My best guess make sure it's merged with master and we'll retry.

@jayaddison
Copy link
Contributor Author

(confirmed up-to-date with master)

@patrickhulce
Copy link
Collaborator

hm weird, just cleared the cache for that PR and we'll see if it fixes things.

@jayaddison
Copy link
Contributor Author

 FAIL  ./dist/lighthouse-dt-bundle.js: 451.81KB > maxSize 450KB (gzip) 

So close :)

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.

thanks very much @jayaddison!

@patrickhulce patrickhulce merged commit a5ff4f5 into GoogleChrome:master Mar 4, 2020
@jayaddison jayaddison deleted the deps-axe-core-3.5.1 branch March 4, 2020 18:40
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.

None yet

5 participants