aframevr / aframe

:a: Web framework for building virtual reality experiences.
https://aframe.io/
MIT License
16.61k stars 3.94k forks source link

Template created elements do not upgrade as CE #3182

Closed WebReflection closed 6 years ago

WebReflection commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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 https://github.com/WebReflection/document-register-element/pull/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 commented 6 years ago

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: https://github.com/WebReflection/document-register-element/pull/55#issuecomment-234586837

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

dmarcos commented 6 years ago

@WebReflection Sweet! Thanks!

WebReflection commented 6 years ago

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:

thoughts ?

WebReflection commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

claytongulick commented 6 years ago

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 commented 5 years ago

Migration time has come. https://github.com/aframevr/aframe/pull/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 commented 5 years ago

That's great news!!

rajsite commented 5 years ago

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?

WebReflection commented 5 years ago

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 commented 5 years ago

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.