Skip to content

fix(idref): fallback to qsa #4844

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

fix(idref): fallback to qsa #4844

wants to merge 1 commit into from

Conversation

straker
Copy link
Contributor

@straker straker commented Jul 30, 2025

No description provided.

@WilcoFiers WilcoFiers self-requested a review July 30, 2025 16:46
result.push(doc.getElementById(token));
} catch {
// don't run QSA on detached nodes or partial trees
const root = closest(vNode, 'html');
Copy link
Contributor

@dbjorge dbjorge Jul 30, 2025

Choose a reason for hiding this comment

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

I'm not so sure this special case belongs here specifically; there are lots of similar cases where getRootNode is being used similarly. I wonder if it'd be better to implement a vNode-based getRootNode (probably one that uses the existing getRootNode for a vNode with an actualNode and which otherwise walks up parents looking for a null/undefined/#document/#document-fragment case and taking the child of that, rather than looking for html specifically), and then doing a more broad audit of places where the non-vNode-based getRootNode is currently used to see where else it would make sense to apply something similar (even if we want those to be separate PRs)

Copy link
Contributor Author

@straker straker Jul 30, 2025

Choose a reason for hiding this comment

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

I tested the performance of doing something like this. I was worried that getRootNode and getElementById would be way more performant that us looking up the parent tree for root manually and then calling qsa on it. However with a page of 2000 nested nodes deep (was all I could create before crashing the page each time), doing the manual lookup and qsa was only 5ms while a getRootNode and getElementById was 0ms (basically instant). So while there is a difference, it's not something I would except to be problematic on an actual page (which when testing was only 1.5ms).

let parent = fragment = document.createDocumentFragment();
for (let i = 0; i < 2000; i++) {
  const div = document.createElement('div');
  div.textContent = i
  div.id = 't' + i
  parent.append(div);
  parent = div;
}

document.body.append(fragment);

axe.setup();
const node = t1999 // the deepest nested node

window.performance.clearMarks();
window.performance.clearMeasures();

window.performance.mark('start');
const root = axe.utils.closest(axe.utils.getNodeFromTree(node), 'html')
const vNode = axe.utils.querySelectorAllFilter(root, '#t988', root.shadowId)[0]
window.performance.mark('end');

window.performance.measure('time', 'start', 'end')
console.log(window.performance.getEntriesByName('time')[0].duration)

As a side note, doing an id lookup on the root vNode is an O(1) lookup since we cache the _selectorMap on the root node, so the 5ms is entirely parent lookup calls and matching it against the selector. We could probably save some time if we didn't need to match against a selector when finding the root node.

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.

2 participants