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

fix: getRootNode when composed is false and the element is the root #337

Merged
merged 2 commits into from
May 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 7 additions & 51 deletions packages/lwc-engine/src/framework/__tests__/dom.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,34 +50,12 @@ describe('dom', () => {
});

it('should return correct value from self', () => {
class Parent extends Element {
handleFoo(evt) {
expect(evt.target).toBe(this.template.querySelector('x-foo'));
}

render() {
return ($api, $cmp) => {
return [
$api.h(
'div',
{
on: {
foo: $api.b($cmp.handleFoo)
},
key: 0,
},
[]
)
]
}
}
}

class Parent extends Element {}
const elm = createElement('x-parent', { is: Parent });
document.body.appendChild(elm);

const match = getRootNode.call(elm, { composed: true });
// We can't assert against document directly, because
// for some reasons, jest is locking up with document here

expect(match.nodeName).toBe('#document');
});
});
Expand Down Expand Up @@ -129,34 +107,12 @@ describe('dom', () => {
});

it('should return correct value from self', () => {
class Parent extends Element {
handleFoo(evt) {
expect(evt.target).toBe(this.template.querySelector('x-foo'));
}

render() {
return ($api, $cmp) => {
return [
$api.h(
'div',
{
on: {
foo: $api.b($cmp.handleFoo)
},
key: 0,
},
[]
)
]
}
}
}

class Parent extends Element {}
const elm = createElement('x-parent', { is: Parent });
document.body.appendChild(elm);
const match = getRootNode.call(elm, { composed: true });
// We can't assert against document directly, because
// for some reasons, jest is locking up with document here

const match = getRootNode.call(elm, { composed: false });

expect(match.nodeName).toBe('#document');
});
});
Expand Down
66 changes: 46 additions & 20 deletions packages/lwc-engine/src/framework/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getOwnPropertyDescriptor,
isUndefined,
isNull,
isTrue,
defineProperties,
} from './language';
import { ViewModelReflection } from "./utils";
Expand All @@ -35,34 +36,59 @@ const {
compareDocumentPosition,
} = Node.prototype;

function findShadowRoot(node) {
let root = node;
while (isUndefined(root[ViewModelReflection])) {
root = root.parentNode;
/**
* Returns the context shadow included root.
*/
function findShadowRoot(node: Node): Node {
// We need to ensure that the parent element is present before accessing it.
if (isNull(node.parentNode)) {
return node;
}
return root;

// In the case of LWC, the root and the host element are the same things. Therefor,
// when calling findShadowRoot on the a host element we want to return the parent host
// element and not the current host element.
node = node.parentNode;
while (
!isNull(node.parentNode) &&
isUndefined(node[ViewModelReflection])
) {
node = node.parentNode;
}

return node;
}

function findComposedRootNode(node: Node) {
while (node !== document) {
const parent = node.parentNode;
if (isNull(parent)) {
return node;
}
node = parent;
/**
* Returns the context root beyond the shadow root.
*
* It doesn't returns actually the root but the host. This approximation is sufficiant
* in our case.
*/
function findComposedRootNode(node: Node): Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain what the problem was? in which scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem was finding the root node on an element rendered in interop. Since there is no parent VM, it was traversing to the document root. Because this isn't inside of a closed root, going to the document root seems correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is already a test case for this. https://github.com/salesforce/lwc/pull/337/files#diff-2a67454c78a85e47d3e8214bb464d731R114

However, if you look at the diff, the composed was set to true instead of false, explaining why we didn't catch that before. I fixed the test.

while (!isNull(node.parentNode)) {
node = node.parentNode;
}

return node;
}

// TODO: once we start using the real shadowDOM, we can rely on:
// const { getRootNode } = Node.prototype;
// for now, we need to provide a dummy implementation to provide retargeting
function getRootNode(this: Node, options: Record<string, any> | undefined): Node {
/**
* Dummy implementation of the Node.prototype.getRootNode.
* Spec: https://dom.spec.whatwg.org/#dom-node-getrootnode
*
* TODO: Once we start using the real shadowDOM, this method should be replaced by:
* const { getRootNode } = Node.prototype;
*/
function getRootNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems functional equivalent, just cosmetic changes. Can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's correct

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right.

this: Node,
options?: { composed?: boolean }
): Node {
const composed: boolean = isUndefined(options) ? false : !!options.composed;
if (!composed) {
return findShadowRoot(this.parentNode); // this is not quite the root (it is the host), but for us is sufficient
}
return findComposedRootNode(this);

return isTrue(composed) ?
findComposedRootNode(this) :
findShadowRoot(this);
}

export function isChildNode(root: Element, node: Node): boolean {
Expand Down