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

[fix] Fixes issue #7026 - style_manager transition conflict (a platform use case) #7107

Closed
wants to merge 3 commits into from
Closed

Conversation

typhonrt
Copy link
Contributor

@typhonrt typhonrt commented Jan 6, 2022

Fixes #7026.

I gave a thorough overview of the problem in the issue, so please refer to the issue and demo repo associated with that issue.

In short ./src/runtime/internal/style_manager.ts does not take into account multiple Svelte runtimes on the same browser page. There is a single generated stylesheet where transition keyframe rules are added / removed at runtime for transitions. Currently if more than one app / runtime transitions at the same time the first runtime to finish all of its transitions clears out the style sheet which is also holding transition rules for all other Svelte runtimes.

This causes all sorts of visual glitching issues with transitions between separately compiled Svelte apps running on the same browser page.

The fix is low impact and that is to assign a unique ID key and create temporary / transition style sheets for each Svelte runtime. This is accomplished with Date.now, but I am certainly open to suggestions on a better unique ID generation mechanism.

No new tests are required and all tests pass.

Below is a gif that shows the transition interruption. The left and right apps are separately compiled Svelte apps. On this platform (FoundryVTT) pressing esc closes all apps, so both close at the same time. The left app has a one second scale transition. The right app has a two second scale transition. When the left app finishes at one second it clears out all transition styles and interrupts the right apps transition. The right app will display fully and then disappear after the end of the two second transition when it is removed from the DOM.
svelte-7026-no-fix

Apologies if I am pinging the wrong folks. I waited until after the holiday weeks to move this forward. I am including the last two committers to style_manager.ts:

Also
@benmccann from our discussion from #contributing on Discord.

This issue is a big quality problem for usage of Svelte in platform use cases. I have made a really nice Svelte integration for the Foundry VTT platform and am imminently releasing a library / tool suite for all 3rd party Foundry developers. I certainly appreciate the help in getting this PR merged and am more than willing to modify it according to any advice from the maintainers.

@typhonrt typhonrt changed the title [fix] Fixes issue #7026 - platform use case [fix] Fixes issue #7026 - style_manager transition conflict (a platform use case) Jan 6, 2022
@typhonrt
Copy link
Contributor Author

typhonrt commented Jan 7, 2022

Regarding @ivanhofer's initial suggestions. unique_id now uses Math.random() and there are added types for the variable initializers related to the ExtendedDoc interface.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. This is only used by animations and transitions, and they should be independent for independently compiled components.

@typhonrt
Copy link
Contributor Author

typhonrt commented Jan 7, 2022

Excellent.. Thanks for moving this forward @dummdidumm! Too bad this missed the 3.45.0 release, but glad it is working toward a merge.

@ivanhofer
Copy link
Contributor

I just took another look at the code and I wonder why does the information get added to the DOM node?
Can we not just use a local variable in this file to store the information? We then won't need to generate a random ID.
Or did I miss something?

@typhonrt
Copy link
Contributor Author

typhonrt commented Jan 7, 2022

I just took another look at the code and I wonder why does the information get added to the DOM node? Can we not just use a local variable in this file to store the information? We then won't need to generate a random ID. Or did I miss something?

Not having been around for the early / mid development of Svelte my assumption is that style_manager in its current form stems from the 1st generation implementation for transitions. One that supports IE and older browsers. It's quite possible that it can be rewritten to solely use the web animation API and store the animations purely as data in memory, however this means dropping IE legacy support. See can-i-use.

@ivanhofer
Copy link
Contributor

@typhonrt I'm not sure if you did understand me correctly. I have created a PR that shows my proposed solution: #7114

Rewriting it to the web animation API would be an option when Svelte drops IE support. Maybe in Svelte version 4?

@dummdidumm
Copy link
Member

I think I prefer solution #7114

@typhonrt
Copy link
Contributor Author

@ivanhofer.. I'm definitely all for just getting this issue solved and in the next Svelte release if at all possible. Your solution is clean whereas my initial solution is not as much. Though being new to the codebase I went with an implementation that adds / removes the least amount of lines of code preferably just in one file, so that gives some insight into the initial fix. Let's get your PR into Svelte 3.45.1! Thanks for taking a look at this.

@benmccann
Copy link
Member

Closing in favor of #7114

@benmccann benmccann closed this Jan 10, 2022
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

Successfully merging this pull request may close these issues.

Support multiple independent compiled apps on same browser page: a platform use case w/ PR
4 participants