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

target-offset check: targets containing shadow children cause false positives #4194

Closed
1 task done
dbjorge opened this issue Oct 12, 2023 · 0 comments · Fixed by #4410
Closed
1 task done

target-offset check: targets containing shadow children cause false positives #4194

dbjorge opened this issue Oct 12, 2023 · 0 comments · Fixed by #4410
Assignees
Labels
fix Bug fixes rules Issue or false result from an axe-core rule target-size

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Oct 12, 2023

Product

axe-core

Product Version

4.8.2

Latest Version

  • I have tested the issue with the latest version of the product

Issue Description

Expectation

The target-offset check uses a get-target-rects helper to break down a "target" into a collection of rects. Part of that work involves checking for any other elements that might be obscuring that target. That check is meant to treat children of the target as part of the target, but the way it does so isn't compatible with a target that contains shadow dom children - it assumes that parentElement.contains(childElementWithinShadow) will return true, but it doesn't. We already have a shadow-aware version of contains (at /lib/core/utils/contains.js) which this should use instead. We should audit for other uses of the wrong contains as part of fixing this issue. Bonus points for a new eslint no-restricted-syntax config that warns us if we accidentally try to use the native one directly again in the future.

This causes getTargetRects to treat the child of the target as obscuring the target. In the motivating case, this ends up treating the target as completely obscured, which causes getTargetRects to return an empty array.

This in turn triggers a separate but related issue in target-offset - when a target's neighbor has an empty target rect array, it treats that neighbor as being 0 distance away from the target. It should treat fully-obscured neighbors as omitted from consideration for the check, not as being 0 distance away.

Actual

In the repro snippet below, axe.run({runOnly: 'target-size'}) emits a false positive violation for a case that ought to pass the SC's spacing exception.

How to Reproduce

<script>
  const shadowTemplate = document.createElement('template')
  shadowTemplate.innerHTML = '<div id="shadow-container"><slot></slot></div>';
  class ShadowOpenWebComponent extends HTMLElement {
    connectedCallback() {
      const shadowRoot = this.attachShadow({ mode: 'open' });
      shadowRoot.appendChild(shadowTemplate.content.cloneNode(true));
    }
  }

  customElements.define('shadow-open', ShadowOpenWebComponent);
</script>
<style>
  #container {
    font-size: 16px;
  }
  #title {
    background-color: #cff;
  }
  #content {
    background-color: #cfc;
    margin-top: 10px;
  }
</style>
<div id="container">
  <div id="title">
    <!-- getTargetRects will incorrectly return [] for this element... -->
    <a id="title-link" href="#target">
      <shadow-open>
        <!-- ...because it doesn't consider this div to be "contained by" #title-link, so thinks it's obscuring it instead -->
        <div>Title</div>
      </shadow-open>
    </a>
  </div>
  <div id="content">
    <!-- target-offset check will incorrectly consider this element to have offset distance 0 from #title-link -->
    <a id="content-link" href="#target">
      <div>Content</div>
    </a>
  </div>
</div>

Additional context

@dbjorge dbjorge added the ungroomed Ticket needs a maintainer to prioritize and label label Oct 12, 2023
@WilcoFiers WilcoFiers added fix Bug fixes rules Issue or false result from an axe-core rule and removed ungroomed Ticket needs a maintainer to prioritize and label labels Oct 13, 2023
@WilcoFiers WilcoFiers added this to the Axe-core 4.9 milestone Oct 13, 2023
@straker straker self-assigned this Apr 15, 2024
@straker straker modified the milestones: Axe-core 4.10, Axe-core 4.9.1 Apr 15, 2024
straker added a commit that referenced this issue Apr 23, 2024
This also adds to the `.eslintrc` to error if `node.contains()` or
`vNode.actualNode.contains()` or used. (also upgraded the
`node.attributes` error to account for `vNode.actualNode.attributes` as
I noticed it was missing).

Closes: #4194

---------

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes rules Issue or false result from an axe-core rule target-size
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants