Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#44339 closed task (blessed) (fixed)

Emoji: Update Twemoji to 11.0

Reported by: kraftbj Owned by: pento
Priority: normal Milestone: 4.9.8
Component: Emoji Version:
Severity: normal Keywords: fixed-major has-patch
Cc: Focuses:

Description

Twemoji changed versioning scheme to match Unicode. Twemoji 11 adds the Unicode/Emoji 11 new emoji.

See release notes at ​https://github.com/twitter/twemoji/releases/tag/v11.0.0

twemoji.js: ​https://github.com/twitter/twemoji/blob/v11.0.0/2/twemoji.js

Previously: #42862 , et al.


Attachments (7)

44339.diff​ (3.4 KB ) - added by kraftbj 8 years ago.
Bump package.json to 11.0 and change CDN to v11 assets
44339.2.diff​ (4.4 KB ) - added by kraftbj 8 years ago.
Add test for superhero+ZWJ+female
44339.png​ (4.6 KB ) - added by peterwilsoncc 8 years ago.
Failing safari test
44339.3.diff​ (4.4 KB ) - added by kraftbj 8 years ago.
Updated test to work in Safari.
44339.4.diff​ (8.3 KB ) - added by kraftbj 8 years ago.
Fresh run of npm install
44339.5.diff​ (163.7 KB ) - added by pento 8 years ago.
44339.6.diff​ (168.1 KB ) - added by kraftbj 8 years ago.
Patch for the 4.9 branch

Download all attachments as: .zip

Change History (29)

#1 @kraftbj
8 years ago

I started messing around trying to figure out how to update the various bits needed for this; sharing my work here.

  • We need to add a test to check for Unicode 11 support. We could use U+1F469 U+200D U+1F9B0 (woman + ZWJ + red hair ).

Attaching a patch to bump the package.json to 11.0 and updating the CDN assets.

Last edited 8 years ago by kraftbj (previous) (diff)

@kraftbj
8 years ago

Bump package.json to 11.0 and change CDN to v11 assets

This ticket was mentioned in ​Slack in #core by kraft. ​View the logs.


8 years ago

#3 @peterwilsoncc
8 years ago

I’ve got a jsbin I use to test the tests that might help with writing them at ​https://jsbin.com/palagay/4/edit?js,output

In the past we’ve hit a bug with skin tone modifiers returning false positives in safari that I’m worried we might hit with hair colour modifiers. I suspect it’s because they’re modifiers not intended to be displayed on their own.

Gender modifiers test correctly so is it possible to use superhero + ZWJ + a gender?

#4 @kraftbj
8 years ago

Female superhero works for me. I'm giving that a go now and will push up an updated patch shortly.

@kraftbj
8 years ago

Add test for superhero+ZWJ+female

#5 follow-up: @kraftbj
8 years ago

The test works for me on OS X in Chrome and Firefox, but is not catching Safari's lack of support.

@peterwilsoncc
8 years ago

Failing safari test

#6 in reply to: ↑ 5 @peterwilsoncc
8 years ago

Replying to kraftbj:

The test works for me on OS X in Chrome and Firefox, but is not catching Safari's lack of support.

I can reproduce the failing test. See 44339.png​ for result in safari from ​https://jsbin.com/palagay/7/edit?js,output

Failing safari test

I'm unsure of the cause, @pento do you have any suggestions?

#7 @pento
8 years ago

It appears that Safari attempts to render the ZWS character (8205). Removing that character from the first array fixes the test.

#8 @kraftbj
8 years ago

  • Keywords needs-testing added

Thanks @pento. Attaching a version of the patch that does not include the ZWS. It works for me using Firefox Dev and Safari stock on latest OS X.

@kraftbj
8 years ago

Updated test to work in Safari.

#9 @kraftbj
8 years ago

  • Keywords needs-testing removed

Tested via OS X in Safari, Safari TP, Chrome, Chrome Canary, Firefox, Firefox Dev, Opera, Opera Dev. Tested via Browserstack's Windows 10 client in latest version Chrome, Firefox, IE, Edge, etc. Safari for Windows squished the graphics a tad, but I don't believe that is related to this ticket.

#10 @peterwilsoncc
8 years ago

  • Keywords has-patch added
  • Owner set to pento
  • Status new → assigned

Thanks @kraftbj,

Before it can go in to core I'm assigning it to @pento for committing to the w.org CDN (apologies if you have done/can do it, I'm not up to date with meta committers).

If there is a risk s.w.org/images/core/emoji/11 has been warmed with 404s during testing, we might need to make the directory s.w.org/images/core/emoji/11.0 depending how aggressively 404s are cached.

#11 follow-up: @pento
8 years ago

​Twitter is the only reliable commit notification system.

Minor note on 44339.3.diff​, you need to revert the change to package-lock.json, (maybe remove your node_modules directory) upgrade to NPM 6.1, then run npm install again, so that it creates sha512 signatures, instead of sha1.

#12 @kraftbj
8 years ago

Thanks @pento! Appreciate your patience-my first patch touching all this new build fun. I reverted, rm -rf node_modules, vvv already has 6.1 (that may have been upgraded after I did the twemoji update), and ran npm install, but it still gave me a sha1 best I can tell. It made some other changes.

Attaching what it gave me.

@kraftbj
8 years ago

Fresh run of npm install

@pento
8 years ago

#13 @pento
8 years ago

Hrrm, that's interesting. package-lock.json can have some strange behaviour at times. 🙃

I'm not sure why it doesn't update the signature to use sha512, but 44339.5.diff​ is reasonable, I think.

I've also included the results of grunt precommit:emoji, which updates the list of HTML entities that twemoji can replace.

#14 @pento
8 years ago

  • Resolution → fixed
  • Status assigned → closed

In 43377:

Emoji: Update Twemoji to version 11.0.

🦹

Props kraftbj,
Fixes #44339.

#15 @pento
8 years ago

  • Keywords fixed-major needs-patch added; has-patch removed
  • Resolution fixed
  • Status closed → reopened

Reopen for backport, though the patch will need a refresh, due to the 4.9 branch including a copy of twemoji in src, instead of in package.json.

@kraftbj
8 years ago

Patch for the 4.9 branch

#16 @kraftbj
8 years ago

  • Keywords has-patch added; needs-patch removed

Refreshed the patch for 4.9.

#17 in reply to: ↑ 11 @netweb
8 years ago

Replying to pento:

... sha512 signatures, instead of sha1.

This is typically due to cached packages locally, the npm cache has to be force cleaned to remove the old sha1's:

  • npm cache clear -force

#18 follow-up: @kraftbj
8 years ago

rm -rf node_nodules && npm cache clear -force && npm install didn't give me a sha512. I haven't dug into it beyond that though.

#19 in reply to: ↑ 18 @netweb
8 years ago

Replying to kraftbj:

rm -rf node_nodules && npm cache clear -force && npm install didn't give me a sha512. I haven't dug into it beyond that though.

Maybe also try deleting the package-lock.json file, at the moment this issue seems more like art than science :(

#20 @ocean90
8 years ago

  • Milestone 4.9.7 → 4.9.8

4.9.7 has been released, moving to next milestone.

#21 @kraftbj
8 years ago

@netweb Do you want to give it a try on your end or do you think this is fine enough to commit as it stands?

#22 @pento
8 years ago

  • Resolution → fixed
  • Status reopened → closed

In 43444:

Emoji: Update Twemoji to version 11.0.

🦹

Backport of [43377] to the 4.9 branch.

Props kraftbj,
Fixes #44339.

Note: See TracTickets for help on using tickets.