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

Bug - Morph text nodes. #2524

Merged
merged 3 commits into from Jan 21, 2022
Merged

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Dec 23, 2021

patchChildren uses firstChild to select the first child node which works correctly with DOM elements and text nodes but then it uses nextElementSibling to get the next node which only returns DOM elements. As a consequence, if the children list is a mix of DOM elements and text nodes (for example, a paragraph content with <br> tags), any text node that is not the first child will be ignored and left in the previous status.

Flagged by #2463

Failing test: https://github.com/alpinejs/alpine/runs/4619366189?check_suite_focus=true#step:6:1106

packages/morph/src/morph.js Outdated Show resolved Hide resolved
Comment on lines +225 to +226
currentFrom = dom(currentFrom).nodes().next()
currentTo = dom(currentTo).nodes().next()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not relevant to this bug but I believe it's a typo as nodes should be a function call

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call, should be yeah

@earthboundkid
Copy link
Contributor

Totally dumb question, but I just learned about https://developer.mozilla.org/en-US/docs/Web/API/Document/createTreeWalker. Would that be useful for the traversals or is it not relevant?

@SimoTod
Copy link
Collaborator Author

SimoTod commented Dec 24, 2021

I don't know, i never used it. it would change the original implementation quite a lot so you need to ask Caleb.

@calebporzio
Copy link
Collaborator

Great question @carlmjohnson!

When I cracked into this problem I started there, but there are two problems with TreeWalker:
A) Oddly enough, t's actually slower than manually crawling in almost every case. I just recently checked it's performance again, and still, slower.
B) That's enough to not use it, but more than that, to my knowledge, there is no way to "skip" a subtree with the TreeWalker and that is required functionality here.

Comment on lines +266 to +267
currentTo = currentTo && dom(currentTo).nodes().next()
currentFrom = currentFrom && dom(currentFrom).nodes().next()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Relevant change. When getting the next element for the next loop iteration, we want to use nodes() so we don’t skip text nodes.

@calebporzio calebporzio merged commit 977c76e into alpinejs:main Jan 21, 2022
@calebporzio
Copy link
Collaborator

Thanks @SimoTod!

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

3 participants