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(engine): fixes #973 to support cloneNode with ie11 devtool comment #974

Merged
merged 5 commits into from
Jan 17, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
6 changes: 3 additions & 3 deletions packages/@lwc/engine/src/faux-shadow/custom-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import { getActiveElement, handleFocusIn, handleFocus, ignoreFocusIn, ignoreFocu
import { HTMLElementConstructor } from "../framework/base-bridge-element";
import { createStaticNodeList } from "../shared/static-node-list";
import { createStaticHTMLCollection } from "../shared/static-html-collection";

const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)';
import { hasNativeSymbolsSupport, isExternalChildNodeAccessorFlagOn } from "./node";

export function PatchedCustomElement(Base: HTMLElement): HTMLElementConstructor {
const Ctor = PatchedElement(Base) as HTMLElementConstructor;
Expand Down Expand Up @@ -98,10 +97,11 @@ export function PatchedCustomElement(Base: HTMLElement): HTMLElementConstructor
get childNodes(this: HTMLElement): NodeListOf<Node & Element> {
const owner = getNodeOwner(this);
const childNodes = isNull(owner) ? [] : getAllMatches(owner, getFilteredChildNodes(this));
if (process.env.NODE_ENV !== 'production' && isFalse(hasNativeSymbolsSupport)) {
if (process.env.NODE_ENV !== 'production' && isFalse(hasNativeSymbolsSupport) && isExternalChildNodeAccessorFlagOn()) {
// inserting a comment node as the first childNode to trick the IE11
// DevTool to show the content of the shadowRoot, this should only happen
// in dev-mode and in IE11 (which we detect by looking at the symbol).
// Plus it should only be in place if we know it is an external actuator.
ArrayUnshift.call(childNodes, getIE11FakeShadowRootPlaceholder(this));
}
return createStaticNodeList(childNodes);
Expand Down
64 changes: 53 additions & 11 deletions packages/@lwc/engine/src/faux-shadow/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
forEach,
getPrototypeOf,
setPrototypeOf,
isFalse,
} from '../shared/language';
import {
parentNodeGetter,
Expand All @@ -31,6 +32,8 @@ import { getShadowRoot } from './shadow-root';
const OwnerKey = '$$OwnerKey$$';
const OwnKey = '$$OwnKey$$';

export const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)';
Copy link
Member

Choose a reason for hiding this comment

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

Let's reuse the shared version:

const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that one might change in the future once we have the weakmaps, so let's keep them separate. Also we plan to disconnect engine from synthetic.


export function getNodeOwnerKey(node: Node): number | undefined {
return node[OwnerKey];
}
Expand All @@ -40,12 +43,18 @@ export function setNodeOwnerKey(node: Node, key: number) {
}

export function getNodeNearestOwnerKey(node: Node): number | undefined {
let ownerKey: number | undefined;
let ownerNode: Node | null = node;
// search for the first element with owner identity (just in case of manually inserted elements)
while (!isNull(node) && isUndefined((ownerKey = node[OwnerKey]))) {
node = parentNodeGetter.call(node);
while (!isNull(ownerNode)) {
if (!isUndefined(ownerNode[OwnerKey])) {
return ownerNode[OwnerKey];
} else if (!isUndefined(ownerNode[OwnKey])) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was causing that for accessing stuff off the root element, it has to traverse all the way to the top (document) to find that that it doesn't have an owner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

additionally, for some reason, IE11 was complaining about this being part of the while statement, so moving it into here makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are inferring that the node is a host element by checking for a key. Can you make it an util like isHostElement() so that the intention is clear and the check can be reused.

// perf optimization:
// root elements have ownKey but not ownerKey, we don't need to walk up anymore
return;
}
ownerNode = parentNodeGetter.call(ownerNode);
}
return ownerKey;
}

export function getNodeKey(node: Node): number | undefined {
Expand All @@ -66,7 +75,7 @@ const portalObserverConfig: MutationObserverInit = {
};

function patchPortalElement(node: Node, ownerKey: number, shadowToken: string | undefined) {
// If node aleady has an ownerkey, we can skip
// If node already has an ownerKey, we can skip
// Note: checking if a node as any ownerKey is not enough
// because this element could be moved from one
// shadow to another
Expand All @@ -76,7 +85,7 @@ function patchPortalElement(node: Node, ownerKey: number, shadowToken: string |
setNodeOwnerKey(node, ownerKey);
if (node instanceof Element) {
setCSSToken(node, shadowToken);
const { childNodes } = node;
const childNodes = getInternalChildNodes(node);
for (let i = 0, len = childNodes.length; i < len; i += 1) {
const child = childNodes[i];
patchPortalElement(child, ownerKey, shadowToken);
Expand Down Expand Up @@ -171,17 +180,17 @@ export function PatchedNode(node: Node): NodeConstructor {
throw new TypeError('Illegal constructor');
}
hasChildNodes(this: Node, ) {
return this.childNodes.length > 0;
return getInternalChildNodes(this).length > 0;
}
// @ts-ignore until [email protected]
get firstChild(this: Node): ChildNode | null {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
// @ts-ignore until [email protected]
return childNodes[0] || null;
}
// @ts-ignore until [email protected]
get lastChild(this: Node): ChildNode | null {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
// @ts-ignore until [email protected]
return childNodes[childNodes.length - 1] || null;
}
Expand All @@ -202,7 +211,7 @@ export function PatchedNode(node: Node): NodeConstructor {
return children.item(children.length - 1) || null;
}
get assignedSlot(this: Node): HTMLElement | null {
const parentNode: HTMLElement = nativeParentNodeGetter.call(this);
const parentNode = nativeParentNodeGetter.call(this);
/**
* if it doesn't have a parent node,
* or the parent is not an slot element
Expand Down Expand Up @@ -263,7 +272,7 @@ export function PatchedNode(node: Node): NodeConstructor {
return clone;
}

const childNodes = this.childNodes;
const childNodes = getInternalChildNodes(this);
for (let i = 0, len = childNodes.length; i < len; i += 1) {
clone.appendChild(childNodes[i].cloneNode(true));
}
Expand All @@ -276,3 +285,36 @@ export function PatchedNode(node: Node): NodeConstructor {
setPrototypeOf(PatchedNodeClass.prototype, Ctor.prototype);
return (PatchedNodeClass as any) as NodeConstructor;
}

let internalChildNodeAccessorFlag = false;

/**
* These 2 methods are providing a machinery to understand who is accessing the
* .childNodes member property of a node. If it is used from inside the synthetic shadow
* or from an external actuator. This helps to produce the right output in one very peculiar
* case, the IE11 debugging comment for shadowRoot representation on the devtool.
*/
export function isExternalChildNodeAccessorFlagOn(): boolean {
return !internalChildNodeAccessorFlag;
}
export const getInternalChildNodes = (process.env.NODE_ENV !== 'production' && isFalse(hasNativeSymbolsSupport)) ?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whenever we use childNodes inside farmework code, we need to go thru this method, otherwise we might get the comment for devtool in IE11

function(node: Node): NodeListOf<ChildNode> {
internalChildNodeAccessorFlag = true;
let childNodes;
let error = null;
try {
childNodes = node.childNodes;
} catch (e) {
// childNodes accessor should never throw, but just in case!
error = e;
} finally {
internalChildNodeAccessorFlag = false;
if (!isNull(error)) {
// re-throwing after restoring the state machinery for setInternalChildNodeAccessorFlag
throw error; // tslint:disable-line
}
}
return childNodes;
} : function(node: Node): NodeListOf<ChildNode> {
return node.childNodes;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in other modes, non-ie11, no devmode, we do nothing but getting the childNodes off the node.

};
12 changes: 7 additions & 5 deletions packages/@lwc/engine/src/faux-shadow/shadow-root.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { createStaticHTMLCollection } from "../shared/static-html-collection";
import { getOuterHTML } from "../3rdparty/polymer/outer-html";
import { retarget } from "../3rdparty/polymer/retarget";
import { pathComposer } from "../3rdparty/polymer/path-composer";
import { getInternalChildNodes } from "./node";

const HostKey = createFieldName('host');
const ShadowRootKey = createFieldName('shadowRoot');
Expand Down Expand Up @@ -269,15 +270,15 @@ const NodePatchDescriptors = {
enumerable: true,
configurable: true,
get(this: SyntheticShadowRootInterface): ChildNode | null {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
return childNodes[0] || null;
},
},
lastChild: {
enumerable: true,
configurable: true,
get(this: SyntheticShadowRootInterface): ChildNode | null {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
return childNodes[childNodes.length - 1] || null;
},
},
Expand All @@ -286,7 +287,8 @@ const NodePatchDescriptors = {
enumerable: true,
configurable: true,
value(this: SyntheticShadowRootInterface): boolean {
return this.childNodes.length > 0;
const childNodes = getInternalChildNodes(this);
return childNodes.length > 0;
},
},
isConnected: {
Expand Down Expand Up @@ -360,7 +362,7 @@ const NodePatchDescriptors = {
enumerable: true,
configurable: true,
get(this: SyntheticShadowRootInterface): string {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
let textContent = '';
for (let i = 0, len = childNodes.length; i < len; i += 1) {
textContent += getTextContent(childNodes[i]);
Expand All @@ -383,7 +385,7 @@ const ElementPatchDescriptors = {
enumerable: true,
configurable: true,
get(this: SyntheticShadowRootInterface): string {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
let innerHTML = '';
for (let i = 0, len = childNodes.length; i < len; i += 1) {
innerHTML += getOuterHTML(childNodes[i]);
Expand Down
18 changes: 10 additions & 8 deletions packages/@lwc/engine/src/faux-shadow/traverse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getNodeKey,
getNodeNearestOwnerKey,
PatchedNode,
getInternalChildNodes,
} from "./node";
import {
childNodesGetter as nativeChildNodesGetter,
Expand Down Expand Up @@ -43,15 +44,16 @@ export function getNodeOwner(node: Node): HTMLElement | null {
if (isUndefined(ownerKey)) {
return null;
}
let nodeOwner: Node | null = node;
// At this point, node is a valid node with owner identity, now we need to find the owner node
// search for a custom element with a VM that owns the first element with owner identity attached to it
while (!isNull(node) && (getNodeKey(node) !== ownerKey)) {
node = parentNodeGetter.call(node);
while (!isNull(nodeOwner) && (getNodeKey(nodeOwner) !== ownerKey)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just fixing types... nothing else.

nodeOwner = parentNodeGetter.call(nodeOwner);
}
if (isNull(node)) {
if (isNull(nodeOwner)) {
return null;
}
return node as HTMLElement;
return nodeOwner as HTMLElement;
}

export function isSlotElement(elm: Element): boolean {
Expand All @@ -76,15 +78,15 @@ export function isNodeSlotted(host: Element, node: Node): boolean {
}
const hostKey = getNodeKey(host);
// just in case the provided node is not an element
let currentElement: Element = node instanceof Element ? node : parentElementGetter.call(node);
let currentElement = node instanceof Element ? node : parentElementGetter.call(node);
while (!isNull(currentElement) && currentElement !== host) {
const elmOwnerKey = getNodeNearestOwnerKey(currentElement);
const parent: Element = parentElementGetter.call(currentElement);
const parent = parentElementGetter.call(currentElement);
if (elmOwnerKey === hostKey) {
// we have reached a host's node element, and only if
// that element is an slot, then the node is considered slotted
return isSlotElement(currentElement);
} else if (parent !== host && getNodeNearestOwnerKey(parent) !== elmOwnerKey) {
} else if (!isNull(parent) && parent !== host && getNodeNearestOwnerKey(parent) !== elmOwnerKey) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

guarding against checking for elements being slotted for manually inserted nodes. this could cause issues, but I don't have a concrete example for a test for this guarding, but the type system was definitely complaining about it.

// we are crossing a boundary of some sort since the elm and its parent
// have different owner key. for slotted elements, this is only possible
// if the parent happens to be a slot that is not owned by the host
Expand Down Expand Up @@ -268,7 +270,7 @@ export function PatchedElement(elm: HTMLElement): HTMLElementConstructor {
return createStaticNodeList(lightDomQuerySelectorAll(this, selectors));
}
get innerHTML(this: Element): string {
const { childNodes } = this;
const childNodes = getInternalChildNodes(this);
let innerHTML = '';
for (let i = 0, len = childNodes.length; i < len; i += 1) {
innerHTML += getOuterHTML(childNodes[i]);
Expand Down