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

Template created elements do not upgrade as CE #3182

Closed
WebReflection opened this issue Oct 20, 2017 · 22 comments
Closed

Template created elements do not upgrade as CE #3182

WebReflection opened this issue Oct 20, 2017 · 22 comments

Comments

@WebReflection
Copy link

Description:
If you use a template and inject offline AFrame components, these won't ever be initialized in the live DOM world once appended.

var scene = '<a-scene><a-box color="red" position="0 2 -5" rotation="0 45 45" scale="2 2 2"></a-box></a-scene>';

var template = document.createElement('template'); // use 'div' in Chrome
template.innerHTML = scene;
document.body.appendChild((template.content || template).firstElementChild);

However, if you change template with div, as example, you'll see that once appended everything is fine.

This makes AFrame component not suitable for any library that creates them via modern template.

Please note this is a Chrome bug only, and I'm not sure if I can fix it via document-register-element polyfill because apparently you are using your own fork.

@WebReflection
Copy link
Author

OK, without using any library, I can confirm this is an issue with Chrome and native V0 API

If you try this code, you'll see only V1 on the page. Change the string template with div or any non template element, and it works for both V0 and V1.

document.registerElement(
  'a-test',
  {
    prototype: Object.create(
      HTMLElement.prototype,
      {
        attachedCallback: {value() {
          this.textContent = this.getAttribute('test');
        }}
      }
    )
  }
);
customElements.define('b-test', class extends HTMLElement {
  connectedCallback() {
    this.textContent = this.getAttribute('test');
  }
});
var template = document.createElement('template');
template.innerHTML = [
  '<a-test test="V0"></a-test>',
  '<b-test test="V1"></b-test>'
].join('');
document.body.appendChild(template.content || template);

Is AFrame planning to migrate to the V1 part of this polyfill ?

I am not sure I should try to patch a death API, please let me know what are the plans.

If you were on V1 already, every browser would work without any issue with templates.

Best Regards

@ngokevin
Copy link
Member

First, thanks for the polyfill! Supporting templates is not a priority at the moment (there's a template component that plugs in better into A-Frame's ECS framework).

If we could upgrade to v1 for free without any bugs, that'd be cool. It would be difficult/risky since A-Frame does some patching to it, and that would need to be revisited. But no plans at the moment unless there's an free/easy path.

@dmarcos
Copy link
Member

dmarcos commented Oct 20, 2017

Thanks! Not immediate plans to migrate to V1. I have not personally seen much work using templates in the A-Frame community but it might be because they don't work 😄 I have not seen the need for them in my own work either. Any other benefits of migrating to V1?

@WebReflection
Copy link
Author

I have not seen the need for them in my own work either

both hyper and lit HTML, to name just two, use templates to smart-render DOM.

People then blame the library ignoring the V0 vs V1 eternal battle when it comes to Custom Elements, hence I'm here to offer help because I want hyperHTML to work with A-Frame too, it'd be a shame if it cannot.

But there's more to that ... let me explain

Any other benefits of migrating to V1?

V1 is the only API that has been approved and adopted by all browsers.
By moving to V1 you automatically have:

  • a future proof code that at some point would not need any polyfill at all (with V0 you are stuck with my poly forever but I won't necessarily patch a death API, it's unsustainable work for me)
  • a native hook within the browser, instead of JavaScript hacks provided by my lib
  • classes to develop components instead of the ugly V0 API ... classes can be safely transpiled thanks to my babel helper
  • better primitives and performance, every attribute triggers changes in V0, only explicit attributes per class trigger changes on V1
  • compatibility with modern templates hence all new libraries that will use the same hyperHTML template-once pattern

Alternatively I'd need to force V0 polyfill even on Chrome, since it might also ditch V0 at any time soon, making AFrame lose its only native supporter at this time.

So ... what are the issue you need to patch in V0 ?

@dmarcos
Copy link
Member

dmarcos commented Oct 20, 2017

I don't question the value of templates what I meant is that in the context of VR experiences I have not seen much demand / need.

The reason we moved to a fork is WebReflection/document-register-element#55
I would be happy to drop the fork but we would have to test thoroughly. It has served us well, bug free for more than a year now. if it ain't broke, don't fix it 😄 Thanks to you for that.

I would like to eventually move to V1, not sure if it's a priority at the moment. What are the timelines for V1 implementation in all the browsers? When can we expect V0 to be dropped?

@WebReflection
Copy link
Author

WebReflection commented Oct 21, 2017

The reason we moved to a fork is WebReflection/document-register-element#55

that's a bug report without any answer ... last message is this one:
WebReflection/document-register-element#55 (comment)

are you sure you still need that fork? your version might be behind various other fixes/changes

It has served us well, bug free for more than a year now.

yeah, DRE is battle tested since about ever: Google AMP, StencilJS and others use it too, happy it works

if it ain't broke, don't fix it

by definition, Software needs updates/maintenance though ... in this case you have a little issue ...

When can we expect V0 to be dropped?

V0 has never landed anywhere, it's dropped since ever.

V1 though, is native in Safari for mac/iOS and Chrome, mobile and Desktop.
That's 70% of the Desktop + 90% of the Mobile Web ... V0 is only and maybe Chrome

If you care about even more robust code and better performance for the majority of the Web, keeping AFrame in V0 is the wrong choice.

If you could tell me what exactly you need/why and how V1 does not work already, I'd be happy to try speeding up needed fixes.

Regards

@ngokevin
Copy link
Member

ngokevin commented Oct 21, 2017

We'd need to spend time investigating and trying. We're not jumping at it since what we have works, and there's risk in having A-Frame run in some browsers with their native implementations (possibly with quirks) and some browsers on a polyfill, and then there's mass testing across every browser needed.

I hope you understand it's not that we don't want to do it, it's just that we're focusing on other things that yield much larger performance improvements (DRE is not very huge on the flame charts) with less effort and risk. You can imagine for 90fps VR, Custom Elements are not our bottleneck. Simply a matter of priority.

But in case you're interested in volunteering/contributing in testing A-Frame with CEv1 for, that'd be cool! We can let you know what the DRE monkeypatch does and that we'd need to be able to wrap/override setAttribute/getAttribute.

@dmarcos
Copy link
Member

dmarcos commented Oct 21, 2017

There are not particular fixes we need from V1 just porting the logic we have. We have our own wrappers around the V0 API as @ngokevin pointed out: https://github.com/aframevr/aframe/blob/master/src/core/a-register-element.js

We want to eventually move to V1, it’s just a matter of fitting it in the current priorities. What we have now works for the time being but will keep a close eye on the browser support and possible bugs. Thanks for the feedback and the polyfill. It has helped us greatly.

@WebReflection
Copy link
Author

in case you're interested in volunteering/contributing in testing A-Frame with CEv1

unfortunately I don't have much extra time these days, but if you need any help at any point ping me.

I'll leave this open since it's still a bug to me, feel free to close it though.

WebReflection added a commit to WebReflection/hyperHTML that referenced this issue Oct 21, 2017
There is an issue with AFrame and Custom Elements creted
using the deprecated V0 API, as described in here:
aframevr/aframe#3182

This patch looks in a best-compromise way for <a-elements>
in HTML templates (no SVG) and it uses eventually a DIV
instead of a TEMPLATE element to generate nodes.
@WebReflection
Copy link
Author

FYI and FWIW hyperHTML now supports AFrame elements (hacked around the issue in core) as you can see in this Code Pen

@dmarcos
Copy link
Member

dmarcos commented Oct 23, 2017

@WebReflection Sweet! Thanks!

@WebReflection
Copy link
Author

WebReflection commented Oct 23, 2017

at this point I also wonder if you'd be interested in this new polyfill I've just backed, fully based on document-register-element code (but it weights 2.9K instead)
https://github.com/WebReflection/ce-v0

Features:

  • it's V0 only, no code and issues related to V1
  • it uses V1 when available to mimic V0 behavior. You go native on Safari/WebKit browsers doing nothing on your code
  • it uses exact same code DRE uses for every other browser. No compatibility issues (but I need to validate this putting in same tests DRE has)
  • even if based on V1, it notifies about every attributes like V0 does
  • it's easier to maintain, it doesn't lock you in its V0+V1 logic as it is for DRE
  • it does NOT bring in built-in extends (AFAIK you don't need them) so it still promotes V1 good practices

thoughts ?

@WebReflection
Copy link
Author

P.S. live tests works on every DRE target browser including IE9

You could also define AFrame components via the Component utility (actually, this independently of the rest of the slimmed down polyfill).

Example:

var MyElement = new Component({
  // the Custom Element name
  name: 'my-element',
  // one or more static property
  static: {
    TEST: 123,
    method() {}
  },
  // alias for createdCallback
  constructor() {},
  // alias for attributeChangedCallback
  onattribute() {},
  // alias for attachedCallback
  onconnected() {},
  // alias for detachedCallback
  ondisconnected() {}
  // any other prototype definition is allowed
  // including getters and setters
});

@ngokevin
Copy link
Member

ngokevin commented Nov 3, 2017

The polyfill does sound promising, but at least for myself I have bigger priorities (I'm sure you don't have much time either). But I'll leave it open for anyone that wants to try/investigate.

@WebReflection
Copy link
Author

hyperHTML V2 is landing tomorrow and every issue here has been solved.

I still suggest you drop my old poly and use ce-v0 for your components, you'll reduce bundle size and improve performance, but ... hey, it's your call.

@dmarcos
Copy link
Member

dmarcos commented Nov 15, 2017

Thanks for the heads up. We will look into it after the 0.8.0 release in December.

@claytongulick
Copy link

Just so you have the feedback - I was bitten by this as well. Jumped in to a-frame, new to the webvr space, but very experienced with web components v1.

Created my project with my normal workflow, created custom elements wrapping a-frame scene, etc...

Nothing worked.

I love a-frame, you guys are doing an amazing job, but v1 is the future, looking forward to when I can use a-frame in the modern web components ecosystem.

Also happy to help with porting.

@dmarcos
Copy link
Member

dmarcos commented May 16, 2019

Migration time has come. #4167 moves A-Frame to Custom Elements V1. The old polyfill served as well a new one is also working as expected. Thanks for your help and patience.

@claytongulick
Copy link

That's great news!!

@rajsite
Copy link

rajsite commented Jul 23, 2019

I ran into this as well using a template element with the following:

    <a-scene background="color: #FAFAFA" embedded>
      <a-box position="-1 0.5 -3" rotation="0 45 0" color="#4CC3D9" shadow></a-box>
      <a-sphere position="0 1.25 -5" radius="1.25" color="#EF2D5E" shadow></a-sphere>
      <a-cylinder position="1 0.75 -3" radius="0.5" height="1.5" color="#FFC65D" shadow></a-cylinder>
      <a-plane position="0 0 -4" rotation="-90 0 0" width="4" height="4" color="#7BC8A4" shadow></a-plane>
    </a-scene>

with appendChild on the content property to attach to the DOM, ie. someParent.appendChild(myTemplate.content). The elements are instantiating in browsers other than Chrome like the issue describes.

Is there a known workaround while waiting for it #4167? @WebReflection you mentioned having a workaround in hyperHTML, can you point to what you had to do?

rajsite added a commit to rajsite/webvi-experiments that referenced this issue Jul 23, 2019
Ran into a known issue in Chrome where using template elements in Chrome causes a-frame elements to fail to initialize. Might need a-frame specific workaround or wait for a-frame v1 transition. See issue aframevr/aframe#3182
@WebReflection
Copy link
Author

can anyone please try this version of the library and let me know if templated stuff still causes troubles?

https://raw.githubusercontent.com/WebReflection/document-register-element/no-template/build/document-register-element.max.js

@WebReflection
Copy link
Author

v1.14.0 should not interfere with templates already live, while everything else would need an issue, with a reproducible bug via code example in the right repository, thanks.

rajsite added a commit to rajsite/webvi-experiments that referenced this issue Jul 26, 2019
Found an issue testing the aframe custom elements with the DOM API due to: aframevr/aframe#3182

Added a toggle to createDocumentFragment to enable the content inert as the default (using template) and active (using createContextualFragment). Seemed like it may be useful to have in-case other custom element libraries also behave poorly with inert template document fragments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants