ember-animation / liquid-fire

Animations & transitions for ambitious Ember applications.
Other
1.15k stars 199 forks source link

Error while quickly switch between views with a transition #409

Open lebbers opened 8 years ago

lebbers commented 8 years ago

I have two tabs that open a specific view with a simple fading transition in-between switching tabs.

this.transition(
    this.hasClass('animatedTabs'),
    this.use('fade', {duration: 5000})
);

One of the views has the following template:

<form>
    <label for="uniqueIdForInput">label</label>
    {{input type="text" id="uniqueIdForInput"}}
</form>

If I open the view above, switch to the other tab and directly switch back the view above (while the transition is not finished yet) the following error occurs. Uncaught Error: Assertion Failed: Attempted to register a view with an id already in use: uniqueIdForInput

Without transitions the error doesn't appear but if I quickly (while the transition is still running) switch between the views the error is displayed and is the transition broken.

As a workaround i've used a normal input element instead of the input helper, this will fix the issue.

frederikbosch commented 8 years ago

You might consider removing the id and place the input helper inside the label. This is valid HTML, you can still click the label and you get rid of the double id problem.

frederikbosch commented 8 years ago

I do not consider this a liquid fire bug. The new template contains the same id as the old template, which should be fixed in your own code.

runspired commented 8 years ago

@frederikbosch Your explanation is a little too casual, this is arguably a bug with Ember which is quickly surfaced when using liquid-fire and may be fixable by changing the view registry to store based on Symbol instead of element ID. The recommended solution will not work for many layouts, and IDs are used for more than just input-to-label mapping, any use of a human-readable ID in ember will lead to this happening with liquid-fire when transitioning within-model or between routes that have the same human-readable ID in use.

frederikbosch commented 8 years ago

@runspired Maybe it is. I will reopen it. But I have to add the following. When build SPA's, with tools like Ember, you have to be careful with using IDs. Every ID on a page should be unique. This is a limitation/feature from HTML.

When a document fragment gets appended to the active document, as happening with liquid-fire, you must realize that appending fragments might lead to duplicate IDs. This is indeed especially likely with within-model changes. There might be a solution possible within Ember, though I think the developer is responsible for creating a valid DOM structure.

code0100fun commented 8 years ago

I'm running into this problem as well. Nesting the <input> in the <label> will not fix my problem though because I have design/CSS constraints that call for the elements to be rendered like:

<dt>
  <label for={{inputId}}>{{label}}</label>
</dt>
<dd>
  {{input id=inputId type=type value=value}}
</dd>

Our designers also use ids to scope styles, so simply not using ids isn't a solution in the long run for me.

If there was a hook I could tie into at the component level before the transition then I could adjust the ids to not conflict (remove one or both, append a tmp to the old one, etc). Any ideas?

runspired commented 8 years ago

@code0100fun if you move to liquid-fire-tweenlite and utilize the (about to be) release of liquid-fire-hooks, I think you could do this using the firstStep hook. The thing is though, if your styles are ID dependent, then changing the ID will cause the view being animated out to distort.

I think it's more wise to patch Ember's view registry to not be ID based. @rwjblue you might be the right guy to ask on this?

rlivsey commented 8 years ago

@code0100fun one way I've handled this is to generate the ID in a component with a unique ID (which can be tagless to not affect the rendering), eg:

let id = 0;
export default Ember.Component.extend({
  tagName: '',
  rowID: Ember.computed({
      get() {
         id++;
         return `field-${id}`;
      }
    })
});

/* template.hbs
  {{yield (hash id=rowID}}
*/

With your example then being:

{{#form-field as |field|}}
  <dt>
    <label for={{field.id}}>{{label}}</label>
  </dt>
  <dd>
    {{input id=field.id type=type value=value}}
  </dd>
{{/form-field}}
frederikbosch commented 8 years ago

Nice solution, @rlivsey. I would even prepend the guid of the component to the field to make sure the ID is not only unique in the component but also outside the component.

runspired commented 8 years ago

I've done things like this before, but it only works so long as the styling isn't ID dependent for scoping. If you can get your designers to scope tighter with classes (e.g. the ember-component-css approach) you could use that to ensure unique IDs. Unfortunately, this currently takes a lot of wiring for something the browser gives you for free.

code0100fun commented 8 years ago

@rlivsey that's essentially what I'm doing to work around it now:

<dt>
  <label for={{randomId}}>{{label}}</label>
</dt>
<dd>
  {{input id=randomId name=inputId type=type value=value}}
</dd>
export default Ember.Component.extend({
  ...
  randomId: Ember.computed(function() {
    return `${this.get('inputId')}-${faker.internet.password(10,false,/[a-f0-9]/)}`;
  })
})

My only problem with this is exactly what @runspired mentioned about designers using ids for styles. I can generally work around it or ask them to use classes but it's a hard sell because I have already asked them not to us descendent selectors to fix the ember view wrapping issues 😅

I like the approach of using liquid-fire-hooks willAnimate* hook but I don't want to tie client apps to the license agreement of GSAP.

Fixing this in Ember seems drastic but using a data-ember-id attribute seems like a decent solution. At minimum having a way to disable the ID uniqueness check during animations would be nice.

runspired commented 8 years ago

@code0100fun what's up with the GSAP license that concerns you? tweenlite is fair game for any project without needing to buy a license. It's not an open source license, but nor is it restrictive.

code0100fun commented 8 years ago

@runspired maybe I'm interpreting this incorectly but the license seems to require you to pay if you apps end users are required to pay a subscription or a fee to gain access.

runspired commented 8 years ago

@code0100fun correct, $50 (total)/yr, which if your end users are paying shouldn't be a burden. Basically, gsap funds it's development by asking the people who generate revenue with the app using it to contribute back.

ef4 commented 8 years ago

The pedantic answer is that reusing ids violates the HTML spec, and if you're intentionally designing animations that will put the same element on the screen twice simultaneously, you just can't rely on ids.

But browsers are quite forgiving, and it's nice if we can be forgiving too. Ember may be able to relax its strictness in the future, but that would require significant changes, probably with user-visible semantic breakage.

runspired commented 8 years ago

@ef4 I don't consider this to be "spec breaking", this is more a grey area where the spec broke down because it was built in an era in which SPAs did not exist. When making a transition with a resulting url change, it's arguable that allowing the same ID twice is in keeping with the original spec, as (theoretically) they represent different documents.

frederikbosch commented 8 years ago

@ef4 @runspired Okay, we have different opinions whether it is an Ember or HTML problem. Let's agree that is at least not caused by liquid-fire. The only thing is that it is limiting liquid-fire. In order to close this issue, I propose to add it to the documentation, under limitations (to be created).

ef4 commented 8 years ago

In order to close this issue, I propose to add it to the documentation, under limitations (to be created).

That sounds good to me.

jpoiri commented 8 years ago

If you have a component with a id on one route, and you transition to another route with a component with the same id using {{liquid-outlet}} I get

ember.debug.js:16582 Uncaught Error: Assertion Failed: Attempted to register a view with an id already in use:

When I go throught the same flow using regular {{outlet}} problem goes away

ef4 commented 8 years ago

@jpoiri: yes, that is the intended behavior. Switching to liquid-outlet implies that you want to keep the old content around long enough to animate it, which means it can't share IDs with the new content.