fix(engine): to show shadow in IE11 devtool DOM explorer #904

caridy · 2018-12-21T02:44:36Z

Details

Does this PR introduce a breaking change?

No

caridy · 2018-12-21T02:46:57Z

packages/@lwc/engine/src/faux-shadow/custom-element.ts

 import { getActiveElement, handleFocusIn, handleFocus, ignoreFocusIn, ignoreFocus } from "./focus";
 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)';


this is how we detect IE11, it doesn't have symbol

caridy · 2018-12-21T02:47:22Z

packages/@lwc/engine/src/faux-shadow/custom-element.ts

@@ -96,6 +98,12 @@ 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)) {


this only runs in non-prod, and it doesn't affect snapshots in jest because that does have symbol

Did you test this in older versions of Chrome, Safari, and Firefox to see how it looks like in the devtool?

caridy · 2018-12-21T02:48:09Z

packages/@lwc/engine/src/faux-shadow/shadow-root.ts

+    }
+    // @ts-ignore $$placeholder$$ is fine, read the node above.
+    c = shadowRoot.$$placeholder$$ = createComment.call(document, '');
+    defineProperties(c, {


this is the trick, you create a comment, and set tagName and childNodes, and IE11 will get very confused, it will display it as a tag with children etc.

caridy · 2018-12-21T02:48:17Z

packages/@lwc/engine/src/shared/fields.ts

@@ -10,7 +10,7 @@ import { defineProperty, create, isUndefined } from "./language";
 * In IE11, symbols are expensive.
 * Due to the nature of the symbol polyfill. This method abstract the
 * creation of symbols, so we can fallback to string when native symbols
- * are not supported. Note that we can't use typeof since it will fail when tranpiling.
+ * are not supported. Note that we can't use typeof since it will fail when transpiling.


just a typo

caridy · 2018-12-21T02:49:33Z

packages/@lwc/engine/src/shared/static-html-collection.ts

@@ -122,11 +122,8 @@ StaticHTMLCollection.prototype = create(HTMLCollection.prototype, {
 setPrototypeOf(StaticHTMLCollection, HTMLCollection);

 export function createStaticHTMLCollection<T extends Element>(items: T[]): HTMLCollectionOf<T> {
-    const collection: HTMLCollectionOf<T> = create(StaticHTMLCollection.prototype, {
-        [Items]: {


this issue here, I don't know how is that it didn't popped before, but this will breaking the inspection of childNodes because Object.create() polyfill is not honoring the symbols, probably because of the symbol polyfill.

caridy · 2018-12-21T02:49:41Z

packages/@lwc/engine/src/shared/static-node-list.ts

@@ -94,11 +94,8 @@ StaticNodeList.prototype = create(NodeList.prototype, {
 setPrototypeOf(StaticNodeList, NodeList);

 export function createStaticNodeList<T extends Node>(items: T[]): NodeListOf<T> {
-    const nodeList: NodeListOf<T> = create(StaticNodeList.prototype, {


same as above.

pmdartus · 2018-12-21T07:02:19Z

packages/@lwc/engine/src/faux-shadow/shadow-root.ts

+        return c;
+    }
+    // @ts-ignore $$placeholder$$ is fine, read the node above.
+    c = shadowRoot.$$placeholder$$ = createComment.call(document, '');


We should add value to the comment (#shadow-root). I would expect older versions of Chrome and Safari would display the comment properly so it would be nice to have some content in it.

yeah, I can work on that... I was hoping to display the open or closed as well, but that is proven to be more tricky.

pmdartus · 2018-12-21T07:03:48Z

packages/@lwc/engine/src/faux-shadow/shadow-root.ts

+ * and a comment node that is intended to use to trick the IE11 DevTools
+ * to show the content of the shadowRoot in the DOM Explorer.
+ */
+export function getIE11FakeShadowRootPlaceholder(host: HTMLElement): Comment {


Can we name this function something else, since it would run in all the browsers that don't support Symbol.

any suggestion?

What about fakeShadowRootPlaceholder without the IE11?

Well, the problem is that no other devTool that I know of will really show that placeholder.

in that case would it make sense to do a user agent check along with the lack of symbol support check so that we're only doing this in IE11 ?

We already do some very trivial symbol check, but is not based on user agent, is feature detection. I will love to stay away of user agent detection as much as possible

okay, so is there an alternative feature detection we can do instead of symbol check since this captures other browsers besides IE11 ?

@tariq-sfdc what other browser besides IE11 in our browser matrix will produce a false negative on symbol?

I'm not sure. I only asked because Pierre's comment suggested that there was a supported browser that didn't support symbols and wouldn't show the placeholder.
Looks like in terms of supported browsers, IE11 is the only real browser that doesn't support symbols

pmdartus · 2018-12-21T07:06:22Z

packages/@lwc/engine/src/faux-shadow/custom-element.ts

@@ -96,6 +98,12 @@ 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)) {


Did you test this in older versions of Chrome, Safari, and Firefox to see how it looks like in the devtool?

caridy · 2018-12-21T15:56:46Z

@pmdartus

Did you test this in older versions of Chrome, Safari, and Firefox to see how it looks like in the devtool?

yes, I did test this in chrome, safari and firefox running compat, and it doesn't affect them because none of those devtools are really relying on the JS obj the represent the dom to show the stuff in the console, instead, they rely on the internal representation, which we don't control.

salesforce-best-lwc-internal · 2018-12-26T20:01:12Z

Benchmark results

Base commit: 7c94042 | Target commit: 38e8eb7

diervo

I don't love that we are feature detecting IE11 based on symbols, but I much rather prefer do that since we know it works, than some user agent check

caridy added 2 commits December 20, 2018 21:17

fix(engine): avoid using defineProperties with symbols

936a8bb

fix(engine): fixing devtool in IE11 to show shadowRoot

352d59e

caridy requested a review from davidturissini December 21, 2018 02:44

caridy changed the title ~~Caridy/ie devtool support~~ fix(engine): to show shadow in IE11 devtool DOM explorer Dec 21, 2018

caridy requested a review from diervo December 21, 2018 02:45

caridy commented Dec 21, 2018

View reviewed changes

pmdartus reviewed Dec 21, 2018

View reviewed changes

fix(engine): PR #904 feedback

38e8eb7

pmdartus approved these changes Jan 7, 2019

View reviewed changes

diervo reviewed Jan 7, 2019

View reviewed changes

diervo approved these changes Jan 7, 2019

View reviewed changes

caridy merged commit 6e2817f into master Jan 7, 2019

caridy deleted the caridy/ie-devtool-support branch January 7, 2019 21:53

This was referenced Jan 16, 2019

Invoking cloneNode(true) returns extra nodes on IE11 #973

Closed

Node.childNodes return the fake shadowRoot comment on IE11 #986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(engine): to show shadow in IE11 devtool DOM explorer #904

fix(engine): to show shadow in IE11 devtool DOM explorer #904

caridy commented Dec 21, 2018

caridy Dec 21, 2018

caridy Dec 21, 2018

pmdartus Dec 21, 2018

caridy Dec 21, 2018

caridy Dec 21, 2018

caridy Dec 21, 2018

caridy Dec 21, 2018

pmdartus Dec 21, 2018

caridy Dec 21, 2018

pmdartus Dec 21, 2018

caridy Dec 21, 2018

pmdartus Dec 21, 2018

caridy Dec 21, 2018

tariqrafique Jan 3, 2019

diervo Jan 4, 2019

tariqrafique Jan 5, 2019

caridy Jan 5, 2019 •

edited

Loading

tariqrafique Jan 7, 2019

pmdartus Dec 21, 2018

caridy commented Dec 21, 2018

salesforce-best-lwc-internal bot commented Dec 26, 2018

diervo left a comment

fix(engine): to show shadow in IE11 devtool DOM explorer #904

fix(engine): to show shadow in IE11 devtool DOM explorer #904

Conversation

caridy commented Dec 21, 2018

Details

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Dec 21, 2018

salesforce-best-lwc-internal bot commented Dec 26, 2018

Benchmark results

diervo left a comment

Choose a reason for hiding this comment

caridy Jan 5, 2019 •

edited

Loading