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] textContent should not be set for <template> element. #7297

Merged
merged 4 commits into from Apr 16, 2022

Conversation

lidlanca
Copy link
Contributor

fix inconsistent rendering of template tags, where text only content is not inserted in the fragment (.content)
elements directly under a template element is non standard, and may break some functionality
for example $(template).innerHTML will be empty when elements are not part of the fragment.

current behavior

A)<template>123</template>

B)<template>1+2={1+2}</template>

C)<template><b>inside</b></template>

image

for A, B content is not part of the template fragment(.content)

@lidlanca lidlanca requested a review from bluwy February 21, 2022 19:24
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation and refactor! This makes sense to me. It would be great to have a test for this too to prevent regressions.

@lidlanca
Copy link
Contributor Author

lidlanca commented Feb 25, 2022

Thanks for the explanation and refactor! This makes sense to me. It would be great to have a test for this too to prevent regressions.

I assume we are looking for a snapshot testing of the compiler output

where should this test be

@bluwy
Copy link
Member

bluwy commented Feb 25, 2022

There's https://github.com/sveltejs/svelte/blob/dispatch-cancelable/test/runtime/samples/template/_config.js currently, but that checks things imperatively. I think something similar could be done instead of a snapshot by checking for textContent or innerHtml for this specific case?

@lidlanca
Copy link
Contributor Author

lidlanca commented Feb 25, 2022

I tried modifying this test, to make some assertions on the dom
however, when the test is invoked

test({ assert, target }) {
...
}}

target for <template>123</template> case.
does not have content populated,

my guess svelte component has c() and m(), and .content is set in m()
I don't know when m() is suppose to be called, or if at all possible to trigger ,
this type of test seem to be very limiting, or I lack the understanding.

@bluwy
Copy link
Member

bluwy commented Feb 26, 2022

m() should have been called before running the test. Is the fix applied when running the tests? If 123 is inside the document fragment, .content.textContent should be 123.

@lidlanca
Copy link
Contributor Author

tests should be passing now.

bluwy
bluwy approved these changes Feb 27, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for adding the tests.

@tanhauhau tanhauhau merged commit fb341cc into sveltejs:master Apr 16, 2022
19 checks passed
baseballyama pushed a commit to baseballyama/svelte that referenced this pull request Apr 16, 2022
…#7297)

* [fix] textContent should not be set for <template> element.

* tidy - name convetion. minor refactor extract "is template" check to a variable and replace usages.

* test template with text content

* update html in test
@Conduitry
Copy link
Member

This has been released in 3.48.0 - thanks!

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.

None yet

4 participants