Repository navigation
Emoji: Skip ignored tags with attributes in wp_staticize_emoji(). - #13918
sanjithsarkar wants to merge 4 commits into
Conversation
`wp_staticize_emoji()` only recognized an ignored tag (code, pre, style, script, textarea) when `>` came right after the tag name. Emoji inside tags with attributes, such as the Code block's `<pre class="wp-block-code">`, were converted to `<img>` in feeds and email. Copying the `[^>]*` pattern from `convert_smilies()` ([45569] / #47489) would fix that, but it also matches longer tag names that start with an ignored name, such as `<code-snippet>`, `<preview>` or `<script-loader>`. The matching closing tag never appears, so emoji or smilies in the rest of the content stop being converted. The opening-tag pattern now requires whitespace, `/` or `>` after the tag name and ignores case. The closing-tag check now also ignores case and allows whitespace before `>`. Both changes are applied to `wp_staticize_emoji()` and `convert_smilies()`. Props prakritee08, therssoftware, westonruter. Fixes #66134.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @praxxiii. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
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. |
…_smilies()` tests.
|
@propsbot refresh |
|
@mukeshpanchal27 This PR builds on #13598 and includes its test. Could you add props for @praxxiii (prakritee08 on WordPress.org)? The description is updated with full test results, including a feed check against trunk. Ready for another look when you have time. |
|
@propsbot add @praxxiii |
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
This still seems brittle. This should not be using regex at all, but rather should be re-developed using WP_HTML_Tag_Processor. This will ensure that tags are handled that contain > in an attribute value. It will also account for markup inside of HTML comments. Neither of these are currently accounted for, as far as I can see.
…e-emoji-ignored-tags
…i()`. Replace the regular expression HTML loop with the Tag Processor, so that emoji are only staticized in text nodes. This handles `>` in attribute values and markup inside HTML comments, neither of which the previous loop accounted for. Text inside `CODE` and `PRE` elements is still skipped, now with nesting taken into account. See #66134.
|
@westonruter Thanks for the review. wp_staticize_emoji() now uses WP_HTML_Tag_Processor instead of the regex loop (8ae84b5), with tests for > in attribute values and markup inside comments. The description is updated with results and timings. One question: convert_smilies() fails the same cases, but it runs on the_content on every page view and the Tag Processor measured about 5x slower there. Should I convert it in this PR, or leave it for a follow-up ticket? |
Trac ticket: https://core.trac.wordpress.org/ticket/66134
Summary
Emoji inside a
<pre>or<code>element that has attributes were converted to<img>in feeds and email. That affects the Preformatted (<pre class="wp-block-preformatted">) and Verse (<pre class="wp-block-verse">) blocks, and Code blocks whose inner<code>has a class, for example from a syntax-highlighting plugin. A default Code block is not affected, because its inner bare<code>already starts an ignored section.wp_staticize_emoji()found tags by splitting the content on/(<.*>)/Uand only recognized an ignored tag when>came right after the tag name. Following @westonruter's review, this PR no longer patches that regular expression.wp_staticize_emoji()now walks the content withWP_HTML_Tag_Processorand replaces emoji only in text nodes. That fixes the original report and also handles>inside attribute values and markup inside HTML comments.convert_smilies()has the same loop. In this PR it only gets the smaller regular expression fix; see "Open question" below.What Changed
src/wp-includes/formatting.php,wp_staticize_emoji()preg_split()loop is replaced by aWP_HTML_Tag_Processorloop overnext_token(). Emoji are replaced in#texttokens only. Text insideCODEandPREis skipped, with a depth counter so the section ends at its own closing tag.get_raw_text()andreplace_raw_text(), becauseset_modifiable_text()escapes its input and cannot insert an<img>. It uses a bookmark span andWP_HTML_Text_Replacement, the same way as the subclasses incomment.phpandscript-loader.php.src/wp-includes/formatting.php,convert_smilies()[^>]*→(?:[\s\/][^>]*)?with theiflag for the opening tag, and a case-insensitive closing-tag check that allows whitespace.tests/phpunit/tests/formatting/emoji.php:390test_wp_staticize_emoji_ignores_tags_with_attributes: the 5 ignored tags, with and without attributestests/phpunit/tests/formatting/emoji.php:427test_wp_staticize_emoji_ignore_block_boundaries: 18 cases that countclass="wp-smiley"in the output. 9 are new in this revision.tests/phpunit/tests/formatting/convertSmilies.php:176test_ignore_smilies_block_boundaries: 6 cases forconvert_smilies()Testing
All of these are covered by the new tests and pass:
<pre class="wp-block-code">🙂</pre>🙂→ only the second emoji is converted<pre class="wp-block-code"><code>🙂</code></pre>→ nothing converted<code-snippet>🙂</code-snippet>🙂→ both converted (same for<preview>and<script-loader>)<PRE class="foo">🙂</PRE>🙂and<pre>🙂</pre >🙂→ only the second is converted<img alt="1 > 0 🙂" src="a.png">🙂→ only the second is converted; the attribute is left alone<pre title="a > b">🙂</pre>🙂→ only the second is converted<!-- 1 > 0 🙂 -->🙂→ only the second is converted<!-- > <pre> -->🙂→ converted; a<pre>in a comment does not start an ignored section<pre><!-- > </pre> -->🙂</pre>🙂→ only the last is converted<code><code>🙂</code>🙂</code>🙂→ only the last is converted1 < 2 🙂 <p>🙂</p>→ both convertedResults of the new tests on each version, run against current trunk:
vendor/bin/phpunit --group formattingwith trunk merged in → 2,295 tests pass. The 5 warnings come fromwpStripAllTagsandwpTrimExcerpttests, which this PR doesn't touch.wp_staticize_emoji()and through thethe_content_feedfilter:<pre class="wp-block-preformatted">)<img><pre class="wp-block-verse">)<img><code class="language-php"><img><pre class="wp-block-code"><code>)<img><img>The earlier revision was also checked in a real
/?feed=rss2response. That check has not been repeated for this revision.Performance
wp_staticize_emoji()runs on feeds and HTML email, and returns early when the text has no emoji, so the Tag Processor only runs on content that contains emoji. Time per call, measured in the local Docker environment (amd64 emulation, so compare the columns rather than read the absolute values):Most of the time on both sides is the existing scan of the emoji list, not the HTML loop.
Behavior Changes
TITLE,IFRAME,XMP,NOEMBEDandNOFRAMESare no longer converted. The Tag Processor does not report their contents as text nodes, the same asSCRIPT,STYLEandTEXTAREA. An<img>is not valid there anyway.<a href="x, the Tag Processor stops there and that tail is left as is. Text before it is still processed.<that does not start a tag, as in1 < 2, no longer hides the text after it from conversion.convert_smilies(): since [45569],<code-snippet>,<preview>and similar tags have stopped smiley conversion for the rest of the content. Content with these tags will now get smilies converted after them.Open question:
convert_smilies()convert_smilies()fails the same new cases as the old emoji loop (>in attribute values, markup in comments, nested ignored tags,<in text). I converted it to the Tag Processor locally and the tests pass, but I left it out of this PR for two reasons:wp_staticize_emoji().the_content,the_excerpt,comment_textand block template output on every page view, with no early return. Tokenizing a 55 KB post took about 8 ms with the Tag Processor against 1.8 ms with the regular expression split in the same environment.I can add it here or open a follow-up ticket, whichever reviewers prefer.
Anything the Reviewer Should Know
$bookmarks,$lexical_updatesand$htmlproperties. If the HTML API has or gets a public way to replace a text node with raw HTML, this should use it instead.<code>inside it is matched by the old pattern.Use of AI Tools
wp_staticize_emoji()andconvert_smilies(), reworkingwp_staticize_emoji()onWP_HTML_Tag_Processorafter review, writing the tests, running them on this branch, on trunk code and with the Emoji: Don't staticize emoji in ignored tags that have attributes. #13598 patch, the performance measurements, and drafting this description.