Shadow DOM v1 #7059

oliverfernandez · 2017-01-17T18:04:30Z

Support for Shadow DOM v1 in AMP Shadow

To cover cases like Safari 10, where Shadow DOM is implemented but Custom Elements is not, native Shadow DOM is disabled because Custom Elements polyfill does not cover custom elements inside shadow roots

Related to #6893

googlebot · 2017-01-17T18:04:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

oliverfernandez · 2017-01-18T08:36:31Z

I signed it!

Company name: Marfeel Solutions SL

googlebot · 2017-01-18T08:36:32Z

CLAs look good, thanks!

dvoytenko · 2017-01-19T01:17:41Z

src/web-components.js

+}
+
+function areNativeCustomElementsSupported() {
+  return isNative(win.document.registerElement) || isNative(win.customElements && win.customElements.define);


iOS never had registerElement, so we can skip isNative(registerElement).

But if the polyfill is loaded before this class, document.registerElement will exist, right? That why we need to check that it exists and is browser native.

You are right that checking for Custom Elements v1 does not make sense.

dvoytenko · 2017-01-19T01:19:08Z

src/web-components.js

+ */
+let shadowDomMethod;
+
+let win = window;


Unfortunately we can't have window-dependent side effects in our source code. Further getShadowDomMethod().call(...) might have issues if resolved via win.Element because we also allow most of APIs to be called from friendly iframes.

What I'm thinking instead, maybe we could try to have a string var: shadowDomSupport. It could have values none, v0 and v1. This way we can set this value from tests and try different code paths.

In that case, isShadowDomSupported() {return shadowDomSupport != 'none'}. And we could switch getShadowDomMethod() to something like this:

createShadowRoot(host) { switch(getShadowDomSupport()) { case 'v0': return host.createShadowRoot(); case 'v1': return host.attachShadow({...}); } assert(...); }

dvoytenko · 2017-01-19T01:19:45Z

src/web-components.js

+ */
+export function isShadowDomSupported() {
+  if (shadowDomSupported === undefined) {
+    shadowDomSupported = !!getShadowDomMethod() && areNativeCustomElementsSupported();


Please add TODO:

TODO: Remove native CE check once WebReflection/document-register-element#96 is fixed.

dvoytenko · 2017-01-19T01:22:14Z

test/functional/test-web-components.js

+      });
+    });
+  });
+});


nit: trailing new line.

dvoytenko · 2017-01-19T01:28:56Z

src/web-components.js

+ */
+export function getShadowDomMethod() {
+  if (shadowDomMethod === undefined) {
+    shadowDomMethod = win.Element.prototype.createShadowRoot || win.Element.prototype.attachShadow;


I'd say let's pull the trigger and make attachShadow (v1) the preferred way if both available.

dvoytenko · 2017-01-19T01:29:27Z

@oliverfernandez Thanks a lot! This looks great! Just a couple of comments.

Gives preference to Shadow DOM v1 over v0

oliverfernandez · 2017-01-19T19:11:42Z

@dvoytenko I did all the changes you suggested.

However, I think that we need to keep checking for native support of document.registerElement, to check that it has not been polyfilled

dvoytenko · 2017-01-20T02:03:35Z

src/web-components.js

+};
+
+/**
+ * @type {string|undefined}


{ShadowDomVersion|undefined}

dvoytenko · 2017-01-20T02:03:44Z

src/web-components.js

+let shadowDomSupportedVersion;
+
+/**
+ * @param {string|undefined} val


{ShadowDomVersion|undefined}

dvoytenko · 2017-01-20T02:05:02Z

src/web-components.js

+
+/**
+ * Returns the supported version of Shadow DOM spec.
+ * @param {function=} optional for testing


Name is missing for @param - the compiler errors out on this in travis. Also, this is element class, right? Maybe let's call it opt_elementClass instead?

dvoytenko · 2017-01-20T02:07:00Z

src/web-components.js

+}
+
+function areNativeCustomElementsSupported() {
+  return isNative(self.document.registerElement);


No, I meant we should actually only do the other one: isNative(self.customElements && self.customElements.define). Unless I misunderstood. But basically on iOS Safari, isNative(self.document.registerElement) will always be false, right?

However, there's still a problem. For some reason the polyfill overrides the existing the native CustomElementRegistry and so, event isNative(self.customElements.define) will always be false :(. We are getting a good traction on fixing the original issue though, but I don't know ETA yet.

The only way I found to determine this correctly is by running this code:

isNative(self.Object.getOwnPropertyDescriptor(self, 'customElements').get)

Ok, got it.

The only drawback then will be that old Chrome versions, which only support Custom Elements v0, won't use native Shadow DOM. Are we ok with this?

Good point. We could certainly also check for registerElement. Could you please take a look if polyfill overrides it as well? If it's too much trouble, we can move with this version, though.

Yes, will do.

By the way, does self.Object.getOwnPropertyDescriptor(self, 'customElements').get works for you in Chrome? I always get undefined

It's undefined because of the polyfill. It always overrides customElement with the direct value. The native implementation, on the opposite, provides a getter and it's defined. So, in theory, we could simply check whether getter is present on not - but that feels too rigid to me.

Either way it's too rigid, but this way could get us going long enough for the original polyfill issue to be fixed and we'll be able to remove isNative check completely :)

I've tested in several devices, and in Chrome 55 self.Object.getOwnPropertyDescriptor(self, 'customElements').get always returns undefined, no matter if the polyfill is present or not. However, in Canary or Safari Tech Preview it behaves as you described. It's like customElements implementation in Chrome 55 is not providing the getter.

In any case, since document.registerElement is not overridden by the polyfill, I'll fix this by checking both document.registerElement or self.Object.getOwnPropertyDescriptor(self, 'customElements').get.

Working on it right now

Thanks a lot!

dvoytenko · 2017-01-20T02:25:39Z

@oliverfernandez Cool. This is very close. I asked a couple more updates. A more serious one - is still an issue with native-vs-non-native detection. I think I found a way to detect that. See comments on that above. Thanks a lot!

Checks again for CE v0 so we don't disable native Shadow DOM in browsers that don't support CE v1

oliverfernandez · 2017-01-24T18:12:12Z

@dvoytenko All changes applied, but I think I'm going to need some help in order Closure Compiler to accept Element.prototype.attachShadow

dvoytenko · 2017-01-24T19:03:53Z

src/shadow-embed.js

-  if (isShadowDomSupported()) {
+  const shadowDomSupported = getShadowDomSupportedVersion();
+  if (shadowDomSupported == ShadowDomVersion.V1) {
+    return hostElement.attachShadow({mode: 'open'});


So, yeah, attachShadow looks to be absent is externs :(

Please try to add this to shadow_dom.js:

/** * @see https://www.w3.org/TR/shadow-dom/#idl-def-ShadowRootInit * @typedef {{ * mode: string * }} */ var ShadowRootInit; /** * @see https://www.w3.org/TR/shadow-dom/#h-extensions-to-element-interface * @param {ShadowRootInit=} opt_init * @return {!ShadowRoot} */ Element.prototype.attachShadow = function(opt_init) {};

This should do it.

dvoytenko · 2017-01-24T19:07:01Z

@oliverfernandez Thanks! All looks good. We just need, as you said, add attachShadow to Element interface in externs. I added a comment above how to do this. Thanks a lot! As soon as travis passes I should be all good to merge.

oliverfernandez · 2017-01-24T19:47:11Z

@dvoytenko I think we got it :)

dvoytenko · 2017-01-24T19:48:29Z

Merged. Thanks a lot, @oliverfernandez! We also did it on the eve of Safari 10.1 release with native CE support :)

oliverfernandez · 2017-01-24T20:24:30Z

@dvoytenko And I think it gets even better! iOS 10.3 beta has been released today too, and I think it includes the updated Safari too. I'm checking it

* Shadow DOM v1 API support * Removed unintended code comment * PR fixes Gives preference to Shadow DOM v1 over v0 * Fixed lint errors * Fixes to closure-compiler errors Checks again for CE v0 so we don't disable native Shadow DOM in browsers that don't support CE v1 * Fix closure compiler error * Added Shadow DOM v1 spec to Closure Compiler

oliverfernandez added 2 commits January 17, 2017 18:31

Shadow DOM v1 API support

ac269b6

Removed unintended code comment

cb49520

jridgewell requested a review from dvoytenko January 18, 2017 18:23

dvoytenko suggested changes Jan 19, 2017

View reviewed changes

oliverfernandez added 2 commits January 19, 2017 19:48

PR fixes

e99863f

Gives preference to Shadow DOM v1 over v0

Fixed lint errors

6255fbf

dvoytenko suggested changes Jan 20, 2017

View reviewed changes

oliverfernandez added 2 commits January 24, 2017 18:50

Fixes to closure-compiler errors

c432e0b

Checks again for CE v0 so we don't disable native Shadow DOM in browsers that don't support CE v1

Fix closure compiler error

3b1c46d

dvoytenko reviewed Jan 24, 2017

View reviewed changes

dvoytenko approved these changes Jan 24, 2017

View reviewed changes

Added Shadow DOM v1 spec to Closure Compiler

b60ddf2

dvoytenko merged commit e0c9c88 into ampproject:master Jan 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shadow DOM v1 #7059

Shadow DOM v1 #7059

oliverfernandez commented Jan 17, 2017

googlebot commented Jan 17, 2017

oliverfernandez commented Jan 18, 2017

googlebot commented Jan 18, 2017

dvoytenko Jan 19, 2017

oliverfernandez Jan 19, 2017

dvoytenko Jan 19, 2017

dvoytenko Jan 19, 2017

dvoytenko Jan 19, 2017

dvoytenko Jan 19, 2017

dvoytenko commented Jan 19, 2017

oliverfernandez commented Jan 19, 2017

dvoytenko Jan 20, 2017

dvoytenko Jan 20, 2017

dvoytenko Jan 20, 2017

dvoytenko Jan 20, 2017

oliverfernandez Jan 20, 2017

dvoytenko Jan 20, 2017

oliverfernandez Jan 20, 2017

dvoytenko Jan 20, 2017

oliverfernandez Jan 23, 2017 •

edited

dvoytenko Jan 23, 2017

dvoytenko commented Jan 20, 2017

oliverfernandez commented Jan 24, 2017

dvoytenko Jan 24, 2017

dvoytenko commented Jan 24, 2017

oliverfernandez commented Jan 24, 2017

dvoytenko commented Jan 24, 2017

oliverfernandez commented Jan 24, 2017

Shadow DOM v1 #7059

Shadow DOM v1 #7059

Conversation

oliverfernandez commented Jan 17, 2017

googlebot commented Jan 17, 2017

oliverfernandez commented Jan 18, 2017

googlebot commented Jan 18, 2017

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

dvoytenko commented Jan 19, 2017

oliverfernandez commented Jan 19, 2017

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

oliverfernandez Jan 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jan 20, 2017

oliverfernandez commented Jan 24, 2017

Choose a reason for hiding this comment

dvoytenko commented Jan 24, 2017

oliverfernandez commented Jan 24, 2017

dvoytenko commented Jan 24, 2017

oliverfernandez commented Jan 24, 2017

oliverfernandez Jan 23, 2017 •

edited