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.

Sign up for GitHub

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: [1710] - Ensure querySelector returns the first item that appears in the DOM for grouped selectors #1713

Merged
merged 6 commits into from
Feb 8, 2025
Merged

fix: [1710] - Ensure querySelector returns the first item that appears in the DOM for grouped selectors #1713

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2bd2d6f
fix: [1710] - Ensure querySelector returns the first item that appea…
christiango Jan 31, 2025
2735107
fix: [1710] - Ensure querySelector returns the first item that appea…
christiango Jan 31, 2025
4935cfb
Merge branch 'master' into christiango/fix-query-selector-bug
capricorn86 Feb 5, 2025
f5ba93a
fix: [1710] - Ensure querySelector returns the first item that appea…
christiango Feb 7, 2025
c6601bd
fix: [1710] - Ensure querySelector returns the first item that appea…
christiango Feb 7, 2025
2b1926c
Merge branch 'master' into christiango/fix-query-selector-bug
christiango Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 47 additions & 119 deletions packages/happy-dom/src/query-selector/QuerySelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,8 @@ export default class QuerySelector {
}
}

const groups = SelectorParser.getSelectorGroups(selector);
const items: Element[] = [];
const nodeList = new NodeList<Element>(PropertySymbol.illegalConstructor, items);
const matchesMap: Map<string, Element> = new Map();
const matchedPositions: string[] = [];
const cachedItem = {
result: new WeakRef(nodeList)
};
Expand All @@ -130,24 +127,7 @@ export default class QuerySelector {
(node[PropertySymbol.ownerDocument] || node)[PropertySymbol.affectsCache].push(cachedItem);
}

for (const items of groups) {
const matches =
node[PropertySymbol.nodeType] === NodeTypeEnum.elementNode
? this.findAll(<Element>node, [<Element>node], items, cachedItem)
: this.findAll(null, (<Element>node)[PropertySymbol.elementArray], items, cachedItem);
for (const match of matches) {
if (!matchesMap.has(match.documentPosition)) {
matchesMap.set(match.documentPosition, match.element);
matchedPositions.push(match.documentPosition);
}
}
}

const keys = matchedPositions.sort();

for (let i = 0, max = keys.length; i < max; i++) {
items.push(matchesMap.get(keys[i]));
}
this.insertMatchesInOrder(items, node, selector, cachedItem);

return nodeList;
}
Expand Down Expand Up @@ -249,19 +229,16 @@ export default class QuerySelector {
(node[PropertySymbol.ownerDocument] || node)[PropertySymbol.affectsCache].push(cachedItem);
}

for (const items of SelectorParser.getSelectorGroups(selector)) {
const match =
node[PropertySymbol.nodeType] === NodeTypeEnum.elementNode
? this.findFirst(<Element>node, [<Element>node], items, cachedItem)
: this.findFirst(null, (<Element>node)[PropertySymbol.elementArray], items, cachedItem);
const items = [];
this.insertMatchesInOrder(items, node, selector, cachedItem);

if (match) {
cachedItem.result = new WeakRef(match);
return match;
}
if (items.length === 0) {
return null;
}

return null;
const match = items[0];
cachedItem.result = new WeakRef(match);
return match;
}

/**
Expand Down Expand Up @@ -466,6 +443,44 @@ export default class QuerySelector {
return null;
}

/**
* Finds elements that match a query selector on a node in order and updates relevant caches
*
* @param items List of items that will be populated with the matches in order.
* @param node Node to search in
* @param selector Selector to match against
* @param cachedItem Cached item.

*/
private static insertMatchesInOrder(
items: Element[],
node: Element | Document | DocumentFragment,
selector: string,
cachedItem: ICachedQuerySelectorAllItem | ICachedQuerySelectorItem
): void {
const groups = SelectorParser.getSelectorGroups(selector);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main concerns with this change are the following:

  • I'm new to this project so I'm afraid this may have some unexpected consequences due to the complex nature of the caches required to make these perform well
  • I'm not sure how much this impacts performance of the library all up, particularly in less complicated query selector scenarios than the one described in the bug.

const matchesMap: Map<string, Element> = new Map();
const matchedPositions: string[] = [];

for (const items of groups) {
const matches =
node[PropertySymbol.nodeType] === NodeTypeEnum.elementNode
? this.findAll(<Element>node, [<Element>node], items, cachedItem)
: this.findAll(null, (<Element>node)[PropertySymbol.elementArray], items, cachedItem);
for (const match of matches) {
if (!matchesMap.has(match.documentPosition)) {
matchesMap.set(match.documentPosition, match.element);
matchedPositions.push(match.documentPosition);
}
}
}

const keys = matchedPositions.sort();
for (let i = 0, max = keys.length; i < max; i++) {
items.push(matchesMap.get(keys[i]));
}
}

/**
* Finds elements based on a query selector for a part of a list of selectors separated with comma.
*
Expand All @@ -480,7 +495,7 @@ export default class QuerySelector {
rootElement: Element,
children: Element[],
selectorItems: SelectorItem[],
cachedItem: ICachedQuerySelectorAllItem,
cachedItem: ICachedQuerySelectorAllItem | ICachedQuerySelectorItem,
documentPosition?: string
): DocumentPositionAndElement[] {
const selectorItem = selectorItems[0];
Expand Down Expand Up @@ -552,91 +567,4 @@ export default class QuerySelector {

return matched;
}

/**
* Finds an element based on a query selector for a part of a list of selectors separated with comma.
*
* @param rootElement Root element.
* @param children Child elements.
* @param selectorItems Selector items.
* @param cachedItem Cached item.
* @returns HTML element.
*/
private static findFirst(
rootElement: Element,
children: Element[],
selectorItems: SelectorItem[],
cachedItem: ICachedQuerySelectorItem
): Element {
const selectorItem = selectorItems[0];
const nextSelectorItem = selectorItems[1];

for (const child of children) {
const childrenOfChild = (<Element>child)[PropertySymbol.elementArray];

child[PropertySymbol.affectsCache].push(cachedItem);

if (selectorItem.match(child)) {
if (!nextSelectorItem) {
if (rootElement !== child) {
return child;
}
} else {
switch (nextSelectorItem.combinator) {
case SelectorCombinatorEnum.adjacentSibling:
const nextElementSibling = child.nextElementSibling;
if (nextElementSibling) {
const match = this.findFirst(
rootElement,
[nextElementSibling],
selectorItems.slice(1),
cachedItem
);
if (match) {
return match;
}
}
break;
case SelectorCombinatorEnum.descendant:
case SelectorCombinatorEnum.child:
const match = this.findFirst(
rootElement,
childrenOfChild,
selectorItems.slice(1),
cachedItem
);
if (match) {
return match;
}
break;
case SelectorCombinatorEnum.subsequentSibling:
const index = children.indexOf(child);
for (let i = index + 1; i < children.length; i++) {
const sibling = children[i];
const match = this.findFirst(
rootElement,
[sibling],
selectorItems.slice(1),
cachedItem
);
if (match) {
return match;
}
}
break;
}
}
}

if (selectorItem.combinator === SelectorCombinatorEnum.descendant && childrenOfChild.length) {
const match = this.findFirst(rootElement, childrenOfChild, selectorItems, cachedItem);

if (match) {
return match;
}
}
}

return null;
}
}
15 changes: 15 additions & 0 deletions packages/happy-dom/test/query-selector/QuerySelector.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,5 +1931,20 @@ describe('QuerySelector', () => {
expect(sibling2.matches('.a ~ .b')).toBe(true);
expect(sibling2.matches('.a ~ .z')).toBe(false);
});

it('Matches grouped selectors in the right order', () => {
const div = document.createElement('div');

div.innerHTML = `
<div class>
<h1><span>Here is a heading</span></h1>
<div class="a">
<span>With a child span</span>
</div>
</div>
`;

expect(div.querySelector('.a,h1')).toBe(div.children[0].children[0]);
});
});
});
Loading