[fix] Removal of empty stylesheets created from transitions #7260

MathiasWP · 2022-02-13T12:37:31Z

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
Prefix your PR title with [feat], [fix], [chore], or [docs].
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with npm test and lint the project with npm run lint

Closes

What has been changed?

The style_manager "cleaned up" the stylesheets it made by only clearing their rules. After clearing them, it removed them from its internal map by running managed_styles.clear(). This cleanup was not optimal - it left the style elements empty in the dom.

The proposed change is to remove style-elements created by style_manager, instead of making them empty and letting them stay in the DOM without any references. I also refactored some of the methods from the runtime/internal/dom.ts file that were only used by the style_manager or in the dom.ts file itself.

If you want to reverse-engineer this bug yourself, then start with the append_empty_stylesheet method in the dom.ts file. As long as append_empty_stylesheet is used, there will be bugs related to empty stylesheets. It does the following:

Creates an empty style-element
Appends it to the dom
Returns only the sheet of the new style-element, leaving the style-tag totally unreferenced by any code

…f making them empty

MathiasWP · 2022-02-15T08:19:29Z

I saw that the actions failed during npm install, and tbh I am not sure what triggers the error. It seemed like a declaration mismatch, so i removed the return-type i set on the function. Seems like a silly fix if it works. Is it because of a difference between TS versions?

MathiasWP · 2022-02-22T20:50:57Z

Any updates on this PR? 👍

AshfordN · 2022-03-14T23:48:42Z

Given that this PR is attempting to patch a memory leak in a common feature (transitions), I'm hoping it would be addressed soon.

MathiasWP · 2022-04-12T16:49:10Z

@Conduitry it would be much appreciated getting some thoughts about this PR from some of the core Svelte-maintainers. It's my first contribution, so I am not sure how to follow this up 🤔

johndunderhill · 2022-04-14T13:25:49Z

It's important to get this fixed. We shouldn't be polluting the DOM like this. This is blocking our use of Svelte transitions. We will not use Svelte transitions until this is fixed.

TrungRueta · 2022-05-06T12:43:32Z

Hello, i think this PR is ok, can we merge it for next version release? for now leak style tag hurt system a lot

src/runtime/internal/dom.ts

test/runtime/samples/style_manager-cleanup/_config.js

Co-authored-by: Yuichiro Yamashita <xydybaseball@gmail.com>

MathiasWP · 2022-05-08T13:42:18Z

Thank you for the great help, @baseballyama, I appreciate it! Committed your suggestions 👍

lukas-nyberg · 2022-06-08T06:58:00Z

Any updates on this? Just ran into it :)

baseballyama

I added one comment , but this is basically LGTM.
Thank you!

test/runtime/samples/style_manager-cleanup/_config.js

baseballyama

LGTM💪

test/runtime/samples/style_manager-cleanup/_config.js

MathiasWP · 2022-06-12T14:12:04Z

@baseballyama I've noticed something that we need to address before merging: I cannot get the test to fail anymore. Is there anything wrong with how i used the raf.tick methods? One of your commits was changing it, but i didn't think that would affect the end result.

baseballyama · 2022-06-12T14:28:32Z

I cannot get the test to fail anymore. Is there anything wrong with how i used the raf.tick methods? One of your commits was changing it, but i didn't think that would affect the end result.

Could you please tell me more details?
Now I just quickly run the test for both master and your branch.
And master is failed and your branch is succeeded.

After that I updated tick like below and test was failed on your branch.

raf.tick(0);
raf.tick(0);

MathiasWP · 2022-06-12T16:12:07Z

I cannot get the test to fail anymore. Is there anything wrong with how i used the raf.tick methods? One of your commits was changing it, but i didn't think that would affect the end result.

Could you please tell me more details? Now I just quickly run the test for both master and your branch. And master is failed and your branch is succeeded.

After that I updated tick like below and test was failed on your branch.
raf.tick(0);
raf.tick(0);

To be honest I'm not sure why it doesn't fail. If it seems to work elsewhere then I guess it's okay. I just wanted to be sure that nothing was wrong.

baseballyama · 2022-07-05T10:41:32Z

@MathiasWP

Maybe you rebased master branch, that's why this PR is closed automatically.
I open the PR now.

tanhauhau · 2022-07-05T15:15:00Z

@MathiasWP is this branch still around? somehow couldn't clone this to test it out: https://github.com/MathiasWP/svelte/tree/style_manager-stylesheet-cleanup

MathiasWP · 2022-07-05T16:07:13Z

@MathiasWP is this branch still around? somehow couldn't clone this to test it out: https://github.com/MathiasWP/svelte/tree/style_manager-stylesheet-cleanup

Oh, i don't think so actually! Sorry! I deleted this fork and created a new one - totally forgot that this PR was up. What can i do to solve this best for you? @tanhauhau

tanhauhau · 2022-07-05T17:14:57Z

probably that's why the PR was closed previously.

What can i do to solve this best for you?

Maybe can see if u could recreate the branch from this commit 662faa3 ?

MathiasWP · 2022-07-05T21:21:00Z

probably that's why the PR was closed previously.

What can i do to solve this best for you?

Maybe can see if u could recreate the branch from this commit 662faa3 ?

I'll do this sometime this week! Any release deadline i should know about?

MathiasWP · 2022-07-06T13:11:50Z

src/runtime/internal/style_manager.ts

-			let i = stylesheet.cssRules.length;
-			while (i--) stylesheet.deleteRule(i);
+			const { style_element } = info;
+			detach(style_element);
 			info.rules = {};


This can also be removed

MathiasWP · 2022-07-06T13:13:01Z

@tanhauhau i recreated the fix on my fork.

I also removed some more code, e.g. the line i commented above. But it does not seem like the change is added in this PR. But you can fork my repo and check out the source code 👍

tanhauhau · 2022-07-06T15:51:11Z

sorry im just trying to see if close and reopen fixed that 🙈

tanhauhau · 2022-07-06T16:00:41Z

recreated the pull request over at #7662

MathiasWP · 2022-07-06T16:58:50Z

recreated the pull request over at #7662

I'm sorry for this inconvenience! It was silly of me to remove the fork, i totally forgot that it would affect this PR...

MathiasWP added 3 commits February 13, 2022 13:11

Updated style_manager cleanup to remove created stylesheets instead o…

37dfcad

…f making them empty

Linted files

5def8c7

Fixed test

b00cadf

MathiasWP changed the title ~~Updated style_manager cleanup to remove created stylesheets instead o…~~ [fix] Updated style_manager cleanup to remove created stylesheets instead o… Feb 13, 2022

MathiasWP changed the title ~~[fix] Updated style_manager cleanup to remove created stylesheets instead o…~~ [fix] Updated style_manager cleanup to remove created stylesheets instead of leaving them in the DOM Feb 13, 2022

MathiasWP changed the title ~~[fix] Updated style_manager cleanup to remove created stylesheets instead of leaving them in the DOM~~ [fix] Updated style_manager cleanup to remove created stylesheets instead of only making them empty Feb 13, 2022

Removed return type declaration

8f7b155

MathiasWP changed the title ~~[fix] Updated style_manager cleanup to remove created stylesheets instead of only making them empty~~ [fix] Correct cleanup for empty stylesheets that were appended to head after transitions Feb 22, 2022

MathiasWP changed the title ~~[fix] Correct cleanup for empty stylesheets that were appended to head after transitions~~ [fix] Correct cleanup for empty stylesheets appended to head from transitions Feb 26, 2022

MathiasWP mentioned this pull request Mar 9, 2022

Support optional transitions #7352

Open

daylilyfield mentioned this pull request Apr 3, 2022

<Svrollbar /> adds multiple <style> tags inside <head> every time you scroll daylilyfield/svrollbar#6

Closed

MathiasWP changed the title ~~[fix] Correct cleanup for empty stylesheets appended to head from transitions~~ [fix] Removal of empty stylesheets created from transitions Apr 8, 2022

baseballyama reviewed May 6, 2022

View reviewed changes

src/runtime/internal/dom.ts Outdated Show resolved Hide resolved

test/runtime/samples/style_manager-cleanup/_config.js Outdated Show resolved Hide resolved

MathiasWP and others added 2 commits May 8, 2022 15:40

Update src/runtime/internal/dom.ts

02bc49b

Co-authored-by: Yuichiro Yamashita <xydybaseball@gmail.com>

Update test/runtime/samples/style_manager-cleanup/_config.js

662faa3

Co-authored-by: Yuichiro Yamashita <xydybaseball@gmail.com>

MathiasWP requested a review from baseballyama June 10, 2022 07:25

baseballyama reviewed Jun 12, 2022

View reviewed changes

test/runtime/samples/style_manager-cleanup/_config.js Show resolved Hide resolved

baseballyama approved these changes Jun 12, 2022

View reviewed changes

test/runtime/samples/style_manager-cleanup/_config.js Show resolved Hide resolved

baseballyama reopened this Jul 5, 2022

baseballyama mentioned this pull request Jul 5, 2022

Transition leaves empty style element #7164

Closed

baseballyama requested a review from tanhauhau July 5, 2022 13:15

MathiasWP commented Jul 6, 2022

View reviewed changes

tanhauhau closed this Jul 6, 2022

tanhauhau reopened this Jul 6, 2022

tanhauhau closed this Jul 6, 2022

tanhauhau mentioned this pull request Jul 9, 2022

[fix] Removal of empty stylesheets created from transitions #7662

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Removal of empty stylesheets created from transitions #7260

[fix] Removal of empty stylesheets created from transitions #7260

MathiasWP commented Feb 13, 2022 •

edited

MathiasWP commented Feb 15, 2022

MathiasWP commented Feb 22, 2022

AshfordN commented Mar 14, 2022

MathiasWP commented Apr 12, 2022 •

edited

johndunderhill commented Apr 14, 2022

TrungRueta commented May 6, 2022

MathiasWP commented May 8, 2022

lukas-nyberg commented Jun 8, 2022

baseballyama left a comment

baseballyama left a comment

MathiasWP commented Jun 12, 2022

baseballyama commented Jun 12, 2022

MathiasWP commented Jun 12, 2022

baseballyama commented Jul 5, 2022

tanhauhau commented Jul 5, 2022 •

edited

MathiasWP commented Jul 5, 2022

tanhauhau commented Jul 5, 2022

MathiasWP commented Jul 5, 2022

MathiasWP Jul 6, 2022

MathiasWP commented Jul 6, 2022

tanhauhau commented Jul 6, 2022

tanhauhau commented Jul 6, 2022

MathiasWP commented Jul 6, 2022

[fix] Removal of empty stylesheets created from transitions #7260

[fix] Removal of empty stylesheets created from transitions #7260

Conversation

MathiasWP commented Feb 13, 2022 • edited

Before submitting the PR, please make sure you do the following

Tests

Closes

What has been changed?

MathiasWP commented Feb 15, 2022

MathiasWP commented Feb 22, 2022

AshfordN commented Mar 14, 2022

MathiasWP commented Apr 12, 2022 • edited

johndunderhill commented Apr 14, 2022

TrungRueta commented May 6, 2022

MathiasWP commented May 8, 2022

lukas-nyberg commented Jun 8, 2022

baseballyama left a comment

Choose a reason for hiding this comment

baseballyama left a comment

Choose a reason for hiding this comment

MathiasWP commented Jun 12, 2022

baseballyama commented Jun 12, 2022

MathiasWP commented Jun 12, 2022

baseballyama commented Jul 5, 2022

tanhauhau commented Jul 5, 2022 • edited

MathiasWP commented Jul 5, 2022

tanhauhau commented Jul 5, 2022

MathiasWP commented Jul 5, 2022

MathiasWP Jul 6, 2022

Choose a reason for hiding this comment

MathiasWP commented Jul 6, 2022

tanhauhau commented Jul 6, 2022

tanhauhau commented Jul 6, 2022

MathiasWP commented Jul 6, 2022

MathiasWP commented Feb 13, 2022 •

edited

MathiasWP commented Apr 12, 2022 •

edited

tanhauhau commented Jul 5, 2022 •

edited