drag/drop not resolving promise on drag end

[apellerano-pw]

Describe the bug
Dragging a sortable-item and then releasing it does not cause _complete() to be called.

To Reproduce
Steps to reproduce the behavior:

1. Create some sortable items with an onDragStop action
2. Drag an item to a new spot and release mouse button
3. onDragStop action is never called

Expected behavior
onDragStop action is called

Additional context
I stepped thru the code and this seems to be the relevant bit:

this.element.addEventListener('transitionend', deferred.resolve);
  transitionPromise = deferred.promise.finally(() => {
  this.element.removeEventListener('transitionend', deferred.resolve);
});

(from https://github.com/heroku/ember-sortable/blob/master/addon/mixins/sortable-item.js#L665-L668)

That transitionend event never fires, so the promise never resolves.

As a workaround I was able to manually set isAnimated=false on the sortable-item and then it works again, since it uses a run.later to resolve the promise instead of the transitionend event.

[apellerano-pw]

I believe this is relevant to the conversation in this PR #223 (comment)

[fivetanley]

Maybe isAnimated should have a default of false? We don't include any styles for animations by default it seems.

[fivetanley]

It looks like we do check to make sure the element is actually animated. Do you have any custom styles for transitions that you're using? https://github.com/heroku/ember-sortable/blob/master/addon/mixins/sortable-item.js#L129

[apellerano-pw]

Doesn't look like we are specifying any transition properties on the element. Here's a snapshot from getComputedStyle(el), lmk if seeing more of it would be useful:

transition: "all 0s ease 0s"
transitionDelay: "0s"
transitionDuration: "0s"
transitionProperty: "all"
transitionTimingFunction: "ease"

isAnimated will check for transition-property: all and that appears to be the default value (https://developer.mozilla.org/en-US/docs/Web/CSS/transition-property).

[apellerano-pw]

If I change the implementation of isAnimated to do

let duration = getComputedStyle(el).transitionDuration;
return duration !== '0s';

Then everything works correctly again. Perhaps the best way to compute isAnimated is with some combo of transition-duration, transition-delay, and transition-property?

[jgwhite]

Then everything works correctly again. Perhaps the best way to compute isAnimated is with some combo of transition-duration, transition-delay, and transition-property?

This logic could certainly be more robust. A PR to that effect would be very welcome.

[cah-brian-gantzler]

Saw this addon promoted on the 31 Days of Ember and thought we would give it a try. On the very first attempt on what I thought would be a simple use, we ran into exactly this problem, but it appears is going on a year and not yet resolved.

Not sure how transition:all is being set on our element, assuming that its in a class somewhere, or we use ember-animated and it is adding it.

Would sure love this fixed as right now it blocking our ability to use this addon at all.

[ygongdev]

@cah-briangantzler thanks for bringing this back to light. Do you mind reproducing your issue via a twiddle?
AFAIK, any issues around transitionend not being fired can be resolved by supplying an animation in order to trigger the event.
#234 (comment)

A long term solution would probably involve refactoring the logic to not be solely controlled by the transitionend event.

[cah-brian-gantzler]

Given the solution and comments, I would think any default simple example would show the problem since transition-property: all seems to be the default. I do now see that the example css from the solution is in the readme, but it doesn't say the css is required to make it work.

Had really wanted to use it with Yeti Table, but I see the group is coded to a ul or ol and the items are li. Would be nice if it was a little less restrictive. So was going to possible re-write a lot of it us use the new on syntax so that the components could be tagless. Perhaps even make this as a modifier. Would love to be able to reorder a table row.

Thanks for the help.

[ygongdev]

Yeah unfortunately, there are still some gaps to be filled in terms of documentation and doing things the "right" way.

I haven't had the chance to try the addon with the new fancy glimmer syntax yet. At the moment, the only way to change the tag is to override the tagName when you're consuming it.

Perhaps, I can explore turning the addon into a tagless, but I'm unfamiliar with the backward-compatibility-ness of doing so, so it might be a future task when Ember 3.14^ is more mature and widely adopted.

[cah-brian-gantzler]

Tagless is not only glimmer, you can do tagless in all version by just saying tagName: "". This does however remove the ability to use the override methods like click. However with modifiers you can replace this behavior.

I did not see what the backward compatible target is for this add on. The modifier polyfill goes back to 2.13. Would that be an acceptable limit if I were to do a pull request?

[ygongdev]

No objections to any PRs :)
However, it seems like we currently support 2.8^, so we would need to drop those
@jgwhite What are your thoughts?

[jgwhite]

@ygongdev how about officially supporting ember > 3.0.0 and anything earlier is just a happy coincidence?

[ygongdev]

@jgwhite 👍 might also be a good excuse to bring in some of the ember 3.x.x goodies.

[cah-brian-gantzler]

Yea, looking at the polyfills.

So here is what Im thinking (Using modifiers)

<Sortable @model=model.items @onChange=(action "reorderItems") as |sortable|>
      <ul {{sortable-group sortable}}>
         {{#each sortable.items as |item|}}
             <li {{sortable-item sortable}}>
                   {{item.name}}
                   <span class="handle" {{sortable-handle sortable}}>&varr;</span>
             </li>
         {{/each}}
     </ul>
</Sortable>

You could then easily re-write the current components as tagless and put the ul in the hbs and use the modifier, then yield the item component as tagless and the li in the hbs using the modifier. Basically everything should work exactly as it is now but using modifiers under the covers.

Now that the modifiers are available you could then do the below which I would love to see working.

<Sortable @model=model.items @onChange=(action "reorderItems") as |sortable|>
      <table {{sortable-group sortable}}>
         {{#each sortable.items as |item|}}
             <tr {{sortable-item sortable}}>
                <td>
                   {{item.name}}
                </td>
                <td>
                   <span class="handle" {{sortable-handle sortable}}>&varr;</span>
                </td>
             </tr>
         {{/each}}
     </table>
</Sortable>

If the above syntax would be acceptable, I will start working on a PR and see if i can pull that off.

[ygongdev]

@cah-briangantzler the syntax looks promising. Is angle bracket invocation a requirement for modifiers (from just looking at your example). What will it look like with the good old mustache?

I think it would make sense for us to move this conversation to somewhere else (perhaps new issue).
Thanks for taking this on!

[cah-brian-gantzler]

The discussion about modifiers has been moved to a #342