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

website breaking x-transition bug (update: 1.20.0) #170

Closed
TomS- opened this issue Feb 11, 2020 · 18 comments
Closed

website breaking x-transition bug (update: 1.20.0) #170

TomS- opened this issue Feb 11, 2020 · 18 comments

Comments

@TomS-
Copy link

TomS- commented Feb 11, 2020

This post was originally about implementing transition lists (https://vuejs.org/v2/guide/transitions.html#List-Move-Transitions), however, the more I use transition, the more I realise that they are slightly buggy and may need work.

I feel like I really good thing to implement is a way of queuing animations so it doesn't happen all at once (https://vuejs.org/v2/guide/transitions.html#Transition-Modes) see "in-out" and "out-in". Maybe you can add x-transition to a template tag for this to work.

For the bugs that I have come across:

  1. Animations will not fire when called too often (https://codepen.io/TomS-/pen/poJjpNp) this is happening to me on FireFox, you'll see if you press next often enough the text will disappear.
  2. This maybe an issue with Flickity but (https://codepen.io/TomS-/pen/dyoYmvK) the slider completely break when animation is applied see (https://codepen.io/TomS-/pen/poJjpNp)
@TomS- TomS- changed the title [Feature Request] List Move Transitions Transitions may need loooking at Feb 12, 2020
@TomS- TomS- changed the title Transitions may need loooking at Bug with x-transition causing missing elements & feature suggestion Feb 12, 2020
@TomS-
Copy link
Author

TomS- commented Feb 13, 2020

Noticed that @calebporzio has done a release addressing some of this issue. So I'm just doing an update to show my findings on v1.11.0 (https://codepen.io/TomS-/pen/eYNJZOx)

Fixed:
1. The text that disappears if you click next fast enough no longer disappears.

Still doesn't work:

  1. If you press "People" then "Fruit" before the transition finishes the sliders disappear, both receiving display: none.
  2. The sliders are still not interactive when an transition is added though they appear less broken. Resizing the window fixes it, so I think there is a sequencing issue when a transition is applied because $nextTick should introduce it's interactivity (https://flickity.metafizzy.co/api.html#resize). See (https://codepen.io/TomS-/pen/dyoYmvK) with no animation applied.
  3. The text that disappears if you click next fast enough.

EDIT:
Actually, I was able to easily make the text disappear by pressing next often enough. It just seems slightly less likely than before.

EDIT:
I'm actually unsure if @calebporzio has actually seen this issue. Looking at changes I don't think they do relate. :-(

@TomS-
Copy link
Author

TomS- commented Feb 16, 2020

Update: 1.20.0 (https://codepen.io/TomS-/pen/BaNKmaV)

Using x-show.transition seems to stop issue 3 (as per above). I believe this is down to transition time, a larger transition time increases the chances of a bug being produced. I'm unable to reproduce issue 1 as the transition happens too fast (you have to catch it between transitions). The reactivity of Flickity still doesn't work, but if you click "People" then "People" again, reactivity will work. I think there is an issue with the timings when transitions of used as this is not an issue when transitions are not used.

@TomS- TomS- changed the title Bug with x-transition causing missing elements & feature suggestion website breaking x-transition bug (update: 1.20.0) Feb 16, 2020
@calebporzio
Copy link
Collaborator

Hey @TomS- It seems like there are a couple of different issues you're referencing here.

Let's just start with interrupting slow transitions breaking things.

I've encountered that issue too. What is the best behavior in a case like that? Let the transition finish? Interrupt it? Block interactions during?

I'd love help researching how Vue and other packages handle this.

I'm pretty busy this week and next so I want to focus on this, but I'll need some help with the legwork of identifying the issue and potential solutions.

Thanks in advance!

@TomS-
Copy link
Author

TomS- commented Feb 19, 2020

@calebporzio I will have some time coming up, I will definitely look into this and see if I can provide any more information. My JavaScript abilities are not the best and I fully believe that someone like @SimoTod would do a much better job, but I'm happy to research into it and identifiy any code that could be causing the issue and a solution.

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 19, 2020

hi @TomS-
I think long running transitions need to be logically defined before we do any coding.

As Caleb suggested, there are 3 possible approaches:

  1. when a new transition interrupts the first one, we immediately terminate the first transition and we run the second one
  2. when a new transition interrupts the first one, we queue it, we complete the first transition and only at that point we run the second one
  3. when a new transition interrupts the first one, we ignore the second transition.

the first option is the one that would look less weird to me but I'm not sure what other frameworks do.

The second point (slider broken when you change tab) is because nextThick is emptied before the new slider shows. I'm not sure about the reason since I haven't had a look at the code yet.
See https://codepen.io/SimoTod/pen/mdJrEJR, open the dev console and click on the second tab.
For this specific case, you don't need to reinitialise the slider (at least in chrome) since they are separate components, if you remove the nextThick part it should simply work.

@TomS-
Copy link
Author

TomS- commented Feb 20, 2020

@SimoTod

I have done some research into VueJS and how it handles animations. But also libraries like AnimeJS, and it seems like the common way of handling this is similar to jQuery's .stop() (https://api.jquery.com/stop/) which is solution 1.

The animation would be immediately interupted and the second one will play. I think number 3 would cause things to not align up with the data that is in the component if an animation happens. I feel like it's important to make sure x-show and the data in x-data matches. I feel like 1 is the only solution to do this.

It is easily possible at the moment if a transition is applied to have display: none on an element while the data says it should be active.

R.E: nextTick, I applied it here because I assumed it would fire when the data has changed. Flickity requires you to trigger .resize() when the visiblity of the slider has changed. The only reason it works without in my demo is because I've set an absolute height so it doesn't need to do the recalculation on visibility change.

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 20, 2020

It should fire when data changes, but it seems to fire too early. Not sure if it's due to the transition or other pieces of code (debounce? Timeout? NextTick emptied before the element is refreshed). I'll try to have a look during the weekend.

@TomS-
Copy link
Author

TomS- commented Feb 20, 2020

@SimoTod Regarding NextTick, there is an example that doesn't have a transition but the next tick works as expected (or what I believe is as expected) I've adapted it so it makes more sense (https://codepen.io/TomS-/pen/dyoYmvK) so it first loads empty purely because there is nothing checking if the images have loaded or no placeholder there. But if you click on the tab you will see that resize() will cause it to calculate the height correctly. As soon as a transition is applied this functionality get's broken.

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 20, 2020

@TomS-

I put a couple of console logs in the source code and I can confirm that nextTick runs after the data context has been updated as expected but, unfortunately, before the transition completes.
I've updated the codepen to break down the execution. https://codepen.io/SimoTod/pen/mdJrEJR

When nextTick runs, the carousel is not visible. Only transition.start() (sets opacity to 0 and size to 95%) and transition.during() (adds the css rules to animate the transition) have been called and the display property is still set to none.
After that point there is a call to requestAnimationFrame which releases the execution flow and Alpine runs the nextTick stack.

Commenting out the first requestAnimationFrame (there are others for later steps) would make the transition progress to transition.show() (removes display:none) without releasing the execution flow. It would display the div (although with opacity: 0) and it would delay the nextThick after this step making the carousel work correctly. This could potentially be a solution (Patched version: https://codepen.io/SimoTod/pen/jOPMQzQ).

I'm not sure if there is a clever way to wait for all transition to finish before calling nextTick, it might add too much complexity to the library. I'll leave with @calebporzio since removing the first requestAnimationFrame could lead to unexpected side effect that I haven't considered.

The alternative solution is to update the doc to make clear that nextTick runs before transitions.
In that case, devs should factor it in their code and delay the callback using something like setTimeout.

@TomS-
Copy link
Author

TomS- commented Feb 21, 2020

@SimoTod

The patched version seems to work perfectly. I'm not quite seeing the animation on the text, only sometimes does it play. I can see the animation between the tabs and reactivity works and I can't seem to cause the text / sliders to disappear.

I don't think changing the functionality of nextTick would be right as the way I see it, simply nextTick is to perform an action after the data is set, but in this case it would be after transition.show() has fired. I think maybe another magic property is needed, but what that is called I'm unsure.

$transition('show', function() {
});

Or

$transitionStart
$transitionShow
$transitionEnd

So you could do:

this.$nextTick(() => {
   this.$transition('show', () =>flkty.resize());
});

What do you think?

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 21, 2020

@TomS- I agree with not touching $nextTick and even changing the transition like the patched version doesn't feel quite right. There are too many assumptions so a different library could still break.

$transition could be a solution but there is something that scratches the back of my head. This $transition would be only available when the component has at least one element using transitions and it would be a callback stack, I assume.

What if each stages of the transition (start/show/hide/end) fires a custom event (i.e. transition-end)?
Then we could, in a very alpine way, listen for @transition-end="updateCarousel()" and everything should work as expected.

@TomS-
Copy link
Author

TomS- commented Feb 21, 2020

@SimoTod I think that's a perfect idea. Custom Events would be a very simple solution, especially since you can define on which transition too. I guess it's just to @calebporzio to agree now. So far it's looking good. What was the solution to fixing the animations disappearing? as this seems like seperate issues. Was it down to it not cancelling the current animation being played? I do wonder if you have 2 sets of animations in a component would it cancel all animations? How would it know that there are animations that act together. I think using <template> to group animations similar to how VueJS does transition-groups could work.

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 21, 2020

I haven't done any code changes for slow transitions.
At the moment, i believe it just works because the default transition is really fast.


UPDATE
here a codepen to simulate the slow transition issue: https://codepen.io/SimoTod/pen/zYGoNLO
Just click quickly on people and fruit.

@TomS-
Copy link
Author

TomS- commented Mar 3, 2020

Thanks @SimoTod that demonstrates the issue perfectly.

@calebporzio
Copy link
Collaborator

Thanks for digging deep on this everyone.

I agree that option 1 is the only way to go.

I would really appreciate an attempt at an implementation, or a PR at least exploring solutions.

The transition system is a little wonky to wrap your head around, but I really think it's a solid system that would allow for something like this.

Thanks again

@TomS-
Copy link
Author

TomS- commented Mar 9, 2020

@calebporzio Unfortunately I'm not clued up enough with JavaScript to be any use on this. I have created another example: https://codepen.io/TomS-/pen/VwLyPyp

This is if your put 1 character in Name: and 1 character in Email: then click a sport, "Subscribe" should animate in, which it does but then disappears right away. This is different to the examples shown above, but another example where the animation api is failing.

@SimoTod
Copy link
Collaborator

SimoTod commented Jun 18, 2020

@HugoDF I've updated the codepen where we experienced the issue with slow transitions and it seems sorted now. https://codepen.io/SimoTod/pen/zYGoNLO
Also, I believe we have addressed all the other points (although the posy is quite long so it's hard to track everything).

I think we can close this ticket. If something is still missing, we can reopen a new issue and start a new discussion.

@HugoDF
Copy link
Contributor

HugoDF commented Jun 18, 2020

Cool, if there are still issues feel free to reopen

@HugoDF HugoDF closed this as completed Jun 18, 2020
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

4 participants