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

Jump to bottom

fix: children and childNodes not returning top-level slotted children #4098

Merged
merged 4 commits into from Mar 27, 2024
Merged

fix: children and childNodes not returning top-level slotted children #4098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9843b5a
fix: children and childNodes not returning top-level slotted children
abdulsattar Mar 25, 2024
6166bbe
fix: revert getAllMatches and do no filtering
abdulsattar Mar 27, 2024
316f47a
Update packages/@lwc/synthetic-shadow/src/faux-shadow/node.ts
abdulsattar Mar 27, 2024
f902276
fix: comment
abdulsattar Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/@lwc/integration-karma/test/light-dom/root/index.spec.js
View file
Open in desktop
Expand Up @@ -78,5 +78,7 @@ describe('root light element', () => {
expect(nodes['x-list'].querySelector('button')).toEqual(nodes.button);
expect(nodes['x-list'].getElementsByTagName('button')[0]).toEqual(nodes.button);
expect(nodes['x-list'].getElementsByClassName('button')[0]).toEqual(nodes.button);
expect(nodes['x-list'].childNodes[0]).toEqual(nodes.button);
expect(nodes['x-list'].children[0]).toEqual(nodes.button);
});
});
2 changes: 1 addition & 1 deletion packages/@lwc/synthetic-shadow/src/faux-shadow/element.ts
View file
Open in desktop
Expand Up @@ -92,7 +92,7 @@ function shadowRootGetterPatched(this: Element): ShadowRoot | null {

function childrenGetterPatched(this: Element): HTMLCollectionOf<Element> {
const owner = getNodeOwner(this);
const childNodes = isNull(owner) ? [] : getAllMatches(owner, getFilteredChildNodes(this));
const childNodes = getAllMatches(owner, getFilteredChildNodes(this));
return createStaticHTMLCollection(
ArrayFilter.call(childNodes, (node: Node | Element) => node instanceof Element)
);
Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/synthetic-shadow/src/faux-shadow/node.ts
View file
Open in desktop
Expand Up @@ -173,7 +173,7 @@ function cloneNodePatched(this: Node, deep?: boolean): Node {
function childNodesGetterPatched(this: Node): NodeListOf<Node> {
if (isSyntheticShadowHost(this)) {
const owner = getNodeOwner(this);
const childNodes = isNull(owner) ? [] : getAllMatches(owner, getFilteredChildNodes(this));
const childNodes = getAllMatches(owner, getFilteredChildNodes(this));
return createStaticNodeList(childNodes);
}
// nothing to do here since this does not have a synthetic shadow attached to it
Expand Down
23 changes: 16 additions & 7 deletions packages/@lwc/synthetic-shadow/src/faux-shadow/traverse.ts
View file
Open in desktop
Expand Up @@ -134,24 +134,33 @@ export function isSlotElement(node: Node): node is HTMLSlotElement {
return node instanceof HTMLSlotElement;
}

export function isNodeOwnedBy(owner: Element, node: Node): boolean {
export function isNodeOwnedBy(owner: Element | null, node: Node): boolean {
if (process.env.NODE_ENV !== 'production') {
if (!(owner instanceof HTMLElement)) {
// eslint-disable-next-line no-console
console.error(`isNodeOwnedBy() should be called with an element as the first argument`);
}
if (!(node instanceof Node)) {
// eslint-disable-next-line no-console
console.error(`isNodeOwnedBy() should be called with a node as the second argument`);
}
}

// owner can be null if all the parents are in the light DOM
// ownerKey is undefined for such nodes
const ownerKey = getNodeNearestOwnerKey(node);
if (isNull(owner)) {
return isUndefined(ownerKey);
Copy link
Member

Choose a reason for hiding this comment

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

Not so sure about this logic. If the owner is null but ownerKey is defined, then isNodeOwnedBy() returns true. By relaxing the input constraints, it seems that we're muddying the job of this function. Maybe this is a sign that we should be handling the light DOM case elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think of this as node being owned by a shadowRoot or document (in which case it's null). It's owned by document, in case of light DOM parents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would having more tests for childNodes/children help build confidence? I suspect we don’t have a lot.

Copy link
Member

Choose a reason for hiding this comment

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

For both childrenGetterPatched and childNodesGetterPatched, if there is no owner, then it means that the element or node is not shadowed, correct? If so, then it seems like we should immediately return getFilteredChildNodes(this) instead of drilling down into getAllMatches() and isNodeOwnedBy().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Done.

}

if (process.env.NODE_ENV !== 'production') {
if (!(owner instanceof HTMLElement)) {
// eslint-disable-next-line no-console
console.error(`isNodeOwnedBy() should be called with an element as the first argument`);
}
if (!(compareDocumentPosition.call(node, owner) & DOCUMENT_POSITION_CONTAINS)) {
// eslint-disable-next-line no-console
console.error(
`isNodeOwnedBy() should never be called with a node that is not a child node of of the given owner`
);
}
}
const ownerKey = getNodeNearestOwnerKey(node);

if (isUndefined(ownerKey)) {
// in case of root level light DOM element slotting into a synthetic shadow
Expand Down Expand Up @@ -196,7 +205,7 @@ export function getFirstSlottedMatch(host: Element, nodeList: Element[]): Elemen
return null;
}

export function getAllMatches<T extends Node>(owner: Element, nodeList: Node[]): T[] {
export function getAllMatches<T extends Node>(owner: Element | null, nodeList: Node[]): T[] {
const filteredAndPatched: T[] = [];
for (let i = 0, len = nodeList.length; i < len; i += 1) {
const node = nodeList[i];
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/synthetic-shadow/src/shared/node-ownership.ts
View file
Open in desktop
Expand Up @@ -58,6 +58,8 @@ export function getNodeNearestOwnerKey(node: Node): number | undefined {
}
host = parentNodeGetter.call(host) as ShadowedNode | null;

// Elements slotted from top level light DOM into synthetic shadow
// reach the slot tag from the shadow element first
Comment on lines +61 to +62
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand this comment. Are we short-circuiting here so that we don't need to traverse to the root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The <slot> element will belong to some other shadow root and check if the node belongs to the owner of the <slot> element. This check will fail and it won't return any elements.

if (!isNull(host) && isSyntheticSlotElement(host)) {
return undefined;
}
Expand Down