1638874 - Adding any emoji in the compose window throws an error

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Khushil Mistry [:khushil324]]

error thrown in doStatefulCommand: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsICommandParams.getBooleanValue]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/messengercompose/ComposerCommands.js :: pokeMultiStateUI :: line 396" data: no]

Comment 1

[Khushil Mistry [:khushil324]]

ESR 68 is also affected.

Comment 2

[Wayne Mery (:wsmwk)]

regression?

Comment 3

[Magnus Melin [:mkmelin]]

Don't know if it's this one, but I there are at bunch a few commands there that cause an error (a caught one).

Comment 4

[Khushil Mistry [:khushil324]]

I will take this as I am working on the commands part in the another patch.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: WIP-Bug-1638874_improve-emoji-support.patch

Apply patch from bug 1582410 before.

In the current implementation, we are inserting emoji as content to <span> and inserting another <span> for the text code of the smiley.

<span> // This will display content as emoji
  <span>:-)</span> // This block will be hidden through CSS.
</span>

When you try to remove this, it will need 5 keypresses to remove. First, :-) will be removed with 3 keypresses. And then, 2 <span> elements will be removed with another 2 keypresses. And we can't do much about it.

So, in this patch, I have tried moving from this implementation to like this: https://www.w3schools.com/charsets/ref_emoji_smileys.asp, inserting it through hexadecimal value: 😀 as now all browsers and editor support this.

This implementation is easy to use and there is no problem while adding or removing them. It will behave just like a character. I have submitted a WIP patch. Look at it. If everyone agrees I will go and remove all the previous smiley related images and CSS. This is also a question, do we want to keep those images and CSS to see the emoji images in previously sent/received mail? If we remove those, they will be shown text codes for those emojis only like :-) etc.

Comment 6

[Ian Neal]

Just a quick query, what about plain text messages?

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9150964 [details] [diff] [review]
WIP-Bug-1638874_improve-emoji-support.patch

Review of attachment 9150964 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/ComposerCommands.js
@@ +2550,4 @@
>      try {
> +      let editor = GetCurrentEditor();
> +      let smileyCode = aParams.getStringValue("state_attribute");
> +      let smileyElement = editor.createElementWithDefaults("span");

shouldn't it be just a text node?

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Ian Neal from comment #6)

Just a quick query, what about plain text messages?

I don't think you can insert a (formatted) smiley in a plan text message.
We do auto detection to format smileys for incoming messages, but this bug is about inserting smileys in HTML messages.

I think we still need these images (at least for now) for the incoming plain text auto-upgrade case.

Comment 9

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #8)

(In reply to Ian Neal from comment #6)

Just a quick query, what about plain text messages?

I don't think you can insert a (formatted) smiley in a plan text message.
We do auto detection to format smileys for incoming messages, but this bug is about inserting smileys in HTML messages.

Right. Displaying smileys for the incoming messages also has some issues, it is not detecting in all the cases. That can be done in another bug.

I think we still need these images (at least for now) for the incoming plain text auto-upgrade case.

Yes, I wanted to know if we want to go on this path and what to do about the current implementation.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1638874_improve-emoji-support-0.patch

We need to insert the textNode through <span> element. Throws an error when you try to inject the textNode directly.

Comment 11

[Magnus Melin [:mkmelin]]

I think this is a good direction.
I'm not sure why the emoticons are all listed with yellow background in the menu, but the inserted ones are only sometimes with yellow... and some cases, like embarrassed therefore look slightly different in the compose body.

Comment 12

[Khushil Mistry [:khushil324]]

I am not seeing any yellow background anywhere, on Mac. Can you share a screenshot?

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: smile.png

I meant fill, not background. Notice how they look different in the two places.

Comment 14

[Khushil Mistry [:khushil324]]

I think this is a Linux related issue: https://askubuntu.com/questions/1029661/18-04-color-emoji-not-showing-up-at-all-in-chrome-only-partially-in-firefox
If you sent the mail and look it at the message window, you will see all the emojis in yellow background.
Do you have any idea and how to solve this?

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1638874_improve-emoji-support-1.patch

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9155220 [details] [diff] [review]
Bug-1638874_improve-emoji-support-1.patch

Review of attachment 9155220 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't know what's up with the emoji colors: with "preformat" text they are all yellow, in the menu they are all yellow. Elsehere some are not yellow.

But I still think we should go ahead with this.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/389202b5f371
Adding any emoji in the compose window throws an error. r=mkmelin

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: Bug-1638874_improve-emoji-support-ESR78.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Emoji usage is problematic.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Comment 19

[Rob Lemley [:rjl]]

Comment on attachment 9155220 [details] [diff] [review]
Bug-1638874_improve-emoji-support-1.patch

[Triage Comment]
Required for bug 1665174. Taking the original version of the patch rather than the esr78 version to avoid confusion as the later is combined with bug 1665174.

Comment 20

[Rob Lemley [:rjl]]

2020-06-15 was milestone 79, which ran 2020-06-01 to 2020-06-29.

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/73bf69d94029