Make WordPress Core

Opened 3 years ago

Closed 17 months ago

Last modified 17 months ago

#52219 closed defect (bug) (fixed)

wp-emoji.js should always skip nodes with the `wp-exclude-emoji` CSS class

Reported by: georgejipa's profile GeorgeJipa Owned by: azaozz's profile 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)

#1 @dd32
23 months ago

  • Version changed from 5.6 to 4.2.2

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 #32197

parse( 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.

This ticket was mentioned in PR #3774 on WordPress/wordpress-develop by @dd32.


18 months 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

     
    372372        rawText = match[0];
    373373        iconId = grabTheRightIcon(rawText);
    374374        i = index + rawText.length;
    375         src = options.callback(iconId, options);
     375        src = options.callback(iconId, options, subnode);
    376376        if (iconId && src) {
    377377          img = new Image();
    378378          img.onerror = options.onerror;

#3 @dd32
18 months 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

#4 @dd32
18 months ago

  • Keywords has-patch added

@azaozz commented on PR #3774:


18 months 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.


18 months 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

@azaozz commented on PR #3800:


18 months 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.

@dd32 commented on PR #3774:


18 months 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

@dd32 commented on PR #3800:


18 months 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:


18 months 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.

@azaozz commented on PR #3800:


18 months 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 \uFE0Eto 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.

@dd32 commented on PR #3800:


18 months 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)

@azaozz commented on PR #3800:


17 months ago
#13

I just rewrote this PR

Thanks, looks good!

#14 @azaozz
17 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.

@kraftbj commented on PR #3800:


17 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 @azaozz
17 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 55186:

Emoji: Always skip nodes with the wp-exclude-emoji CSS class.

Patches twemoji.js to add support for a doNotParse() callback. Uses that callback to always exclude emojis in HTML elements with the above class.

Props: dd32, peterwilsoncc, azaozz.
Fixes #52219.

Note: See TracTickets for help on using tickets.