dom-accessibility-api icon indicating copy to clipboard operation
dom-accessibility-api copied to clipboard

`computeAccessibleDescription` throws when passed a detached element

Open jlp-craigmorten opened this issue 1 year ago • 1 comments

Triaging from https://github.com/guidepup/virtual-screen-reader/issues/48

When computeAccessibleDescription(element) is passed an element that is not attached to the document it throws the following error:

TypeError: root.getElementById is not a function

      at node_modules/dom-accessibility-api/sources/util.ts:103:5
          at Array.map (<anonymous>)
      at root (node_modules/dom-accessibility-api/sources/util.ts:101:17)
      at computeAccessibleDescription (node_modules/dom-accessibility-api/sources/accessible-description.ts:16:31)
      at Object.<anonymous> (src/test/int/aaa.int.test.ts:34:35)

The issue stems from https://github.com/eps1lon/dom-accessibility-api/blob/d7e6e3dbea2460777d1355406c8d0899d5346a2d/sources/util.ts#L96C16-L96C32:

  1. In the case of a detached element the node.getRootNode resolves to the node itself.
  2. getElementById() is a method unique to the Document interface and thus doesn't exist on the node and we get an error.

To reproduce:

import {
  computeAccessibleDescription,
  computeAccessibleName,
  getRole,
} from "dom-accessibility-api";

describe("Isolated Node", () => {
  // Passes
  it("should compute the role", async () => {
    const isolatedNode = document.createElement("button");
    isolatedNode.textContent = "test-button-text-content";

    expect(getRole(isolatedNode)).toEqual("button");
  });

  // Passes
  it("should compute the accessible name", async () => {
    const isolatedNode = document.createElement("button");
    isolatedNode.textContent = "test-button-text-content";

    expect(computeAccessibleName(isolatedNode)).toEqual(
      isolatedNode.textContent
    );
  });

  // Fails with `TypeError: root.getElementById is not a function`
  it("should compute the accessible description", async () => {
    const html = `<button id="button" aria-describedby="description">test-button-text-content</button>
    <p id="description">
      test-description-text-content
    </p>`;

    const isolatedNode = document.createElement("body");
    isolatedNode.innerHTML = html;

    expect(
      computeAccessibleDescription(isolatedNode.querySelector("#button"))
    ).toEqual(isolatedNode.textContent);
  });
});

There are some options:

  1. Use node.ownerDocument in all cases - there appears to be precedent for just using this elsewhere in the project (opposed to feature detecting node.getRootNode). Not certain what the impact would be here?
  2. Leave the root node logic alone and use root.querySelector(`#${CSS.escape(id)}`); as an alternative to getElementById().
  3. Add docs to state that this isn't a supported usage - ideally with an error that explains why instead of an uncaught exception.

jlp-craigmorten avatar Feb 08 '24 13:02 jlp-craigmorten

Having a quick play it seems:

  • Option (1) is more involved than stated, simply replacing results in Shadow DOM test failures.
  • Option (2) currently fails tests with CSS is not defined errors. Browser support is reasonable (REF: https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static#browser_compatibility) but depending on the needs of this library, it may be not suitable to assume CSS is defined. There are polyfills available, see https://github.com/mathiasbynens/CSS.escape/blob/master/css.escape.js.

jlp-craigmorten avatar Feb 08 '24 13:02 jlp-craigmorten