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

[KModal]: Investigate KModal backdrop entrance transition not being smooth anymore #848

Open
AlexVelezLl opened this issue Dec 2, 2024 · 29 comments
Assignees

Comments

@AlexVelezLl
Copy link
Member

Product

I have found this issue while testing learningequality/studio#4842 that updates Studio to the latest KDS release. And I have barely noticed it in Kolibri Too

Expected behavior

KModal backdrop entrance transition should be smooth as it was in 4.x.x. Example video of using KDS 4.5.1:

Compartir.pantalla.-.2024-12-02.04_44_44.mp4

Actual behavior

KModal backdrop now in KDS 5.0.0-rc10 shows a choppy animation.

Compartir.pantalla.-.2024-12-02.04_46_32.mp4

Where even during some frames, only some html elements are affected by the backdrop and others are not.

Image

Steps to reproduce the issue

  1. Check out this Studio PR: Updates kds breaking changes studio#4842
  2. Open a modal
  3. Check the KModal entrance animation.
@MisRob MisRob added the help wanted Open source contributors welcome label Dec 2, 2024
@MisRob MisRob changed the title [KModal]: KModal backdrop entrance transition not being smooth anymore [KModal]: Investigate KModal backdrop entrance transition not being smooth anymore Dec 2, 2024
@MisRob
Copy link
Member

MisRob commented Dec 2, 2024

@Sahil-Sinha-11
Copy link
Contributor

Hey @MisRob, I would like to work on this issue, please assign it to me.

@MisRob
Copy link
Member

MisRob commented Dec 9, 2024

Hi @Sahil-Sinha-11, sure thanks! I am not sure if this is better task than #690, usually 'investigate' tasks can be tricky. However I am happy if we can find out more about what's the cause or at least some more information, it's still very helpful :)

@Sahil-Sinha-11
Copy link
Contributor

Sahil-Sinha-11 commented Dec 9, 2024

Hey @MisRob @AlexVelezLl, I linked the KDS with Kolibri, but I’m not seeing the changes reflected yet. Would you mind helping me figure out what might be going wrong? I’d really appreciate your assistance! Thanks.

Screen.Recording.2024-12-09.at.9.00.37.PM.2.mp4

@AlexVelezLl
Copy link
Member Author

Hi @Sahil-Sinha-11! After updating the KDS codebase you need to wait a little bit for the js bundler to build your changes again, can you please make sure that webpack is rebuilding the plugins after you update kds?

@Sahil-Sinha-11
Copy link
Contributor

Hey @AlexVelezLl, yes, I did wait for a few seconds and this is what it shows... It is detecting changes but not reflecting on the local host , what should I do?
Image

@AlexVelezLl
Copy link
Member Author

Could you check your Kolibri node_modules folder and see if the kolibri-design-system package is soft linked?

@Sahil-Sinha-11
Copy link
Contributor

Could you check your Kolibri node_modules folder and see if the kolibri-design-system package is soft linked?

Image
this one?

@AlexVelezLl
Copy link
Member Author

Yes, I see it is indeed soft linked. Hmmm maybe is your browser caching the js files? It only happens with KDS changes? Can you try opening the page in incognito mode?

@Sahil-Sinha-11
Copy link
Contributor

Yes, I see it is indeed soft linked. Hmmm maybe is your browser caching the js files? It only happens with KDS changes? Can you try opening the page in incognito mode?

I tried on incognito but it is still the same . what should I do?

@AlexVelezLl
Copy link
Member Author

Can you please try stopping the devserver, clearnig the webpack cache (removing the node_modules/.cache/webpack folder), then restart the devserver, and do a hard refresh on the page?

Can you also check if this only happens with KDS changes?

@Sahil-Sinha-11
Copy link
Contributor

Thanks a lot, @AlexVelezLl After I cleared the node_modules/.cache/webpack it works now.

@Sahil-Sinha-11
Copy link
Contributor

Hey @AlexVelezLl, while investigating I changed the transition time to be 1 second and I found the transition happens only on the page not on the appbar, once the transition is complete on page then it goes to appbar, I am not sure but maybe it has something to do with koverlay, am I in the right direction?
https://github.com/user-attachments/assets/e8b8836d-587f-44d9-9e55-d8698dca7822

@AlexVelezLl
Copy link
Member Author

Yes, its a good direction. You should probably also see what are the differences between KModals in v4.0.0 (release-v0.17.x branch in Kolibri, and release-v4 branch in KDS) and v5.0.0 (develop branches on Kolibri and KDS), and play with both of them seeing the differences.

@Sahil-Sinha-11
Copy link
Contributor

Hey @AlexVelezLl, after my research I think the problem might be in

  1. The function getOverlayEl relies on window.overlayEl in the _useoverlay, which is efficient after initialization. However, if initialization (mountOverlay) fails or is delayed so when the transition begins it might not have been mounted.
  2. The frequent movement of elements between the DOM and the overlay container using KOverlay and Teleport might be causing problem.

Although I reverted changes made in kmodal in the previous version v4.0.0 in kds I didn't find any difference on screen. How can I proceed further from here?

@MisRob
Copy link
Member

MisRob commented Dec 16, 2024

If it's useful for confirming if the issue originated with the overlay logic, this is my pull request that introduced it #722

@Sahil-Sinha-11
Copy link
Contributor

Hey @MisRob , i was trying to mount the toolbar in the way below but I did not observed any changes am I doing it in the right way ?
Image

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 what are you trying to achieve or experiment with here? This script is meant to attach the overlay element to DOM in products such as Kolibri or Studio when KDS is installed, so I don't exactly understand.

@Sahil-Sinha-11
Copy link
Contributor

@Sahil-Sinha-11 what are you trying to achieve or experiment with here? This script is meant to attach the overlay element to DOM in products such as Kolibri or Studio when KDS is installed, so I don't exactly understand.

I am trying to mount the appbar inside the overlay when created here
Image

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 I don't think KDS should be mounting KToolbar automatically upon initialization. KToolbar is only a component about which consumers decide whether it should be used. That's different from the overlay element. It would help me to understand why you are trying to do this - would you describe the general purpose of the experimentation?

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 Also Sahil, I wanted to let you know that will be offline starting later today and note that we will be closed from December 23 to January 5.

@Sahil-Sinha-11
Copy link
Contributor

Sahil-Sinha-11 commented Dec 17, 2024

@Sahil-Sinha-11 Also Sahil, I wanted to let you know that will be offline starting later today and note that we will be closed from December 23 to January 5.

Thank you for letting me know, @MisRob I appreciate the heads-up. Looking forward to working on more interesting issues together when you're back. Have a wonderful holiday break!

@Sahil-Sinha-11
Copy link
Contributor

@Sahil-Sinha-11 I don't think KDS should be mounting KToolbar automatically upon initialization. KToolbar is only a component about which consumers decide whether it should be used. That's different from the overlay element. It would help me to understand why you are trying to do this - would you describe the general purpose of the experimentation?

while Investigating I found the toolbar wasn't reflecting the transition as expected so I thought it was not mounted inside the overlay so the transition delay was happening, so I tried to mount it,
Image
when I removed it the transition worked fine on page.

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 Ah alright, no the toolbar is not mounted inside the overlay, that's intended. Basically no elements are mounted there when Kolibri is initialized, we teleport modals to the overlay element later. We couldn't have the toolbar in the overlay in any case, because modals need to be on top of everything, toolbars included - so I don't think we could possible resolve this issue by doing something like this.

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 Looking at all this and particularly this comment, it seems that the timing is the main culprit, right? Here are some thought about what I would try to look at if I were investigating:

These are really just experimental ideas that could give us further insight. If that doesn't bear any fruit, I think we could leave it for now unless someone else has another ideas - I don't know what would I do myself at this point. We may need to re-consider how to improve our approach to teleporting overall. In any case, all the things you've found out so far are already very valuable :) Thanks!

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 Oh and one more thing to try out may be to see what would happen if you removed the Vue transition from inside the modal to outside of it, just temporarily to see if it has any effect

<transition...
  <KModal...>

These ideas may not necessarily be solutions but if you investigated in detail if/how/why it helped with the timing issues, we could understand even a bit better what would be possible direction to deal with this.

@Sahil-Sinha-11
Copy link
Contributor

@Sahil-Sinha-11 Looking at all this and particularly this comment, it seems that the timing is the main culprit, right? Here are some thought about what I would try to look at if I were investigating:

These are really just experimental ideas that could give us further insight. If that doesn't bear any fruit, I think we could leave it for now unless someone else has another ideas - I don't know what would I do myself at this point. We may need to re-consider how to improve our approach to teleporting overall. In any case, all the things you've found out so far are already very valuable :) Thanks!

I tried the 1st point by changing the time of transition to 1 sec, I observed that first the transition happens on the page except ktoolbar, when it finishes on the page the transition happens on the toolbar immediately.

@Sahil-Sinha-11
Copy link
Contributor

I’ll give the ideas a try you shared and see how they work. Thanks for the suggestions!

@MisRob
Copy link
Member

MisRob commented Dec 17, 2024

@Sahil-Sinha-11 Ah okay, so it seems you think it could be something specific to KToolbar. Then you might try to gradually remove some markup or styles from it, perhaps, to get closer to the cause - it's one of the common debugging techniques. Alright, thanks and see you later in January!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants