#52219 closed defect (bug) (fixed)
wp-emoji.js should always skip nodes with the `wp-exclude-emoji` CSS class
Reported by: | GeorgeJipa | Owned by: | azaozz |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 4.2.2 |
Component: | Emoji | Keywords: | has-patch commit |
Focuses: | javascript | Cc: |
Description
I just discovered it only happens only if the node is added later to the DOM.
Change History (17)
This ticket was mentioned in PR #3774 on WordPress/wordpress-develop by @dd32.
2 years ago
#2
- Keywords has-patch added
This iterates up the tree from the textNode looking for emoji's and skips if the .wp-exclude-emoji
class is present on any parent Elements.
This also causes .wp-exclude-emoji
to be respected on elements that were on the page at pageload, unlike the existing .wp-exclude-emoji
implementation.
Trac ticket: https://core.trac.wordpress.org/ticket/52219
A patch to Twemoji is required for this change to be effective on elements that are present on the pageload, and not run through the MutationObserver, as it's not possible to know which element Twemoji is operating upon otherwise.
-
js/twemoji.js
372 372 rawText = match[0]; 373 373 iconId = grabTheRightIcon(rawText); 374 374 i = index + rawText.length; 375 src = options.callback(iconId, options );375 src = options.callback(iconId, options, subnode); 376 376 if (iconId && src) { 377 377 img = new Image(); 378 378 img.onerror = options.onerror;
#3
@
2 years ago
- Keywords has-patch removed
I've run into this again, but this time on the WordPress.org Support forums now that we support the Block Editor for replies and the emoji replacer is replacing emojis within the editor interface. Ideally.. it wouldn't.. as the emoji's should be replaced upon render, not upon save.
PR #3774 is an attempt at resolving this, but it has a few shortcomings:
- In order to make twemoji respect the
.wp-exclude-emoji
class on pageload, a patch to Twemoji is required. - Performance wise, this requires every emoji's parents to be iterated upon
2 years ago
#5
Thinking if we are going to "patch" twemoji.js, may as well alter the way it collects the text nodes when looking for emoji. Can exclude any nodes with a specific class (which can be passed in the settings) and their sub-nodes. Would be fastest and shouldn't be hard to maintain.
Or even better: add another callback when determining if a node should be parsed. Something like this should work:
// ignore all nodes that are not type 1, that are svg, or that // should not be parsed as script, style, and others else if (nodeType === 1 && !('ownerSVGElement' in subnode) && !shouldntBeParsed.test(subnode.nodeName.toLowerCase())) { // WP start if ( twemoji.doNotParse && twemoji.doNotParse( subnode ) ) { continue; } // WP end grabAllTextNodes(subnode, allText); }
This ticket was mentioned in PR #3800 on WordPress/wordpress-develop by @azaozz.
2 years ago
#6
Patch twemoji.js to add support for a doNotParse()
callback. Use that callback to always exclude emojis in elements with wp-exclude-emoji
class.
Trac ticket: https://core.trac.wordpress.org/ticket/52219
2 years ago
#7
This seems to be working well here. It slows down parsing in twemoji.js by about 9ms here when testing with a "large" post of about 5k words. That, of course, would depend on the browser and the computer/device processing speed. The slow-down is even smaller if there are elements with the wp-exclude-emoji
class.
2 years ago
#8
@azaozz I agree, I was going for the minimal approach in changes, but your minimal change is even more minimal.
I'm going to close this ins preference for #3800
2 years ago
#9
It slows down parsing in twemoji.js by about 9ms
9ms seems insignificant, or significant.. What's the full processing time in that case? Is it 20ms or 100ms?
@peterwilsoncc commented on PR #3800:
2 years ago
#10
if we also propose it upstream as a PR to twemoji seems like a good enough approach to me.
It would be good to try getting something in upstream prior to a local patch, are either of you able to set up a PR? I'm unclear what's changed in the file due to it coming over in its entirety.
2 years ago
#11
It would be good to try getting something in upstream prior to a local patch
Yes definitely. Having a way to exclude certain HTML elements from parsing was discussed few times before. Even think the idea to have a callback was suggested by the Twemoji's developers (sorry, don't seem to be able to find where the discussions took place, was some years ago). There's another, more recent request: https://github.com/twitter/twemoji/issues/370#issuecomment-515755373.
Twemoji implemented support for the Unicode variation selectors) to request emoji or text presentation of a character (works by adding VS15 (U+FE0E) to force text presentation, and VS16 (U+FE0F) for emoji presentation).
Tried solving the WordPress issue by appending \uFE0E
to emojis inside nodes with wp-exclude-emoji
class before parsing. However this only works for some emojis. Most are unaffected and Twemoji still replaces them with images. Even if it worked for all appending \uFE0E
to emojis dynamically (like inside an editor) would be pretty cumbersome as it would have to instantly change what the user is pasting/typing.
Thinking it would probably be good to add this fix to core for 6.2 and if this feature becomes available in Twemoji, switch back to using an unpatched version.
2 years ago
#12
I just rewrote this PR to split the initial commit into two: a) Add a local twemoji, and b) make the alterations to it 8426f7fd89bd1a64f1f4953c396a3c4dbb08c25a, just so that it's clear what this PR actually does.
Using the Unicode variation selectors) as noted does work, but requires double-parsing the content, first to modify the contents to add the selector and second to convert the emojis. It's a great solution for one-offs but not really for dynamics.
(Apologies @azaozz if I broke something, please verify and If I screwed something up, just force-push your local branch again I guess)
#14
@
23 months ago
- Keywords commit added
- Milestone changed from Awaiting Review to 6.2
Thinking this should be committed to 6.2 to ensure emojis are not replaced with images in inline editors on the front-end. If a similar feature is implemented upstream, this can be removed/reverted with the next twemoji.js update.
23 months ago
#15
Regarding upstream, it seems like Twemoji may be a causality of the changes at Twitter or at least it is uncertain. For the sake of transparency and tracking what alternatives may exist, opened https://core.trac.wordpress.org/ticket/57600
#16
@
23 months ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 55186:
23 months ago
#17
Committed in https://core.trac.wordpress.org/changeset/55186.
Relevant code: https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/wp/emoji.js?marks=148-170#L140
The only place the
wp-exclude-emoji
class is checked is within the MutationObserver, which is only called for DOM insertions. It appears that this is probably intentional, based on the original implementation via #32197parse( document.body )
causes Twemoji to process every textnode, and the callback presented provides no context as to what element it's operating on - so we can't even return false there (to cause it to generate no img replacement).The reference below to
twemoji.parentNode
gave me hope, but this appears not to be set anywhere I can find (either while debugging, or looking at the twemoji source).I suspect this is going to require an upstream bug report to allow some form of context to be made available to the callback, so that we can return a falsey value here to abort the replacement.
Setting version to 4.2.2, as it appears this has always been how this functionality works.