Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: trying to use emojis with an uppercase letter breaks the message field #3560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohnXLivingston
Copy link
Contributor

@JohnXLivingston JohnXLivingston commented Dec 5, 2024

Previoux fix (#3501) was wrong and introduced other issues:

  • impossible to define an emoji with uppercase letters
  • possible to use an emoji 🔤 by using :Abc:, but it won't display

So, the correct fix is just to remove the 'i' modifier for the shortname_regex, to always have case sensitive emojis.

More details about this PR

Before my previous fix (#3501), emojis were broken if you tried to call them without beeing case sensitive.

Consider we have a ":abc:" short_name.
The shortname_regex was case insensitive, so it detected for example ":ABC:". But then, the following line was broken:

const cp = converse.emojis.by_sn[ref[0]].cp;

by_sn[ref[0]] was undefined, and the .cp make it fail.

Same here:

return refs.length === 1 && (text === refs[0]['shortname'] || text === refs[0]['emoji']);

text === refs[0]['shortname'] is case sensitive.

So, i added the text.toLowerCase()... But this is assuming that shortnames are lowercase... When you deal with custom emojis, nothing tells you have to use lowerCase.
(And .toLowerCase() may be inexact for some alphabets).

Moreover, this was fixing the message sending... But the message display was broken. There was a missing toLowerCase() here:

const { url } = converse.emojis.by_sn[shortname];

To resume:

  • v10 is broken. If you call ":ABC:", Converse was totally broken, and you need to reload the page. And when there is such message in the chat, it was broken too if a remember well.
  • my previous fix was only working if all shortnames are lowercase. It appears that in my project, some users decided to use uppercase for some custom emojis (can't blame them for that).

Trying to handle case insensivity would be difficult... And could cause some troubles if there are multiple emojis with are equals when case insensitive... (in the JSON object, you can have ":ABC" and ":abc"...).

So i think it is better to make shortnames case sensitive. In most case, it will just work.
(the only code part that was trying to make emojis shortnames case insensitive is the i modifier on the regex this PR changes)

The only issue i can see, is if you have some custom emoji without the ":" prefix, and when you use a mobile phone that will automatically use an uppercase for the first letter of a new sentence. This seems a reasonable assumption.

… field.

Previoux fix (conversejs#3501) was
wrong and introduced other issues:
* impossible to define an emoji with uppercase letters
* possible to use an emoji 🔤 by using :Abc:, but it won't display

So, the correct fix is just to remove the 'i' modifier for the
shortname_regex, to always have case sensitive emojis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant