HTML API: Use create_fragment_at_node to allow more contexts in create_fragment #7777
Conversation
This reverts commit ba9e218.
null should not be returned in this case, but it is part of the signature and should be covered here.
…i/use-create-fragment-at-node-for-main-create-fragment-method
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
I spent some amount of time scrutinizing this, and it seems like you have done an exceptional job here @sirreal.
At first I didn’t like accepting arbitrary HTML as the context — there’s a hidden performance surprise in there. On the other hand, I realize that to do this properly we have to build the stack of open elements, and the way that your create_fragment_at_current_node() handles that with resetting the bookmarks while leaving the stack of open elements intact seem fitting.
Do we need to prefix the DOCTYPE declaration though? What if someone wants a parser in <!DOCTYPE quirks><p><table><td>? I don’t know if that makes sense or is warranted.
I thought about that. One thing we could do is optimize the most common case where the context is
I added the DOCTYPE because I suspect we usually want to be in no-quirks mode. If that is not added, then the I'd like to better document this behavior and explain the more advanced option. |
The opposite of that is that
This seems proper. That makes a safe default with an escape hatch for those who want to do something where they adopt their own risk. |
…i/use-create-fragment-at-node-for-main-create-fragment-method
|
This can pull in tests from #7141 and should also do this (mentioned in that description):
|
…in-create-fragment-method
…l-api/use-create-fragment-at-node-for-main-create-fragment-method
Inspired by WordPress#7141 Co-authored-by: Dennis Snell <dennis.snell@automattic.com>
Inspired by WordPress#7141 Co-authored-by: Dennis Snell <dennis.snell@automattic.com>
|
I've pulled some valuable changes like tests and documentation from #7141 into this PR. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
✅
This PR builds on (and requires) #7348.This modifies
::create_fragment( $html, $context )to use a full processor andcreate_fragment_at_nodeinstead of the other way around. This makes more sense and makes the main factory methods more clear, where the state required for fragments is set up increate_fragment_at_nodeinstead of in bothcreate_fragmentandcreate_fragment_at_current_node.This allows for more HTML contexts to be provided to the basic
create_fragmentwhere the provided context HTML is appended to<!DOCTYPE html>, a full processor is created, the last tag opener is found, and a fragment parser is created at that node viacreate_fragment_at_current_node.The HTML5lib tests are updated accordingly to use this new method to create fragments.
Trac ticket: https://core.trac.wordpress.org/ticket/62357
Closes #7141.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.