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

use document.importNode for when img/iframe use loading="lazy" #349

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Sep 6, 2024

This attempts to reuse the same logic as hasCustomElement as an isImportNode. That currently only becomes true when an image/iframe has the attribute loading="lazy"

To solve solidjs/solid#1828 avoiding using document.importNode for every other element.

This in theory should have worked on the first try. For some reason nested elements with such an attribute won't be marked as isImportNode. Handled this oddity with a least resort regexp for now.

_$template('<img src=""loading=lazy>', true, false),
_$template('<div><img src=""loading=lazy>', true, false);

Without the regexp the last one won't have true, false)

@titoBouzout titoBouzout changed the title use importNode when img is loading lazy as <img loading="lazy"/> use document.importNode for when img/iframe use loading="lazy" Sep 6, 2024
@titoBouzout
Copy link
Contributor Author

ok, this is ready for review. I was able to get rid of the regexp.

--
I had the issue(now fixed) of the following nested template not having the "use importNode" bit
_$template('<div><img src=""loading=lazy>');

I presume because the condition child.id didnt enter. So I moved the copy of the flag above it, and that worked. Now I wonder if theres some latent bug of hasHydratableEvent and hasCustomElement suffering the same problem. That child templates do not copy some properties to its parents

sublime_text_4Vbyo6Px85

@ryansolid
Copy link
Owner

It's a bit odd. What I mean is every native element will have child.id I'd think. But maybe I'm wrong. Maybe only ones with dynamic bindings or something.

@ryansolid ryansolid changed the base branch from main to next September 9, 2024 21:21
@ryansolid ryansolid merged commit fd4c85f into ryansolid:next Sep 9, 2024
1 check failed
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.

2 participants