Emoji: Fix defects in the emoji scripts, and bring them under TypeScript checking and test - #13592
Open
westonruter wants to merge 34 commits into
Open
westonruter wants to merge 34 commits into
westonruter wants to merge 34 commits into
Conversation
Include src/js/_enqueues/lib/emoji-loader.js in the tsconfig.json file list and resolve the 30 errors that surfaced, so that the file checks cleanly. Two defects came to light in the process: * supportsWorkerOffloading() did not return a boolean, contrary to its documented return type. The Worker feature detection tested the truthiness of URL.createObjectURL, which is always defined, so the expression evaluated to the method itself rather than to true. It now checks that the method is callable. * The WPEmojiSettings typedef documented source.concatemoji, source.twemoji and source.wpemoji as nested properties of a plain object, which meant none of them were ever actually typed. The supports property that this script writes and that wp-emoji.js reads was not documented at all. Both are now described by dedicated WPEmojiSettingsSource and EmojiSupports typedefs, and SupportTests is narrowed from nullable booleans to booleans, matching what browserSupportsEmoji() returns. The remaining changes are annotations rather than logic. The session storage value is null checked before being parsed, the promise wrapping the support tests is given an explicit type parameter, and the drawing context is cast so that one set of tests can continue to serve both the canvas and the OffscreenCanvas contexts. WorkerGlobalScope is declared in a new typings/worker-globals entry. It is absent from the dom lib, and the webworker lib that provides it cannot be added alongside dom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Include src/js/_enqueues/wp/emoji.js in the tsconfig.json file list and resolve the 27 errors that surfaced. The emoji settings types move out of emoji-loader.js and into a new typings/wp-emoji entry, now that a second file reads them back. Keeping them as JSDoc in emoji-loader.js did work, because TypeScript treats that file as a global script and so publishes its typedefs globally, but that arrangement would break the moment either file gained an import or an export. The same entry declares the vendored Twemoji library, whose parse options include a doNotParse callback that upstream does not have. The settings types were also incomplete: baseUrl, ext, svgUrl and svgExt are all exported from PHP and read by wp-emoji, and none of them were described. typings/worker-globals is renamed to typings/browser-globals, since the vendor-prefixed MutationObserver aliases belong beside WorkerGlobalScope as another thing the dom lib does not declare. Typing Twemoji revealed that the onerror handler passed to it never does anything. It refers to the library object in three places where it means the image element, and the library object has no parentNode, so the guard is never true. A broken emoji image is therefore never replaced by its alt text, and the data-error attribute the MutationObserver tests for is never set, leaving that branch dead as well. Correcting this changes behavior, so it is marked with @ts-expect-error and a note, and is left to be tracked on its own. Those directives fail once the references are corrected, so they cannot be forgotten. The remaining changes are annotations. Two small extractions were needed to give TypeScript something to narrow: doNotParse reads the class name into a variable before testing it, and the imgAttr override captures the value rather than reading it through the arguments object inside a closure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every declaration in src/js/_enqueues/wp/emoji.js now uses const, or let where the binding is reassigned, and each variable gets its own declaration rather than sharing a comma separated list. The file already required an ES2020 toolchain, as .jshintrc sets esversion 11 and the emoji loader beside it has shipped const and arrow functions since it became a script module. Two loop bodies change shape. In the MutationObserver callback, addedNodes, removedNodes and ii were hoisted to the top of the callback and reassigned on each pass; they are now declared where they are first assigned, inside the loop. The same goes for node in the inner loop. Each was written before being read on every iteration, so the narrower scope is not observable. The params object in parse() was declared at the top of the function and assigned further down, past an early return. It is now a const at the point of assignment, which is also where its type annotation belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every other script which wraps itself in an IIFE puts the @output tag in a docblock of its own at the top of the file, and then gives the IIFE a second docblock carrying the description and the @PARAM tags, immediately above the function. See api-request.js, api.js and autosave.js. Typedefs sit between the two, as in code-editor.js. wp-emoji was the one file combining all of it into a single block, with @output trailing after the parameters it had nothing to do with. Adding the WPEmojiParseArgs typedef made that worse, since the typedef then separated the @PARAM tags from the function they describe. Note that TypeScript does not read these tags. It types both parameters from the arguments at the bottom of the file instead, so the annotations here serve documentation and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Support for Internet Explorer 11 was dropped in 5.8, and since wp-emoji.js
started using const and let the browser can no longer so much as parse the
file: it fails on the first declaration in wpEmoji(), long before reaching the
guarded block. The Trident/7.0 user agent sniff and the branch it protected
were therefore dead in every browser which can run this script at all.
The branch existed because IE 11 implemented MutationObserver by splitting a
text node wherever it met a template interpolation symbol such as "{{". It
joined the pieces back together before testing them for emoji. With a
Trident/7.0 user agent forced, the old code merged three sibling text nodes
into one and removed the other two from the document; both before and after
this change the same parent node is handed to parse(), so nothing downstream
sees a difference.
Removing the loop also retires the two string casts its node value arithmetic
needed, which were only there because nodeValue is nullable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MutationObserver has been baseline widely available since July 2015, so wp-emoji no longer needs to look for the vendor-prefixed spellings of it, and no longer needs to guard its use behind a truthiness check. The observer is constructed directly and the block it used to sit inside is outdented. The constructor is now reached as a plain global rather than as a property of window. Going through window was only ever a requirement of the detection itself, since naming WebKitMutationObserver directly would throw where it does not exist. The two vendor-prefixed names were also the only reason the browser-globals typings declared anything besides WorkerGlobalScope, so those declarations go as well. Note that mce-view.js and comment-reply.js still carry the same fallback chain; neither is type checked yet, so neither is touched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
browserSupportsSvgAsImage() could not return false. It asked document.implementation.hasFeature() whether SVG images were supported, but the DOM standard requires hasFeature() to return true for any argument at all, so the check has reported support regardless of the browser ever since that change was adopted. The one other path through the function, taken when hasFeature is missing entirely, already returned true on the same reasoning. The PNG fallback was therefore unreachable in both directions, and the params now name the SVG settings directly. This leaves baseUrl and ext unread by any script. They stay in the exported settings, both because something else may be reading the settings object and because the emoji_url and emoji_ext filters behind them still feed wp_staticize_emoji(), which builds img tags server side for feeds and email. A note to that effect is added where the two are declared, so that they are not mistaken for dead weight later. Worth noting separately: a site filtering emoji_url to serve its own PNGs has had no effect on the front end for as long as the detection has been broken. That is not a change in behavior, only a change in how visible it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
doNotParse() searched the class attribute for the substring wp-exclude-emoji rather than testing for the class itself, so a class which merely contained those characters excluded an element too. An element classed wp-exclude-emoji-wrapper, my-wp-exclude-emoji or wp-exclude-emojis was skipped along with everything beneath it. Using classList.contains() narrows this to the class the feature is documented around, which is also the form the only consumer in the tree uses: ExternalLink in @wordpress/components. Both guards around the test are dropped as well. Twemoji only calls doNotParse for element nodes, and skips anything within an SVG before it gets that far, so neither the null check nor the string check on className could be reached. The callback is typed to take an Element accordingly, which is what lets classList be read without a cast. One consequence worth recording: the callback no longer tolerates being handed something which is not an element. Nothing hands it one today, but a throw here would escape through grabAllTextNodes() and stop the whole page being parsed rather than merely failing to exclude one element. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The return was documented as a bare Object, which said nothing about the two functions on it. It now names them, as typeof parse and typeof test, so the shape is checked against what is actually returned and the signatures cannot drift from the docblocks on parse() and test() themselves. Spelling the signatures out here instead would have duplicated documentation which already sits on each function. The returned object also uses shorthand property names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The observer callback counted backwards through two nested while loops, reading each collection by index. Both are now for...of loops, with the record destructured into the two node lists it is read for. The node checks become instanceof tests rather than comparisons against nodeType and nodeName. That is what the surrounding code meant in the first place, and because instanceof narrows where the numeric comparison did not, the three casts needed to reach data, alt and getAttribute are no longer required. IDEs which do not follow inline cast comments were reporting those three properties as unresolved. Reading a text node's containing element through parentElement rather than parentNode is likewise both narrower and closer to the intent, since only an element is ever passed on to be parsed. One behavioral fix comes with this. The guard for an image which has been replaced by its own alt text ended in a return, abandoning every remaining record in the callback rather than just the one it had recognized. That made the callback sensitive to the order the records were visited in: counting backwards it discarded the older records, and counting forwards it would instead discard the newer ones, so a legitimately added element could be left unparsed. It is now a continue, which skips only the record in question and leaves the outcome independent of the order entirely. The guard is unreachable in practice today, because the data-error attribute it looks for is only set by the onerror handler that never runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Testing the node against HTMLElement before handing it to parse() removes the last cast in the observer callback, since the narrowing now establishes what the cast was asserting. Resolving the text node's container with a conditional rather than reassigning the loop variable also retires the continue which guarded against a text node having no containing element, as such a node now simply fails the same instanceof test. This stops non-HTML elements being parsed, which corrects a case Twemoji itself tries to avoid. It skips anything inside an SVG when collecting text to replace, but that test is applied to the descendants of whatever it is given, not to the node itself, so an <svg> arriving as an added node had its own text children replaced with <img> elements, which is not valid inside SVG. The same went for MathML. Both are now left alone. Comment nodes are skipped marginally sooner than before. They reached parse() previously but were returned from immediately, having no child nodes, so nothing about the outcome changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The handler given to Twemoji for an image which fails to load referred to the Twemoji library object in three places where it meant the image element. The library object has no parentNode, so the condition never held and the handler returned having done nothing: the data-error attribute was never set, and a broken image was never replaced by the emoji character it stood for. A reader was left with a broken image icon and no text at all. Twemoji's own handler, which this one was adapted from, uses `this` throughout, which is what it is called with. The MutationObserver looks for that same data-error attribute in order to recognize a replacement it must leave alone, so that branch could never be reached either. Both halves work now, and they depend on each other: without the attribute the observer parses the replacement text straight back into the image which has just failed, and round it goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three docblocks in the emoji loader said the name of a thing over again in place of describing it. The SessionSupportTests typedef had no description at all and neither of its two properties had one, so nothing recorded that the timestamp is in milliseconds since the epoch, which is the detail a reader of that code most needs. The supportTests parameter of setSessionSupportTests() and the tests parameter of testEmojiSupports() were each described by their own name reworded. These were found by running the rules which the JSDoc configuration does not enable yet, as suggested for that effort, in this case require-property- description and informative-docs. The configuration itself is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The attribute recording that an emoji image failed to load was written with setAttribute() and read back with getAttribute(), naming data-error in full at both ends. Both now go through dataset, which is what it is for, and which names the attribute once each way without the prefix. This is the same attribute either way, so anything else setting or reading data-error directly continues to interoperate. Both directions were exercised: the marker written through dataset is visible to getAttribute(), and a marker written by setAttribute() is still recognized by the observer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The JavaScript coding standard calls for a space after function when the
function is anonymous, as in the ( function ( $ ) { form it gives for the
jQuery wrapper, while a named function keeps its name against the parenthesis.
Seven closures in wp-emoji were missing that space. This is the same spacing
the PHP standard asks of a closure, and the standard is explicit elsewhere that
such agreement is deliberate, noting that a space is preferred after the
negation operator in order to conform to the PHP standards.
The rest is the standard's spacing rules applied where they had been missed: an
extra space had crept in after a return, a property value was padded to line up
with the one above it although the standard aligns nothing in an object
declaration, a call passed its arguments without the spaces inside the
parentheses which are always required, and a closing parenthesis was up against
a template literal.
None of this is checked by anything today. The ESLint configuration covers the
inline documentation alone, and says so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five of the callbacks passed to Twemoji, and the one given to the `MutationObserver`, are arrow functions now. None of them referred to this or to arguments, so none of them needed a binding of their own, and the emoji loader alongside this file already writes every one of its callbacks this way. The two which return a single expression say so: the attributes callback returns its object directly, and the override installed for `imgAttr` returns the captured value. The handler for a failed image load stays a function expression. It is called with the image as `this`, which an arrow would take from the enclosing scope instead. That is not left to be noticed by eye: the callback is declared to take an `HTMLImageElement` as `this`, so converting it stops the build. The function which wraps the whole file is also left as it is, that being the form the coding standard gives for such a wrapper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The attributes callback was declared returning the default, and then the whole property was replaced afterwards when the caller had supplied its own. Which attributes apply is now settled in one place, before the params are composed, and the callback returns whatever that came to. This removes the local copy the replacement needed. It could not read args.imgAttr directly, because args is a parameter which is assigned a fallback just above, so the check that the property is an object is not something TypeScript will still hold to inside a callback which runs later. Deciding it once sidesteps that: the result is a constant, and nothing reads the property again. The default is now one object for the whole parse rather than a new one for each image. Twemoji only ever reads what the callback returns, copying it onto the image attribute by attribute, and the caller supplied path has always handed back the same object every time regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither of the emoji scripts had a test of any kind. The PHPUnit tests for emoji touch wp-includes/js/wp-emoji-loader.js into existence so that the PHP which prints the script tag can find it, but nothing has ever run a line of either file. These cover wp.emoji.test() and everything wp.emoji.parse() decides: that it declines to do anything when the browser needs no help, when there is nothing to parse, or when the element has no children; which URL and extension the images are given; the class name and the attributes, each with and without the caller replacing them; which characters the callback declines to replace, and which it still replaces when only flags are unsupported; which class names exclude an element from parsing and which merely resemble the one that does; and that a failed image is replaced by the character it stood for and marked so that the observer leaves it be. The test page gains the settings and a stand-in for Twemoji, both before wp-emoji itself, since it reads the one and waits for the other. The stand-in records what it is handed, which is how the tests reach the callbacks that wp-emoji composes but does not otherwise expose. The elements are built detached from the document: wp-emoji watches the body for additions, so attaching them would start it parsing partway through a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The observer had no tests. It is what notices emoji added to the page after the first parse, and it holds the other half of the recovery from an image which fails to load: the handler marks the image it is replacing, and the observer leaves that replacement alone rather than turning it straight back into the image which has just failed. Only the marking half was covered. Four tests are added. Two are for the ordinary path, an element and a text node added to the page. The other two are the guard and its control: the same replacement is made twice, once with the marker the handler sets and once without, and only the unmarked one is parsed. That the two differ is what shows the marker to be doing the work. These attach their elements, unlike the rest of the file, since the observer is watching for exactly that. Waiting for it needs a little care. Mutation records are delivered in a microtask, so the tests settle on one to put themselves behind the observer. A timer cannot be used: every test in this suite is wrapped by sinon-test, which swaps the timers for a fake clock, and a callback left on that clock is never called at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The return was described as typeof parse and typeof test, which TypeScript resolves but which editors do not, leaving them unable to say what wp.emoji is at all. Every call made through it was then reported as a call to something which might not be a function, thirteen of them in the QUnit tests alone. The two signatures are written out instead. TypeScript checks the returned object against either form, so nothing is given up by preferring the one both understand. While here, wpEmoji() is called as the function it is. It builds no instance and never refers to this, returning an object instead, so calling it with new did nothing but discard an empty instance, and the @Class tag above it described something the code has never been. Note that this was not what the editors were complaining about, and correcting it alone does not quiet them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
The tests were documented as taking a canvas 2D context, although the context they are given is an offscreen one whenever they run in a Worker. Reconciling the two took a cast through unknown, since neither context type is assignable to the other. Saying that either will do removes the need for it. The two are named once in a typedef rather than in each of the three docblocks. Obtaining the context is also moved to where the kind of canvas is still known. Asking a canvas which might be either for a 2D context gives back a union including every other kind of context, because the argument no longer picks out an overload, and that in turn was most of what the cast was covering up. What is left is a context which might be null, which the tests have always assumed away. That assumption is now written down. It fails in the same place as before, one line earlier and saying what went wrong instead of reporting that a property of null cannot be set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tests are passed to one another as arguments rather than referred to by name, so that minification cannot rename them out from under the Worker the tests are serialized into. Each was documented only as a Function, which accepts any arguments at all, so nothing about how they were called was checked. The one call to browserSupportsEmoji() went through such a parameter, which is why narrowing what that function accepts had no effect on it. Each of the three now has a callback describing what it takes and returns, and those are used in place of Function. Passing them in the wrong order is reported, as is asking for a test which does not exist, neither of which was reported before. Worth having for code which is turned into a string and run somewhere else, where a mistake of that kind surfaces as emoji quietly not being replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Add coverage for multiple mutation records handled in a single observer callback.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR modernizes and type-checks the emoji scripts, fixes parsing and fallback defects, and adds QUnit coverage.
Changes:
- Adds shared TypeScript declarations and updates
tsconfig.json. - Fixes fallback handling, mutation processing, SVG/MathML filtering, and exclusion matching.
- Adds emoji fixtures and parser/observer tests.
A moderate issue remains: add a regression test covering multiple mutation records processed in one observer callback.
File summaries
| File | Description |
|---|---|
typings/wp-emoji/index.d.ts |
Adds emoji settings and Twemoji types. |
typings/browser-globals/index.d.ts |
Declares WorkerGlobalScope. |
tsconfig.json |
Enables type checking for emoji scripts. |
tests/qunit/wp-includes/js/wp-emoji.js |
Adds emoji parser and observer tests. |
tests/qunit/index.html |
Loads emoji fixtures and implementation. |
src/js/_enqueues/wp/emoji.js |
Modernizes parsing and fixes observer/fallback behavior. |
src/js/_enqueues/lib/emoji-loader.js |
Adds stronger typing and modernizes support detection. |
Review details
Suppressed comments (1)
src/js/_enqueues/wp/emoji.js:113
- This
instanceof HTMLElementcheck is the fix for SVG/MathML added nodes, but the suite never adds either kind; thesvgUrlassertions only test URL options. A regression that passes an added SVG or MathML node to Twemoji would therefore go undetected. Add observer tests with SVG and MathML roots containing emoji and assert that Twemoji is not called.
if ( node instanceof HTMLElement && test( node.textContent ) ) {
- Files reviewed: 4/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A sweep of both emoji scripts for values TypeScript could only see as any left two in the loader, and none at all in wp-emoji. The variable holding the result of each flag comparison was declared without a type and assigned further down, so it was any throughout. It holds what emojiSetsRenderIdentically() returns, which is now described, so it is said to be a boolean. The message arriving from the Worker was cast to the support tests where it was read. It is described in the handler's signature instead. This is not merely moving the assertion: the Worker is built a few lines above from this file's own function, which posts exactly what that function returns, so the shape of the message is a contract this file controls rather than a guess about data from somewhere else. What remains untyped is the three ignored catch clauses, which TypeScript gives as unknown under strict, and which is correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now that the kind of test being run is named rather than merely called a string, the switch which runs it covers every kind there is, and the return beneath it can no longer be reached. Removing it means a test added without a case is reported, where before it would quietly have been answered with false: the browser would have been recorded as unable to render the new emoji, and Twemoji loaded for no reason. Note that the compiler says nothing about this as configured. Unreachable code is a suggestion by default, which editors show and the command line does not, and allowUnreachableCode is not among the things strict turns on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TypeScript treats unreachable code as a suggestion unless told otherwise, which editors show and the command line does not, and it is not among the things strict turns on. Anyone running the check would see nothing, while anyone with the file open would see it marked. Setting allowUnreachableCode reports it either way. Every file currently checked passes with it on, so nothing needs fixing to adopt it; it is here to catch what comes next as more files are added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…them all The observer tests each made one change and then waited, so the observer was only ever given a single record at a time, and a single record is exactly the case in which skipping one and abandoning the rest look the same. Returning from the callback rather than continuing past the record it recognized passed the whole suite. The test added here makes both changes before waiting, so that the two arrive together, and puts the fallback first. The element added beside it must still be parsed. Returning instead of continuing now fails, as does any later change which makes the callback sensitive to the order the records arrive in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Address the missing SVG/MathML regression tests and incomplete settings typing; also correct the test assertion message.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/js/_enqueues/wp/emoji.js:113
- The new
HTMLElementgate is the behavior that prevents an added<svg>or MathML root from being handed to Twemoji, but the suite has no observer test for either case. A regression to the old root-node handling would therefore pass all 29 tests; add cases that append SVG and MathML roots containing emoji and assert their text remains unchanged and Twemoji is not called.
tests/qunit/wp-includes/js/wp-emoji.js:410
- This assertion message contradicts the test: without the
data-errormarker, the replacement is expected to be parsed, but the message says the marker stops parsing. A failure would therefore point maintainers toward the wrong behavior; update the message to describe the unmarked replacement being parsed.
assert.strictEqual( twemoji.lastObject, nodes.paragraph, 'It is the marker, and nothing else, which stops the replacement being parsed.' );
- Files reviewed: 4/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
What keeps an SVG or MathML element from being parsed is the test that the element is an HTML one, and nothing exercised it. Widening that test back to any element, which is what the observer did before, passed the whole suite. Three cases are added: an SVG element and a MathML element, each holding an emoji and each added after the page has loaded, and a text node added within an SVG element, which reaches the same test by way of the element containing it. None of the three may reach Twemoji, which would otherwise put an HTML image inside markup where one does not belong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The message on its closing assertion described the pair of tests it belongs to rather than the one thing that assertion establishes, and described it the wrong way round: the replacement is parsed here, precisely because the marker is absent. Read on a failure, which is the only time it is read, it pointed at the opposite behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
westonruter
marked this pull request as ready for review
September 17, 2026 23:46
|
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. |
These were written from what wp-emoji passes rather than from what Twemoji accepts, and were incomplete as a result. Checking them against the vendored copy, which is the one that matters since it carries WordPress's own patch, turned up three things. Neither size nor folder was declared, though parse() reads both when working out where the assets are, so passing either would have been rejected. Nor did the attributes callback admit returning null, which is what Twemoji falls back to when none is given, and what wp-emoji itself returns when handed a null in place of the attributes. The resolved options a callback receives always carry a size, so that is said too. Only what wp-emoji uses is described, which is now stated rather than left to be discovered: the library also exposes replace(), test() and convert, and parse() takes a callback in place of the options object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The library was swapped for the jdecked fork in r57626, after the original was abandoned following the change of ownership at Twitter, and everything about the vendored copy has said so since: its licence header, its default asset URL, and the version it reports. The docblock at the top of wp-emoji was not updated with it, and still sent readers to the repository which was left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds both emoji scripts to the TypeScript checked files, fixes the defects that surfaced along the way, modernizes the JavaScript, and gives them their first tests.
For the loader this continues r60899 and r60902, which converted it to a script module and modernized it in passing. Neither touched
wp-emoji.js, which is why one of the two already read as current JavaScript and the other did not.Each commit is one change with its own reasoning, and was verified in isolation.
Defects fixed
The fallback for an emoji image which fails to load has never worked. The handler given to Twemoji referred to the library object in three places where it meant the image element. The library object has no
parentNode, so the handler returned having done nothing: a reader was left with a broken image icon rather than the character the image stood for. Introduced in r35637 for Core-34640, so it has been shipping since 4.4.The
MutationObserverlooks for thedata-errorattribute that handler was supposed to set, in order to recognize a replacement it must leave alone, so that branch was unreachable too. The two need each other — without the attribute, the observer parses the replacement text straight back into the image which has just failed.That guard abandoned every remaining mutation record rather than the one it recognized, which made the callback sensitive to the order records were visited in. It could not be left for later: the moment the handler sets the attribute the guard becomes reachable, so correcting the handler alone would have traded one defect for another. Measured with only the handler corrected, an element added alongside a failed image is dropped; with the guard corrected too, it is kept.
An
<svg>element had its text replaced with<img>elements. Twemoji avoids SVG when collecting text to replace, but applies that test to the descendants of whatever it is given rather than to the node itself, so an<svg>arriving as an added node was parsed. The same went for MathML.The exclusion class was matched as a substring, so
wp-exclude-emoji-wrapper,my-wp-exclude-emojiandwp-exclude-emojisall excluded an element and everything beneath it.Previously, PhpStorm reported the following problems with
emoji.js:There are now zero problems reported for either
emoji.jsoremoji-loader.js.Code which could not run
const, failing before it reaches the guarded block. Confirmed by parsing the built file with acorn atecmaVersion: 5.browserSupportsSvgAsImage(). It askeddocument.implementation.hasFeature(), which the DOM standard requires to returntruefor any argument, so the PNG fallback was unreachable in both directions.MutationObserver, and the check that it exists.browserSupportsEmoji(), once the kind of test being run is named rather than merely called a string. Removing it means a test added without a case is reported, where before it would quietly have been answered withfalse.TypeScript
Thirty errors were reported for the loader and twenty-seven for
wp-emoji.js. Two entries are added undertypings/: one forWorkerGlobalScope, which thedomlib does not provide and which thewebworkerlib cannot be used for as the two conflict; and one for the settings PHP exports through_print_emoji_detection_script()together with the vendored Twemoji library, which live outside either script because both read them. The settings were also incompletely documented —baseUrl,ext,svgUrlandsvgExtare all exported and read, and none were described.The functions the support tests are handed are described with callbacks rather than as a bare
Function, which is what makes the one call tobrowserSupportsEmoji()checkable at all: passing them in the wrong order, or asking for a test which does not exist, is now reported. Worth having for code which is turned into a string and run in a Worker, where a mistake of that kind surfaces as emoji quietly not being replaced.allowUnreachableCodeis turned on intsconfig.json. Unreachable code is a suggestion by default, which editors show and the command line does not, so it was visible to anyone with the file open and invisible to CI. Every checked file passes with it on, so nothing needed fixing to adopt it.One thing worth knowing for the wider effort in Core-65997: TypeScript passing is not on its own enough to say an annotation is a good one. Describing what
wpEmoji()returns astypeof parsetype checks perfectly well, and leaves PhpStorm unable to say whatwp.emojiis at all, which has it report every call through that as possibly not a function. Writing the signatures out suits both.Tests
Thirty-three QUnit tests, run against both the unminified and the minified script. They cover
wp.emoji.test(), everythingwp.emoji.parse()decides, the callbacks it composes but does not otherwise expose, and the observer it installs.Every defect above is covered by a test which fails without its fix. Each was checked by reverting that fix in the source, running the suite, and confirming the right tests go red by name on both pages, then restoring.
Two gaps in that coverage were found in review rather than by that check, and are worth recording. The observer tests originally made one change apiece and then waited, so the observer was only ever given a single record, which is exactly the case where skipping one record and abandoning the callback look the same; and nothing exercised the test which keeps an SVG or MathML element away from Twemoji. Both are covered now, and each was confirmed by reverting the fix it guards.
The observer tests need a little care, noted in the file for whoever comes next: every test in this suite is wrapped by sinon-test, which swaps the timers for a fake clock, so waiting for the observer with a timer hangs the suite rather than failing it. Mutation records arrive in a microtask, and waiting on one of those works.
The loader has no tests. It exports nothing and does its work when imported, so there is no seam to test through; giving it one is a larger change and belongs elsewhere.
Size
wp-emoji.min.jswp-emoji-loader.min.jsThe loader grows. Three assumptions it had always made are now written down and cost a few bytes each: that a 2D context was obtained, that session storage returned something, and that
URL.createObjectURLis callable rather than merely present.Not included
Core-58663, the expiry on the cached support tests, is untouched.
Core-66120 is not fixed here. An excluded element is still parsed when it is itself the node handed to
parse(). Running that ticket's cases against this branch gives the same counts as against trunk, including the case its reporter had not tested: text added inside an excluded element is replaced too. The two do meet in one place, though — the fix proposed there is a check in the observer beforeparse()is called, and this rewrites that loop, so whichever lands second will need rebasing.Trac ticket: https://core.trac.wordpress.org/ticket/66131
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Substantially the whole of it. The code changes, the QUnit tests, the commit messages and this description were authored by the model working from my direction, and were reviewed and corrected by me throughout; two of the thirty commits are entirely mine. Every behavioral claim above was verified by running it — reverting each fix and watching the relevant test fail by name, parsing the built file to confirm the IE 11 branch is unreachable, and reproducing Core-66120 against both trunk and this branch — rather than taken on the model's word. The two coverage gaps noted above were found by Copilot's review, not by that process.
🤖 Generated with Claude Code
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.