Improve handling of skin tone variant emoji
Changes PlannedPublic
Actions

Authored by epriestley on Feb 7 2018, 6:53 PM.

Details

Reviewers

Maniphest Tasks

T12644: Emoji autocomplete ordering doesn't follow unicode standards
T13063: Maybe render all text with IMPRESSIVE illuminated drop caps

Summary

Ref T13063. Fixes T12644. Currently, our emoji name list comes from EmojiOne. Their source list is okay, but denormalizes a bunch of variants, including skin tones.

This is cumbersome for users and can feel a little tone deaf (ha ha ha).

Improve this by:

Marking which emoji accept skin tone modifiers.
Collapsing them in the typeahead, so you just get one :man:, not :man: thorugh :man_tone_5:.
Adding a preferred skin tone setting, so you can make all your :thumbsup: have darker or lighter skin by default if you want, without needing to scroll through the list every time.
Default the skin tone setting to randomly select among all the tones.
Allow emoji to be ordered non-alphabetically for cases like "thumbsup" vs "thumbsdown".

This addresses T12644 by removing all the variants of "thumbs", and then making "thumbsup" come first, so :thu now autocompletes to :thumbsup:.

Test Plan

Entered tone (:woman:) and non-tone emoji (:dog:).
Typed :thumbs, got thumbs up by default.
Typed :clap: a bunch, got a range of human skin tones.
Set my preferred tone, got just that tone.

Diff Detail

Repository

rP Phabricator

Branch

emoji2

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 19386
Build 26222: Run Core Tests
Build 26221: arc lint + arc unit

Event Timeline

epriestley created this revision.Feb 7 2018, 6:53 PM

Harbormaster completed remote builds in B19386: Diff 45592.Feb 7 2018, 6:54 PM

epriestley requested review of this revision.Feb 7 2018, 6:54 PM

Two general thoughts:

The default behavior (random human skin tone) might be a little surprising/weird; I don't think other applications generally do that. I think it's pretty cool, but maybe it's not as cool as I think. For emoji like :man: and :child: it feels appropriate to me, but for emoji that are sort of self-attributed like :clap: and :raised_hands: maybe it's not that great. We could make the default "unspecified" instead if this feels weird, or we could make random only apply to the "people" emoji (man, woman, bride, child, mountain biker, construction worker, etc) and not to the "body parts" emoji (hand, v, clapping, raised hands, thumbs up, etc).
I'd still like to let you pick a tone explicitly at some point with a fancier typeahead (e.g., use left and right to cycle through options or something) but the default control doesn't have a convenient way to do that.

Looks good except for minor nitpicks.

resources/emoji/manifest.json
1345–1354	This feels like more of a one-off than a real fix for T12644.
src/applications/macro/typeahead/PhabricatorEmojiDatasource.php
70–75	Can we DRY this up? And is this rule about tone modifiers part of the spec, or just a fortunate implementation detail?
91–97	This is how we avoid breaking the rendering of existing more-specific emoji, right?
src/applications/settings/setting/PhabricatorSkinToneSetting.php
29	"for the emoji that you send"? Or will this in fact render all emojis with the user's requested skin tone?
33	This should itself be a setting 😛
39	For consistency, should this be "Unspecified", "Neutral", or "None"?

This revision is now accepted and ready to land.Feb 7 2018, 8:04 PM

The order is definitely a one-off -- I'm only really solving "thumbs up is hard to type" pretty narrowly -- but I think we probably have to do some one-offs no matter what and I don't know any other examples of stuff that's total junk right now offhand.

I think both alphabetical and Unicode ordering have some unintuitive results in the context of a typeahead: for example, Unicode ordering puts :apple: before :a: if you type :a, and :banana: before :b: for :b. We could maybe do "Unicode ordering, but exact match if you type a thing exactly", but I bet there are some cases that gets questionable too (although maybe that's actually close enough to perfect).

Unicode ordering also puts :pear: before :peach:, although someone typing :pea is almost certainly after :peach:. Of course, alphabetical ordering puts :peace: first, which is less likely than either. Maybe we really want frequency ordering, or per-user "frecency" that remembers your previous selections?

I imagine we'll have a clearer picture before the one-offs get out of control, but I'm not really sure where the issues are right now. (But maybe you already have some other examples offhand where alphabetical is doing bad things?)

(I'll clean the code stuff up a bit.)

src/applications/settings/setting/PhabricatorSkinToneSetting.php
29	It's just emoji that you type. I like your phrasing much better than mine.
33	I intentionally made the default-default not selectable because an administrator setting the global default to "Light Skin Tone" is probably asking for trouble, just like we don't let administrators set the default pronoun to "his". There's currently no way to say "you can set the global default to 'random' or 'neutral' but not anything else", although if there was that would probably be reasonable. Not sure if it's worthwhile to add it just for this. Maybe if we get a mix of positive and not-so-positive feedback.