HTML API: Fix CDATA section null and whitespace handling #7230
Conversation
CDATA sections should behave like text. This means applying the rules for inserting text in foreign content: - Null bytes should not change the frameset-ok flag. - Whitespace should not change the frameset-ok flag. - Other characters set frameset-ok flag to false. Fix this behavior.
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. |
| $current_token = $this->bookmarks[ $this->state->current_token->bookmark_name ]; | ||
| $cdata_content_start = $current_token->start + 9; | ||
| $cdata_content_length = $current_token->length - 12; | ||
| if ( $cdata_content_length !== strspn( $this->html, "\0 \t\n\f\r", $cdata_content_start, $cdata_content_length ) ) { |
There was a problem hiding this comment.
The yoda condition PHPCS flagged here seems to be a false positive. I suspect it's confusing a string literal on the line with a literal comparison, but the string literal is not being compared. It's fine to have it on the other side…
There was a problem hiding this comment.
it doesn't define Yoda conditions the way most of us do, I think. by flipping this conditional it's happy.
There was a problem hiding this comment.
I suspect it was finding a literal in the RHS of the comparison and thinking that literal are supposed to go on the left (yoda), but it ignores the fact that the literal is in a function call. But that's just my intuition.
Switching the order is fine if it satisfies the linter 🤷
There was a problem hiding this comment.
I believe it's the variable on the left hand side that it doesn't like, unless both are variables, in which case it's happy and doesn't care.
joda condition checks like these aren't about literals, but accidental assignment to a variable.
|
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. |
sirreal
force-pushed
the
html-api/fix-cdata-whitespace-frameset-handling
branch
from
d5a528c
to
47ccb34
Compare
There was a problem hiding this comment.
@sirreal with the merge of #7236 we should be able to add a short-circuit to skip the additional processing if the ->text_node_classification is WP_HTML_Tag_Processor::TEXT_IS_NULL_SEQUENCE || WP_HTML_Tag_Processor::TEXT_IS_WHITESPACE.
that may not make much of a difference, but it should have already been checked in those cases. maybe it's good enough to leave this in since we don't have to perform any character reference decoding.
what do you think?
|
After reviewing this and text subdivision, my preference is to go the other direction. I've pushed a change to stop running subdivision on CDATA and let this change handle CDATA on its own. |
| @@ -3368,70 +3368,55 @@ public function get_comment_type(): ?string { | |||
| * @return bool Whether the text node was subdivided. | |||
| */ | |||
| public function subdivide_text_appropriately(): bool { | |||
There was a problem hiding this comment.
When viewing with whitespace changes, the diff here is confusing. I've adjusted this method so that it only operates on text nodes.
There was a problem hiding this comment.
This seems fitting. It seemed fitting to join them together since text nodes and CDATA sections both represent text content. But computationally, CDATA sections are easier and don't need the abstraction.
I think I prefer classifying CDATA sections, but it also seems rather unlikely to find fully NULL byte or fully whitespace content inside of a CDATA section, so in practice it may not matter.
…orbid FRAMESET. When CDATA sections (which can only occur inside SVG and MathML content) consist only of NULL bytes or whitespace characters they should not clear the "frameset ok" flag. Previously they have always been clearing this flag, but in this patch the logic is updated to detect these sequences properly. Developed in #7230 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58867]. Props dmsnell, jonsurrell. See #61576. git-svn-id: https://develop.svn.wordpress.org/trunk@58977 602fd350-edb4-49c9-b593-d223f7449a82
…orbid FRAMESET. When CDATA sections (which can only occur inside SVG and MathML content) consist only of NULL bytes or whitespace characters they should not clear the "frameset ok" flag. Previously they have always been clearing this flag, but in this patch the logic is updated to detect these sequences properly. Developed in WordPress/wordpress-develop#7230 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58867]. Props dmsnell, jonsurrell. See #61576. Built from https://develop.svn.wordpress.org/trunk@58977 git-svn-id: http://core.svn.wordpress.org/trunk@58373 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…orbid FRAMESET. When CDATA sections (which can only occur inside SVG and MathML content) consist only of NULL bytes or whitespace characters they should not clear the "frameset ok" flag. Previously they have always been clearing this flag, but in this patch the logic is updated to detect these sequences properly. Developed in WordPress/wordpress-develop#7230 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58867]. Props dmsnell, jonsurrell. See #61576. Built from https://develop.svn.wordpress.org/trunk@58977 git-svn-id: https://core.svn.wordpress.org/trunk@58373 1a063a9b-81f0-0310-95a4-ce76da25c4cd
CDATA sections should behave like text. This means applying the rules
for inserting text in foreign content:
Fix this behavior.
See the problematic behavior here.
The HTML input looks like this (null byte has been replaced with visual character):
In this case, frameset-ok should not be changed. The HTML API should fail with
WP_HTML_Unsupported_Exception: Cannot process non-ignored FRAMESET tags.The current behavior (fixed in this PR) is to add the CDATA token without any inspection. This is incorrect because, depending on the contents of the CDATA section, the frameset-ok flag should be set to false.
Trac ticket: https://core.trac.wordpress.org/ticket/61576
Follow up to: [58868]
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.