Skip to content

Properly support template elements in DOM #14906

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

Closed
wants to merge 10 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jul 10, 2024

The template element in HTML 5 is special in the sense that it does not
add its contents into the DOM tree, but instead keeps them in a separate
shadow DOM document fragment. Interacting with the DOM tree cannot touch
the elements in the document fragment.

@nielsdos nielsdos force-pushed the dom-templates branch 4 times, most recently from cbff8b8 to 7ba97ad Compare July 10, 2024 17:47
@nielsdos nielsdos marked this pull request as ready for review July 10, 2024 17:47
@nielsdos nielsdos requested a review from Girgias July 10, 2024 17:47
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Seems reasonable, I've got some questions

@Girgias
Copy link
Member

Girgias commented Jul 10, 2024

Also feel free to already merge the first two commits if you want

@nielsdos nielsdos force-pushed the dom-templates branch 2 times, most recently from ff6eaa5 to 0ca7791 Compare July 10, 2024 20:09
nielsdos added 4 commits July 10, 2024 22:10
The template element in HTML 5 is special in the sense that it does not
add its contents into the DOM tree, but instead keeps them in a separate
shadow DOM document fragment. Interacting with the DOM tree cannot touch
the elements in the document fragment.
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

@nielsdos nielsdos requested a review from kocsismate as a code owner July 11, 2024 19:30
@nielsdos nielsdos marked this pull request as draft July 11, 2024 20:15
@nielsdos
Copy link
Member Author

nielsdos commented Jul 11, 2024

This still needs a bit more work apparently; and I need to check if the new code works causes problems with nsDef being filled in...

@nielsdos nielsdos force-pushed the dom-templates branch 6 times, most recently from d6c52c1 to 52f2737 Compare July 14, 2024 20:03
@nielsdos nielsdos marked this pull request as ready for review July 14, 2024 20:44
@nielsdos
Copy link
Member Author

I realized that the destruction solution was not good yet because of the following two reasons:

  • Destroying a template can be done indirectly
  • Destroying a template can be done through another extension (e.g. simplexml)

So it doesn't suffice to add the logic for destroying templates in the dtor of dom. Instead, we have to callback from ext/libxml to destroy our associated template content. To accomplish that, I introduce the ability for namespaces to signal additional information to ext/libxml. We already used the _private field from libxml to cache information for dom, but we can use a few lower bits using pointer tagging to set some flags as well. We use the flag bits to signal that a custom cleanup routine needs to be called in case the node is a special type of node, which makes sense to signify that via a namespace. This mechanism can be expanded upon later too.

@nielsdos nielsdos requested a review from Girgias July 14, 2024 20:50
@nielsdos
Copy link
Member Author

Upon merging I will keep the first two commits separately and then squash the others into a third one.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

The hook idea makes sense, I had also forgotten about SimpleXML

@@ -137,4 +142,28 @@ void php_dom_remove_templated_content(php_dom_private_data *private_data, const
}
}

zend_long php_dom_get_template_count(const php_dom_private_data *private_data)
Copy link
Member

Choose a reason for hiding this comment

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

Should this return a uint32_t instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm probably

@nielsdos nielsdos closed this in 6980eba Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants