search

aframevr / aframe

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

Revisiting setAttribute/attributeChangedCallback behavior #960

Closed dmarcos closed 7 years ago

dmarcos commented 8 years ago

We're hitting some issues with multiple calls to setEntityAttribute on a single call to setAttribute. There are two separate problems:

  1. Firefox uses the register-element polyfill. Our wrapped getAttribute is used to retrieve the old attribute value to check if oldValue === newValue. The check fails because we end up comparing objects instead of strings and attributeChangedCallback is always called. The native implementation in Chrome doesn't have this problem. A PR into the register-element polyfill to invoke HTMLElement.getAttribute instead of our wrapped getAttribute might be the clean solution. There's no downside for them and it will make the polyfill more robust.
  2. attributeChangedCallback behaves synchronously and when a component data changes we call setEntityAttribute twice. First in the attributeChangedCallback and later in our setAttribute wrapper. We need to find a way to avoid redundant calls.

@ngokevin thoughs?

ngokevin commented 8 years ago
  1. I was also running into that issue. When doing setAttribute, it would do getAttribute to figure out what changed. For the obj-model component, that was returning a parsed/deserialized object. Since the obj-model uses the src type, it breaks because the src type does not produce the same value when called twice in a row. So a PR should fix that.
  2. We could remove the setEntityAttribute from the setAttribute since attributeChangedCallback is called anways, but in the case we do like setAttribute('material', 'color', 'red'), we need to call setEntityAttribute with some processed data. So, yeah we need to refactor somehow.

By the way, how do I go confirm that attributeChangedCallback is in fact getting called synchronously? Using setTimeout?

dmarcos commented 8 years ago

I use the debugger. If attributeChangedCallback is called before the line below then it means is synchronous:

https://github.com/aframevr/aframe/blob/dev/src/core/a-entity.js#L479

cvan commented 8 years ago

Have y'all thought about switching to the webcomponents.js custom elements polyfill? (filed as #567.) perhaps just try it first.

ngokevin commented 7 years ago

setEntityAttribute now no longer called in setAttribute