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

[Octane] animated-container throws "Cannot read property 'unlock' of null" #96

Closed
balinterdi opened this issue Mar 29, 2019 · 4 comments

Comments

@balinterdi
Copy link
Contributor

balinterdi commented Mar 29, 2019

After upgrading to Octane, I got an Cannot read property 'unlock' of null, thrown by this line:

https://github.com/ember-animation/ember-animated/blob/master/addon/components/animated-container.js#L166

I put a breakpoint there and have found that this.sprite is not set to the correct sprite because this._ownElement returns null:

https://github.com/ember-animation/ember-animated/blob/master/addon/components/animated-container.js#L133-L142

So this.sprite remains its initial value, null.

That, in turn, is because this._ownElement is only set if this._inserted is set to true and that happens in the didInsertElement hook which is called after we get to the line that calls this.sprite.unlock().

The code I have is as follows:

  <AnimatedContainer class="flex flex-row flex-wrap">
    {{#animated-each this.sortedPlayers key="id" use=transition as |player|}}
      {{#link-to
        "player"
        player.id
        class="no-underline text-blue hover:text-blue-darker w-350px"
      }}
        <PlayerCard @player={{player}} />
      {{/link-to}}
    {{/animated-each}}
  </AnimatedContainer>

Not sure how this is related to Octane but this has worked in the non-Octane version, in 0.5.1 of ember-animated.

@ef4
Copy link
Contributor

ef4 commented Mar 29, 2019

Hmm, our tests are green all the way out through canary as of last night, so this must have some specific preconditions to make it fail.

Here are some notes on how the animate task is supposed to work, timing wise:

animate: task(function * (duration, animationTask) {
    this._startingUp = true;
    let service = this.get('motionService');
    let sprite;
    let useMotion;

    // During the first render, we don't yet have an element and this will return undefined
    let element = this._ownElement();

    if (element){
      // This branch only happens on renders that are not the first render, 
      // when we already have an element in DOM before the animation begins.
      sprite = Sprite.sizedStartingAt(element);
      this.sprite = sprite;
      sprite.lock();
      useMotion = true;
    } else {
      // This branch happens on initial render, when there's no element in DOM
      // at the start of animation.
      useMotion = this.get('onInitialRender');
    }

    try {
      // This is supposed to cause us to wait for ember to finish rendering.
      yield afterRender();

      // So from this point forward we expect that we necessarily must have an element.
      
      yield microwait();
    } finally {
      this._startingUp = false;
    }

    yield * service.staticMeasurement(() => {
      if (!sprite) {
        // This branch covers the initial render case. Now that the initial render has 
        // happened, we can create our sprite.
        sprite = Sprite.sizedEndingAt(this._ownElement());
        this.sprite = sprite;
      } else {
        // This branch covers the non-initial render case. We already had a sprite, but need
        // to measure its final bounds.
        sprite.measureFinalBounds();
      }
    });

    // Since the static measurement has finished, from this point forward we
    // expect to always have this.sprite.

    if (useMotion) {
      yield* new (this.motion || Resize)(this.sprite, { duration })._run();
    }

    yield animationTask;

    this.sprite.unlock();
    this.sprite = null;
  }).restartable()

@balinterdi
Copy link
Contributor Author

Thank you, that was quite helpful.

I continued digging into this and have found that the "real error" (the first one that then causes another one to barf) is in Sprite#_rememberSize.

https://github.com/ember-animation/ember-animated/blob/d7ee355a09/addon/-private/sprite.js#L568

This is called from AnimatedContainer#animate (the function I first thought you analyzed above), which calls Sprite.sizedEndingAt:

https://github.com/ember-animation/ember-animated/blob/d7ee355a09/addon/components/animated-container.js#L136

The problem is that inside Sprite, this.element seems to be a text node, so this.element.style is undefined, therefore this.element.style.width crashes and burns (sorry for the pompousness, as English is not my native tongue, I feel strangely drawn to using wild expressions unashamedly):

Screenshot 2019-03-29 at 18 49 57

I wonder what I'm doing wrong/differently that ember-animated sets out to measure a text node (which I guess it shouldn't). The template snippet I use is above, in my original description.

@balinterdi
Copy link
Contributor Author

balinterdi commented Apr 3, 2019

Turns out what caused this is that there was an animated-each inside the animated-container and since animated-each adds a leading and ending text node that became the firstNode and lastNode returned by getViewBounds:

https://github.com/ember-animation/ember-animated/blob/d7ee355a09/addon/-private/ember-internals.js
(for the stack trace of how we get there, see above).

Here is the snippet:

  <AnimatedContainer class="flex flex-row flex-wrap">
    {{#animated-each this.sortedPlayers key="id" use=transition as |player|}}
      {{#link-to
        "player"
        player.id
        class="no-underline text-blue hover:text-blue-darker w-350px"
      }}
        <PlayerCard @player={{player}} />
      {{/link-to}}
    {{/animated-each}}
  </AnimatedContainer>

Consequently, #97 fixed this for me. I'm not completely sure if that's a solid fix but it seems reasonable that we don't want text nodes inside an AnimatedContainer and #97 takes care of one such case.

I guess we should also take care (as in, tell people) that they shouldn't put pure text nodes inside animated-container?

@balinterdi balinterdi changed the title AnimatedContainer broken in Octane (Preview) [Octane Preview] animated-container throws "Cannot read property 'unlock' of null" Apr 3, 2019
@balinterdi balinterdi changed the title [Octane Preview] animated-container throws "Cannot read property 'unlock' of null" [Octane] animated-container throws "Cannot read property 'unlock' of null" Apr 3, 2019
@balinterdi
Copy link
Contributor Author

This also works with the latest octane blueprint, so I'm closing.

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

2 participants