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

fix(skip-link,region): Allow multiple skiplinks at page top #1496

Merged
merged 6 commits into from
Apr 26, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Apr 15, 2019

Redefine a skip link as any anchor element whose href starts with #... and which precedes the first anchor element whose href doesn't start with #... (that is, a link to a page)

Closes: #1346
Closes: #863

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @WilcoFiers

@straker straker requested a review from a team as a code owner April 15, 2019 16:14
@straker
Copy link
Contributor Author

straker commented Apr 15, 2019

We should hold off merging this for just a bit. As it stands, it increases the time of the skip-link rule on a page of >11k elements from ~30ms to ~140ms (not huge, but still). We should figure out a way to cache the firstPageLink so we only have to do it once for the entire run instead of for each anchor element.

lib/commons/dom/is-skip-link.js Outdated Show resolved Hide resolved
lib/commons/dom/is-skip-link.js Show resolved Hide resolved
@straker
Copy link
Contributor Author

straker commented Apr 17, 2019

When #1503 is merged, I can cache the firstPageLink so we won't have to recalculate it for each node.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Very minor comment.

test/checks/navigation/skip-link.js Outdated Show resolved Hide resolved
test/checks/navigation/skip-link.js Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers changed the title fix(skip-link,region): improve definition of skip-link fix(skip-link,region): Allow multiple skiplinks at the top a page Apr 26, 2019
@WilcoFiers WilcoFiers changed the title fix(skip-link,region): Allow multiple skiplinks at the top a page fix(skip-link,region): Allow multiple skiplinks at page top Apr 26, 2019
@straker straker merged commit 642c8f1 into develop Apr 26, 2019
@straker straker deleted the improveSkipLink branch April 26, 2019 17:30
stephenmathieson added a commit to mohanraj-r/axe-core that referenced this pull request May 10, 2019
* develop: (50 commits)
  docs: add jsdom example and tests (dequelabs#1530)
  fix(aria-valid-attr-value): allow aria-owns to pass when element is not in the DOM (dequelabs#1526)
  fix(isSkipLink): cache first page link (dequelabs#1525)
  chore: update Babel dependencies (dequelabs#1527)
  chore(package): Update karma to version 4.1.0 (dequelabs#1528)
  feat: Improve perf of axe.run [WWD-1821] (dequelabs#1503)
  fix(prettier): ignore generated api doc files (dequelabs#1522)
  fix(skip-link,region): Allow multiple skiplinks at page top (dequelabs#1496)
  fix(raw-reporter): do not output `DqElement`s (dequelabs#1513)
  fix: Scroll state had top and left properties flipped (dequelabs#1469)
  refactor: commons.color.getBackgroundColor method (dequelabs#1451)
  fix(aria-valid-attr-value): allow aria-controls to pass when element is not in the DOM
  chore: Update husky to the latest version 🚀 (dequelabs#1514)
  style: format HTML files with Prettier (dequelabs#1508)
  test: Fix invalid test html (dequelabs#1502)
  feat(rule): Inline text spacing must be adjustable with custom stylesheets (dequelabs#1446)
  chore: Remove version number from axe.d.ts (dequelabs#1499)
  chore: Update make-dir to the latest version 🚀 (dequelabs#1465)
  fix: Exclude  iframe for html-has-lang rule (Issue 1424) (dequelabs#1430)
  feat(utils): add support for complex CSS selectors (dequelabs#1494)
  ...
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.

Incorrect skiplink error Skip links and landmark regions
2 participants