Closed Bug 993899 Opened 8 years ago Closed 6 years ago

[Keyboard] Emoji layout in keyboard

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:3.0?, tracking-b2g:backlog)

RESOLVED FIXED
feature-b2g 3.0?
tracking-b2g backlog

People

(Reporter: Omega, Assigned: rakhavan)

References

Details

(Keywords: feature, foxfood)

Attachments

(4 files, 1 obsolete file)

As a user, I can input Emoji with built-in Keyboard.
We should totally do this.  And I suspect that it would be pretty easy to add new layout pages to our existing layouts. We'd just have to figure out where to squeeze in the mode switch key.

Rudy: what you do you think about this?

Carrie or Juwei: Do you have UX comments about supporting emoji?
Flags: needinfo?(rlu)
Flags: needinfo?(jhuang)
Flags: needinfo?(cawang)
I'm assuming, however, that our default font supports emoji.  If not, then this becomes a lot harder.
At the begin of this week I was looking for the keyboard recommendations and found [1]. It is a little old (from 2012) it says in slide 2:

"Tap and hold to access common smileys (or emoji) and punctuation symbols."

Based on the recommendations we don't need to add new layout since it should works like the punctuation symbols.

I want to work on this as part of my GSoC project.

[1] https://wiki.mozilla.org/images/2/20/Keyboard_Specs.pdf
As a practical matter, there is probably only room for one or two smilies or emoji characters in the alternate character menu for '.', so this can only be a partial solution. Our Fira font does not seem to include emoji, but if I visit the wikipedia emoji page in the FirefoxOS browser it does display emoji characters, so I guess we support them somehow.
Attached file Add emoticon
Attachment #8426522 - Flags: review?(dflanagan)
My last attachment add support to emoticons. For emoji see https://github.com/r-gaia-cs/gaia/commit/371dc3f099cfba1c59c9af50d21e90b4044cd525.

@David AFAIK our font only support a VERY small number emoji and that's the reason that I submit the patch with emoticons.
Comment on attachment 8426522 [details] [review]
Add emoticon

This is interesting as a proof-of-concept, but it is premature to review without first getting thoughts from the keyboard UX team about what is wanted here.

I'd prefer a solution that did not need JS code to make the emoticon key appear: there ought to be a way to just have that in the layout file. Also, this patch probably interacts poorly with the email and url layouts which have other special keys to the left of the spacebar.  (The UI tests app includes input fields of these types so you can check your layouts for urls and email addresses.)

Our Fira sans font does not have emoji in it, but somehow our browser app displays emoji, presumably with a different font.  So if we want to support emoji we may need to use an additional font.  Note that this would mean we would have to use the additional font in every app that used the keyboard or it would be possible to type characters that could not be displayed. So this might be an issue that goes far beyond the keyboard itself.
Attachment #8426522 - Flags: review?(dflanagan)
See http://djf.net/smile.html for a test that demonstrates at least one emoji character in our browser.
Hi all,

I think it is reasonable to add one more layout for emoji instead of just embedding those chars into one key's alternative char menu.

If we go this way, then the question would be if we could switch to emoji layout with just the ime switch key (the globe key) or we need to add another mode-switching key for this, as David mentioned in comment 1.

--
Raniere,

Thanks for looking into this, but as you may already, the spec you're reading is an old one, and many things change since then. 
BTW, we have a spec in bug 983043 for keyboard update 2.0. However, it seems this emoji part was not covered yet.

Thanks.

--
++ni Omega, for he is the recent keyboard UX goto-man, AFAIK. :)
Flags: needinfo?(rlu) → needinfo?(ofeng)
Flags: needinfo?(jhuang)
(In reply to Rudy Lu [:rudyl] from comment #9)
> I think it is reasonable to add one more layout for emoji instead of just
> embedding those chars into one key's alternative char menu.
> If we go this way, then the question would be if we could switch to emoji
> layout with just the ime switch key (the globe key) or we need to add
> another mode-switching key for this, as David mentioned in comment 1.
> ++ni Omega, for he is the recent keyboard UX goto-man, AFAIK. :)

I think it's good idea to treat Emoji as an independent keyboard (use IME switch key) since it has a lot of tabs and controls already.

@Bruce, IIRC we'll target Emoji to v2.1, right?
Flags: needinfo?(ofeng) → needinfo?(bhuang)
Flags: needinfo?(cawang)
Duplicate of this bug: 1028352
Just found that we already have emoji support in our font system, so we could implement this.
Depends on: 939280
Attached image emoji layout screenshot
Back when the emoji font first landed I did this little experiment. Having a separate layout for emoji may look great, but it could be greater with a little more customizations since it's not just a bunch of letters in 4 or 5 rows & lines.
I'd say a global secondary layout with custom styles (a little different from the usual keyboard styling) and accessed through a smile ":)" button between "space bar" and "." buttons..
We could also give the user the ability to enable/disable it
?
We have a spec update on emoji support,
Please refer to bug 983043 comment 23.
I took a look at the spec but didn't understand how the user will move into emoji layout and go out of it.
(In reply to Raniere Silva from comment #15)
> I took a look at the spec but didn't understand how the user will move into
> emoji layout and go out of it.

I think this should live as a separate layout, so you switch to emoji with [IME switching] key and back to other layouts, like English, with the same action, I suppose [IME switching] key on emoji layout is that smiley icon located at the lower left button.
Priority: -- → P1
(In reply to Rudy Lu [:rudyl] from comment #17)
> I think this should live as a separate layout, so you switch to emoji with
> [IME switching] key and back to other layouts, like English, with the same
> action, I suppose [IME switching] key on emoji layout is that smiley icon
> located at the lower left button.

If, for example, you have only English and Emoji layout this will be ok but if you have French, Germany and Emoji layouts this will became a problem since you will need to press more than one key to get the Emoji layout and/or back to the language layout.

Omega, could you get a feedback from UX team for this comment?
Flags: needinfo?(ofeng)
(In reply to Raniere Silva from comment #18)
> (In reply to Rudy Lu [:rudyl] from comment #17)
> > I think this should live as a separate layout, so you switch to emoji with
> > [IME switching] key and back to other layouts, like English, with the same
> > action, I suppose [IME switching] key on emoji layout is that smiley icon
> > located at the lower left button.
> 
> If, for example, you have only English and Emoji layout this will be ok but
> if you have French, Germany and Emoji layouts this will became a problem
> since you will need to press more than one key to get the Emoji layout
> and/or back to the language layout.
> 
> Omega, could you get a feedback from UX team for this comment?

I Agree.. And another suggestion for how this should work is having a separate key between [Space] and [.] keys (or somewhere else easy to access) that acts like a secondary layout just like the [12&] Key.
And It should be accessible through all keyboard layouts..
I agree it's easier to switch to Emoji layout with a key in each layout, but that causes some problem:
1) The bottom row of each layout is already cramped. Adding any key there would make UX bad.
2) If we don't make Emoji a separate layout, user will have no chance to use 3rd party keyboard with Built-in Emoji.

To avoid the cons, we decided to use a separate layout for Emoji.
Flags: needinfo?(ofeng)
Attached file 2.1 Emoji Keyboard_Visual spec.zip (obsolete) —
Hi,
The attachment is "2.1 Emoji Keyboard_Visual spec"
Please let me know if there's any question about the visual spec.
Thanks!
(In reply to Omega Feng [:Omega] [:馮於懋] from comment #10)
> 
> @Bruce, IIRC we'll target Emoji to v2.1, right?

Yes, but we'll confirm it after completing the 2.1 blockers.
Flags: needinfo?(bhuang)
Update Emoji Keyboard Visual spec:
1. Categoery tab width
2. image assets (on/off/pressed)
Attachment #8466119 - Attachment is obsolete: true
The confusing part here is how the smilies/emojis themselves are actually going to be implemented. Are there going to be a script that converts plain text like ":-)" & ":-/" &  ":-(" to the icons provided and vise versa? If so, how fox emojis are constructed? Something like :happyfox: ?

If they're going to be inserted as <img> within html? How are going to deal with the SMS case?

So many details I don't see them being discussed anywhere? :)
emoji's have their own utf8 code.
:Nefzaoui Emoji's follow the Unicode standard as Jan was saying. They're all outlined here: https://mozilla.app.box.com/files/0/f/2098581252

Pavel/Rudy, what's the status on these emojis? What's it looking like at this point? Anything I can do to help? Pavel, last I checked in, I asked what size would be optimal for me to export at. Let me know :)
Flags: needinfo?(rlu)
Flags: needinfo?(pi)
Pavel will help to work on the font part, and I am going to implement a new keyboard layout for this, but haven't started yet, will send my patch here for feedback when I have WIP.
Assignee: nobody → rlu
Flags: needinfo?(rlu)
blocking-b2g: --- → 2.2?
Feature does not block, marking feature-b2g 2.2? for scoping.
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
See bug 983043 for UX spec update.
feature-b2g: 2.2? → 2.2+
Flags: needinfo?
blocking-b2g: --- → backlog
QA Whiteboard: [COM=Gaia::Keyboard]
Flags: needinfo?
Keywords: feature
Mass-unassign feature-b2g: 2.2+ to re-scope 2.2 features.
feature-b2g: 2.2+ → ---
tracking-b2g: --- → +
Marking 2.2 features for 3.0 considerations.
feature-b2g: --- → 3.0?
blocking-b2g: backlog → ---
Assignee: rlu → nobody
Just connecting the dots...Thomas Beduneau has an emoji keyboard in the marketplace. This might serve as a useful starting place, and a solution for users until this bug lands.

https://marketplace.firefox.com/app/emoji-keyboard/
https://github.com/enwin/emoji

The design is similar to Carol's spec, but not identical. It does share the ime switch button not lining up with the same function on roman keyboards, which I find annoying in practice. But not as annoying as not being able to type emoji. :) FWIW.
Hi Ralph. Unfortunately the emoji keyboard you mentioned is close to unusable (I posted yesterday about this in the mailing list). And I'd rather not derail this bug with alternatives. We should have all the pieces needed to ship an emoji keyboard with our own custom artwork. Let's stay focused on landing.
Thomas emailed me after my previous comment. To clarify the unusable symptoms of Thomas's keyboard are only present on my ZC3. It seems to be working well on the Flame.

Hopefully Thomas and I can get those issues worked out for the ZC3. It would still be nice for his keyboard to function well on that device as well.

Again, I'd like us to focus on landing an official FxOS emoji keyboard.
(In reply to Reza Akhavan [:jedireza] from comment #34)
> Again, I'd like us to focus on landing an official FxOS emoji keyboard.

Do ping me if you need help to test out what's in the code base currently.
Assignee: nobody → rakhavan
Patryk, Tim and I shared an email thread and we talked about hiding the recent emoji tab to simplify the scope and get this landed. We think it's reasonable to create a follow up bug for adding the recent emoji tab feature later.

I've started working on this. So far I've hid the recent emoji tab and added the emoji keyboard to the Makefile.

You can see the changes I've made so far on my `emoji-keyboard` branch:

https://github.com/mozilla-b2g/gaia/compare/master...jedireza:emoji-keyboard?expand=1

After getting the current keyboard and font on my device I noticed that some emoji are missing. You can see which one's here:

https://gist.github.com/jedireza/c78209d2b4554de5d59e#file-01-missing-md

Thank you Tim for helping me get the current keyboard installed on my device. I kept notes on the dev process so far. You can follow along here:

https://gist.github.com/jedireza/c78209d2b4554de5d59e#file-02-dev-notes-md

This is really close to be shippable. It seems that we just need to resolve the missing emoji. What other road blocks can/will we run into after these changes?
(In reply to Reza Akhavan [:jedireza] from comment #36)
> Patryk, Tim and I shared an email thread and we talked about hiding the
> recent emoji tab to simplify the scope and get this landed. We think it's
> reasonable to create a follow up bug for adding the recent emoji tab feature
> later.
> 
> I've started working on this. So far I've hid the recent emoji tab and added
> the emoji keyboard to the Makefile.
> 
> You can see the changes I've made so far on my `emoji-keyboard` branch:
> 
> https://github.com/mozilla-b2g/gaia/compare/master...jedireza:emoji-
> keyboard?expand=1
> 

Please simply remove the button definition instead of comment it out. 
To create the patch to enable it, remove this tag to make it ship by default instead

https://github.com/jedireza/gaia/blob/d42afee5f90839eb1fef37c88adadc5d6823c02a/apps/keyboard/js/layouts/emoji.js#L5-L8

keyboard_test.js would need some adjustment too.

> After getting the current keyboard and font on my device I noticed that some
> emoji are missing. You can see which one's here:
> 
> https://gist.github.com/jedireza/c78209d2b4554de5d59e#file-01-missing-md
> 
> Thank you Tim for helping me get the current keyboard installed on my
> device. I kept notes on the dev process so far. You can follow along here:
> 
> https://gist.github.com/jedireza/c78209d2b4554de5d59e#file-02-dev-notes-md
> 
> This is really close to be shippable. It seems that we just need to resolve
> the missing emoji. What other road blocks can/will we run into after these
> changes?

The missing smiley might be a font priority issue. We would need to tweak that in a Gecko pref instead but that might not be the best approach. We should loop :jfkthame if that's indeed the case.

Thank you for taking the initiative!
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #37)
> keyboard_test.js would need some adjustment too.

Don't worry about this test, it is corrected in bug 1184802.
Tim, I've made the changes you requested. PR updated:
https://github.com/mozilla-b2g/gaia/compare/master...jedireza:emoji-keyboard?expand=1

I emailed with Pavel about the missing emojis. It looks like we indeed are missing some and need some help getting the artwork finished. Pavel mentioned checking with Patryk, who IIRC is just getting back from vacation.

Looks like the only blocker right now is the missing emoji.
(In reply to Reza Akhavan [:jedireza] from comment #39)
> Tim, I've made the changes you requested. PR updated:
> https://github.com/mozilla-b2g/gaia/compare/master...jedireza:emoji-
> keyboard?expand=1

The patch is ready I think. Send me a pull request and I will r+ it.

> 
> I emailed with Pavel about the missing emojis. It looks like we indeed are
> missing some and need some help getting the artwork finished. Pavel
> mentioned checking with Patryk, who IIRC is just getting back from vacation.
> 
> Looks like the only blocker right now is the missing emoji.

As I said in the e-mail, I think that's a font precedence issue. You should attach the screenshots and ask :jfkthame on this, either on a new bug that would made the necessary change on Gecko/B2G or this bug.

Thanks!
Flags: needinfo?(rakhavan)
> The patch is ready I think. Send me a pull request and I will r+ it.

https://github.com/mozilla-b2g/gaia/pull/31263

> As I said in the e-mail, I think that's a font precedence issue. You should attach the screenshots and ask :jfkthame on this, either on a new bug that would made the necessary change on Gecko/B2G or this bug.

I chatted with :pivanov and he's fixed a lot of the missing emoji. Currently it looks like some of the emoji codes exist in default font and the Emoji font is being used as a fallback to the default. I trust he'll work through that.

We're getting there. :)
Flags: needinfo?(rakhavan)
Attachment #8643924 - Flags: review?(timdream)
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

So we will land this only after the font issue is resolved right?
Attachment #8643924 - Flags: review?(timdream) → review+
> So we will land this only after the font issue is resolved right?

Yes, I was going to wait for the font issues to be resolved before adding the checkin-needed flag.
Hey Guys, 

I fixed the Emoji font ... but I found an issue when we use it for example in SMS App.
If we want to use Emoji Icons instead the default Unicode icon versions of default font (https://en.wikipedia.org/wiki/Emoji#ref_U2700_as_of_Unicode_version) we need to set font-family: "FirefoxEmoji", sans-serif; to element who display the emojis.

We can set here https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/style/composer.css#L23
font-family: "FirefoxEmoji", sans-serif; and the message input element will show the correct emojis and text. But we need to do this in few more places in SMS App and if we want to copy/paste the icon in other app or just use Emoji Keyboard in some other app we need to be sure that the icons will be displayed from our Emoji font not from the default one.

Can we do something better/clever? for example ... what if we set FirefoxEmoji to be the default font for gaia and Fira to be second one in the list? Something like font-family: "FirefoxEmoji", "Fira Sans", sans-serif;

Hope that my explanation is not a mess.

Tim? Reza? Any thoughts?
Flags: needinfo?(timdream)
Flags: needinfo?(rakhavan)
(In reply to Pavel Ivanov [:ivanovpavel][:pivanov] from comment #45)
> We can set here
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/style/
> composer.css#L23
> font-family: "FirefoxEmoji", sans-serif; and the message input element will
> show the correct emojis and text. But we need to do this in few more places
> in SMS App and if we want to copy/paste the icon in other app or just use
> Emoji Keyboard in some other app we need to be sure that the icons will be
> displayed from our Emoji font not from the default one.
> 
> Can we do something better/clever? for example ... what if we set
> FirefoxEmoji to be the default font for gaia and Fira to be second one in
> the list? Something like font-family: "FirefoxEmoji", "Fira Sans",
> sans-serif;

I think we should just make the Emoji as the default font, if there is no compatibility concern.

BTW, why was the font named "FirefoxEmoji" and not "Firefox Emoji" or any other special name of it's own right? We should not be exposing the internal working name to the world.

Johnathan, I couldn't help but notice we do not put Apple Color Emoji on Mac as a default font (as opposed to Safari). Should we do that on B2G? Thanks.
Flags: needinfo?(timdream) → needinfo?(jfkthame)
I don't think we should make FirefoxEmoji the default or first font for content in general, just so it'll get used for any emoji codepoints that happen to be present..... this will hurt performance globally, because for the vast majority of content we'll be first testing a font that doesn't support the characters in the text, and only second falling back to a font that's actually usable.

What we probably want to do instead is to return FirefoxEmoji from GetCommonFallbackFonts for the appropriate codepoint ranges, so that when an emoji codepoint occurs in the midst of content styled with a standard (text) font, we'll pick the desired fallback. This is what we do for Apple Color Emoji, for example (in gfxPlatformMac::GetCommonFallbackFonts), and for Segoe UI Emoji (in gfx::WindowsGetCommonFallbackFonts).

Currently, AFAIK the implementation in gfxAndroidPlatform is shared between Android and B2G, but we do have some #ifdef'd code in there that distinguishes between MOZ_WIDGET_ANDROID and MOZ_WIDGET_GONK. So I think it'd make sense to add a B2G-specific fragment in GetCommonFallbackFonts there and make it prefer the FirefoxEmoji font.

It looks like this bug is about implementing the keyboard layout, so I'd suggest filing a new bug (in Core::Graphics:Text) for the Gecko change needed here. It should be a pretty simple patch.
Flags: needinfo?(jfkthame)
Hey Jonathan,

here is the Core::Graphics:Text bug 1193481.
Can you help with the bug summary if is not correct?

Tim: I will change the font name to "Firefox Emoji".
Reza: I will fix few glyphs in to the font but I think after bug 1193481 we are ready to land this one.
No longer depends on: 1193481
Flags: needinfo?(jfkthame)
Flags: needinfo?(rakhavan)
Hey Reza,

Patryk has few comments which I think are reasonable can we fix them? Here are the comments:

1. Swiping and tapping on an emoji both cause vibration feedback. Swiping should not do this. On swipe = no vibration.

2. When you have the emoji keyboard enabled in the main keyboard screen the “EN” button should show a the smiley emoji. When in the emoji keyboard to get back to english keyboard we should be using the “EN” graphic and not a smiley emoji. So basically the “EN” and the “smiley emoji” need to be reversed.

Thanks in advance :)
Flags: needinfo?(rakhavan)
I'm on it.
Flags: needinfo?(rakhavan)
> 1. Swiping and tapping on an emoji both cause vibration feedback. Swiping should not do this. On swipe = no vibration.

I started looking into this but haven't made any progress yet.

> 2. When you have the emoji keyboard enabled in the main keyboard screen the “EN” button should show a the smiley emoji. When in the emoji keyboard to get back to english keyboard we should be using the “EN” graphic and not a smiley emoji. So basically the “EN” and the “smiley emoji” need to be reversed.

I'm not sure we should change this or if we do, not include it as a blocker for this bug. The reason I say that is because it's how the keyboard works today. For example on my Foxfooding device I used 3 keyboards; English, Erabic and Emoji (from the marketplace). When I choose a keyboard I see "En" when I'm on English keyboard, "Ar" when I'm on the Arabic keyboard and a Smiley when I'm on the emoji keyboard. In this case it wouldn't be a simple toggle between two keyboards.
Patryk, thoughts?
Flags: needinfo?(padamczyk)
(In reply to Reza Akhavan [:jedireza] from comment #52)

> > 2. When you have the emoji keyboard enabled in the main keyboard screen the “EN” button should show a the smiley emoji. When in the emoji keyboard to get back to english keyboard we should be using the “EN” graphic and not a smiley emoji. So basically the “EN” and the “smiley emoji” need to be reversed.
> 
> I'm not sure we should change this...

Right; I don't think we should do that. This symbol indicates the *current* keyboard layout, not the layout that tapping it will switch to. The suggested change would be quite confusing for users with multiple keyboards enabled.
Flags: needinfo?(jfkthame)
For #2, spoke with interaction designers, and let's keep it as is.
Flags: needinfo?(padamczyk)
> 1. Swiping and tapping on an emoji both cause vibration feedback. Swiping should not do this. On swipe = no vibration.

Tim, some guidance on this would be much appreciated. I found `apps/keyboard/js/keyboard/feedback_manager.js` but finding it difficult to track back down exactly how feedback is triggered/controlled.

I've also noticed while using the keyboard with the vibration setting turned on, that it "feels" as if the vibration is happening immediately (like a touchstart) so I wonder how much will need to change to prevent this feedback when a swipe is detected.
Flags: needinfo?(timdream)
WebIDE and a break point pointing at [1] can give you a stack trace on where the vibrate is originated.

[1] https://github.com/mozilla-b2g/gaia/blob/e9d7c4e3ede5ca1559d/apps/keyboard/js/keyboard/feedback_manager.js#L49

My educated guess is that it come from the DefaultTargetHandler here [2]

[2] https://github.com/mozilla-b2g/gaia/blob/e9d7c4e3ede5ca1559d7375ac78a7f97d40dc414/apps/keyboard/js/keyboard/target_handlers.js#L17

This is pretty problematic because we do not know if the user intends to swipe or tap at the time of touchstart, but we always trigger feedbacks at touchstart. This applies not only to swipe-able Emoji layout but also candidate selection panel for Asian IMEs.

If we can't live with that we would have to change the UX:

1. Don't trigger feedback at all for keys on the swipe-able panels.
2. Trigger feedback at touchend instead.
Flags: needinfo?(timdream) → needinfo?(pivanov)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #57)
> 2. Trigger feedback at touchend instead.

Note that this will make people feel the keyboard is less responsive.
> This applies not only to swipe-able Emoji layout but also candidate selection panel for Asian IMEs.

Let's create a separate bug for handling keyboard feedback, specifically as it relates to swipes? It doesn't sound like vibration should block this bug.
(In reply to Reza Akhavan [:jedireza] from comment #59)
> > This applies not only to swipe-able Emoji layout but also candidate selection panel for Asian IMEs.
> 
> Let's create a separate bug for handling keyboard feedback, specifically as
> it relates to swipes? It doesn't sound like vibration should block this bug.

I agree.
Flags: needinfo?(pivanov) → needinfo?(padamczyk)
Flags: needinfo?(padamczyk)
[Tracking Requested - why for this release]: Brought up in foxfooding, duping foxfood bug to this one.
Keywords: foxfood
Duplicate of this bug: 1175338
Flags: needinfo?(pi)
What's the status of this bug?
Flags: needinfo?(rakhavan)
We seem to be in good shape. Pavel recently finished updating the font (bug 1207260).

Pavel, what other things are needed? Are there any other blockers?
Flags: needinfo?(rakhavan) → needinfo?(pivanov)
Just waiting to land bug 1206844 and we can land this one.
Flags: needinfo?(pivanov)
Summary: [Keyboard] To support Emoji in keyboard → [Keyboard] Emoji layout in keyboard
Applying the patch I am seeing red background from bug 1203455 on the Emoji layout, which is not intended.

https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/style/keyboard.css#L28

Please set a background color on the container on the swiping panel.
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Removing r+ per comment 66.
Attachment #8643924 - Flags: review+
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

PR updated: rebased from master and added the swipe panel background color.
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Tim there were some build integration tests that were failing. I updated the PR to fix them. Could you take a look at the last commit and let me know if the changes are reasonable.

https://github.com/jedireza/gaia/commit/d6d85e267bc5266a4ee17180d6c066d68a92e8e5
Flags: needinfo?(timdream)
For the two

assert.deepEqual(inputKeysInManifest.sort(),
-                       [].concat(layoutIds, 'number').sort());
+                       [].concat(layoutIds, 'number', 'emoji').sort());

You chould just find the |var layoutIds| of each test and add |emoji| into the array.

Otherwise the patch looks fine.
Flags: needinfo?(timdream)
Please also rebase on top of bug 1216409.
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Rebased off of master from a few moments ago. Also applied the VS16 to BMP conversion. Thanks for the script Tim.
Pavel, the see comment I leave on the pull request. I have problems with some of the symbols:

-- some of them are added to the Emoji layout but they don't exist in the font, please remove them from the layout or update the font file again, or both (as the number cases have shown).
-- the flag symbols are placed on the wrong code points.

Please refer to bug 1216399 comment 4 on how BMP code points works with Emoji and non-Emoji. I asked Reza to remove the CSS style to reveal our problem here. We should not be using CSS to target Emoji rendering but to specify what to show at Unicode level.
Flags: needinfo?(pivanov)
FYI I just found the authoritative documents.

http://www.unicode.org/Public/emoji/1.0//emoji-data.txt

This is the list of Emoji.

http://unicode.org/Public/UCD/latest/ucd/StandardizedVariants.html

and this is list of standardized variants which append an VS16 should convert the boring symbol into Emoji. We should always make sure black white symbols on the Emoji layout comes with an VS16 (and only show Emoji when VS16 exists).

I don't think what bug 1216409 do is entirely correct in this case ... we probably don't need VS16 for some BMP characters and we probably need VS16 for some non-BMP characters. This document should give us the standard guideline.
There is a ordering chart that could help us design the layout.

http://unicode.org/emoji/charts/emoji-ordering.html

(these charts are published at http://unicode.org/emoji/charts/index.html )
Flags: needinfo?(pivanov)
Pavel, what's the update on comment 74? I don't see pull request being updated nor an updated Emoji font file.
Flags: needinfo?(pivanov)
Work on in ... I will update the PR tonight and I will ping Reza about it
Flags: needinfo?(pivanov)
Thing that we can ask for r+ (bug 1221419 will polish the Emoji font) Reza if you are OK :) ask for r+
Flags: needinfo?(rakhavan)
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Recently rebased with master to ensure treeherder stayed green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8a0bc740e643cc8d22382d8d7359bed98e1ff397

Tim, I also edited the commit messages of our PR to include the bug id so we could keep both Pavel's and my commit history as is without squashing.
Flags: needinfo?(rakhavan)
Attachment #8643924 - Flags: review?(timdream)
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Thanks for updating the commit comments.

I am seeing bug 1216409 is being reverted :'(. I don't see any VS16 is kept in the file (or, it's not visible to me because the current patch does not use escape sequences).

Please check the table at comment 75 and make sure all BMP Emoji codepoint that needs an VS16 has one with it. We need to ensure key label and keyboard output contains VS16 so Emoji don't accidentally turn black and while anywhere.
Attachment #8643924 - Flags: review?(timdream)
I am pretty sure the current patch stripped all VS16 -- I am seeing black and white Emoji just by looking at [1] in Safari.

[1] https://raw.githubusercontent.com/jedireza/gaia/emoji-keyboard/apps/keyboard/js/layouts/emoji.js

If there are VS16 then Safari will pick Color Emoji in this unstyled raw text but there isn't.
They have VS16 but I will double check them again.
(In reply to Pavel Ivanov [:ivanovpavel][:pivanov] UX from comment #83)
> They have VS16 but I will double check them again.

It would be easier to check if you use escape sequences.
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Tim, Pavel added a patch for the VS16. We're ready for another review.

Also recently rebased and tests are all clear:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=0ad16fc3da360aa8dff2348c198ba7a4fdbe660f
Attachment #8643924 - Flags: review?(timdream)
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

I did not load the new font and the patch into the phone. Instead I look at 

https://mozilla.github.io/fxemoji/dist/FirefoxEmoji/index.html

and do spot check if there are symbols missing (on things I have noted on the last review).

1) I don't see U+2122 (the TM sign) in the page.
https://github.com/mozilla-b2g/gaia/pull/31263#discussion_r42580168
2) I am seeing U+1F35 here, which should have been U+1F35E (the "bread" Emoji) on the page. The page indicates it is being encoded in the wrong place too.
https://github.com/mozilla-b2g/gaia/pull/31263#discussion_r44501269
3) According to http://unicode.org/Public/UCD/latest/ucd/StandardizedVariants.html , some of the non-BMP chars need VS16 too because they default to text style
https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502077
https://github.com/mozilla-b2g/gaia/pull/31263/files#r44501970
https://github.com/mozilla-b2g/gaia/pull/31263/files#r44501992
https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502008
https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502033

Set this to r+ once both changes are corrected.

(1) can be corrected by removing TM sign from the Emoji layout for now or actually update the font.
(2) needs to update the font *and* update the layout.
(3) needs to update the layout even though it might not be visible on the browser.

It might be worthy to check 

http://www.unicode.org/Public/emoji/1.0//emoji-data.txt

and ensure anything default to text style are appended with VS16, but I think adding VS16 to BMP code points & (3) should be enough.

(I did not, for example, say U+1F441 ("EYE") should followed with a VS16, even though it is marked as text-default in the data chart. We might want to follow that if we want to.)

(The exact guideline is illustrated here and explained by the accompany paragraph http://unicode.org/reports/tr51/index.html#Emoji_vs_Text_Display )


I am sorry this took longer than we think it would be, but it is important the layout output the right codepoints to ensure interoperability across different devices.
Attachment #8643924 - Flags: review?(timdream) → review+
Comment on attachment 8643924 [details] [review]
[gaia] jedireza:emoji-keyboard > mozilla-b2g:master

Tim, Pavel added a new commit to the PR that I assume addresses these issue. No worries on it taking longer than expected, getting it done right is important.

Pavel, if you think the changes address Tim's feedback (above), let's mark this as review? again.
Flags: needinfo?(pivanov)
Yeah it will be cool if Tim can take a look at these changes again.
Tim can you check it again please :)
Flags: needinfo?(pivanov) → needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (OOO Nov 16-26; please needinfo) from comment #86)
> 3) According to
> http://unicode.org/Public/UCD/latest/ucd/StandardizedVariants.html , some of
> the non-BMP chars need VS16 too because they default to text style
> https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502077
> https://github.com/mozilla-b2g/gaia/pull/31263/files#r44501970
> https://github.com/mozilla-b2g/gaia/pull/31263/files#r44501992
> https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502008
> https://github.com/mozilla-b2g/gaia/pull/31263/files#r44502033
> 

Not every icons flagged by these comments need a VS16, but according to the spec [1] there seems to be no harm marking *everything* with VS16 anyway so we should land this.

[1]  http://unicode.org/reports/tr51/index.html#Emoji_vs_Text_Display
Flags: needinfo?(timdream)
Thanks Tim :)

Reza: It's your turn :)
Thanks for the help to all of you :)
Woohoo! On master:
https://github.com/mozilla-b2g/gaia/commit/e8c15ae4e5324a210000ee0a869a962aa542009f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Please try to squash work like this in the future to a single commit (or fewer) if possible. Commits should be atomic in nature, so a commit like `updated build integration tests` is not ideal, and should be squashed into the commit which changed the functionality of the tests. You can squash future work with `git rebase`. This is part of the requirements to land code into gaia.

This feature is awesome though, thanks for working on this!
(In reply to Kevin Grandon :kgrandon from comment #92)
> Please try to squash work like this in the future to a single commit

Yes, we discussed and compromised on this since there we a mixed set of commits from Pavel and myself. Prefixing the commits with the bug number was the middle ground. Now after looking back at them, I could've done a better job at grouping them to reduce the number of commits.

> This feature is awesome though, thanks for working on this!

;) thank you
You need to log in before you can comment on or make changes to this bug.