From 41ef3bc4383a83b051ebe1c94318ced9958fa6b5 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 24 May 2022 23:51:36 +0100 Subject: [PATCH 1/4] Write tests around composer badly handling gendered facepalm emoji --- test/editor/parts-test.ts | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 test/editor/parts-test.ts diff --git a/test/editor/parts-test.ts b/test/editor/parts-test.ts new file mode 100644 index 00000000000..b77971c2aa7 --- /dev/null +++ b/test/editor/parts-test.ts @@ -0,0 +1,35 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { EmojiPart, PlainPart } from "../../src/editor/parts"; + +describe("editor/parts", () => { + describe("appendUntilRejected", () => { + const femaleFacepalmEmoji = "🤦‍♀️"; + + it("should not accept emoji strings into type=plain", () => { + const part = new PlainPart(); + expect(part.appendUntilRejected(femaleFacepalmEmoji, "")).toEqual(femaleFacepalmEmoji); + expect(part.text).toEqual(""); + }); + + it("should accept emoji strings into type=emoji", () => { + const part = new EmojiPart(); + expect(part.appendUntilRejected(femaleFacepalmEmoji, "")).toBeUndefined(); + expect(part.text).toEqual(femaleFacepalmEmoji); + }); + }); +}); From db068cd08956cbb40bbfa014575d0ad2b7de2f98 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 24 May 2022 23:57:29 +0100 Subject: [PATCH 2/4] Commit export for tests to be happy --- src/editor/parts.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/editor/parts.ts b/src/editor/parts.ts index 460a74ca86d..ec5e9a84cd6 100644 --- a/src/editor/parts.ts +++ b/src/editor/parts.ts @@ -361,7 +361,7 @@ class NewlinePart extends BasePart implements IBasePart { } } -class EmojiPart extends BasePart implements IBasePart { +export class EmojiPart extends BasePart implements IBasePart { protected acceptsInsertion(chr: string, offset: number): boolean { return EMOJIBASE_REGEX.test(chr); } From 7afa8bda9074cf13e381cd4dca7e8b6adcc2a025 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 25 May 2022 00:04:55 +0100 Subject: [PATCH 3/4] Fix edge case around composer handling gendered facepalm emoji --- src/editor/parts.ts | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/editor/parts.ts b/src/editor/parts.ts index ec5e9a84cd6..d250c79827d 100644 --- a/src/editor/parts.ts +++ b/src/editor/parts.ts @@ -93,6 +93,7 @@ abstract class BasePart { this._text = text; } + // chr can also be a grapheme cluster protected acceptsInsertion(chr: string, offset: number, inputType: string): boolean { return true; } @@ -128,14 +129,18 @@ abstract class BasePart { // append str, returns the remaining string if a character was rejected. public appendUntilRejected(str: string, inputType: string): string | undefined { const offset = this.text.length; - for (let i = 0; i < str.length; ++i) { - const chr = str.charAt(i); - if (!this.acceptsInsertion(chr, offset + i, inputType)) { - this._text = this._text + str.slice(0, i); - return str.slice(i); - } - } - this._text = this._text + str; + // Take a copy as we will be taking chunks off the start of the string as we process them + // To only need to grapheme split the bits of the string we're working on. + let buffer = str; + let char = ""; + do { + buffer = buffer.slice(char.length); + // We use lodash's grapheme splitter to avoid breaking apart compound emojis + char = split(buffer, "", 2)[0]; + } while (char && this.acceptsInsertion(char, offset + (str.length - buffer.length - char.length), inputType)); + + this._text += str.slice(0, str.length - buffer.length); + return buffer || undefined; } // inserts str at offset if all the characters in str were accepted, otherwise don't do anything @@ -555,7 +560,8 @@ export class PartCreator { case "\n": return new NewlinePart(); default: - if (EMOJIBASE_REGEX.test(input[0])) { + // We use lodash's grapheme splitter to avoid breaking apart compound emojis + if (EMOJIBASE_REGEX.test(split(input, "", 2)[0])) { return new EmojiPart(); } return new PlainPart(); From 07bdcc404a7c1c95b5ae093bda303be9af924fa7 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 25 May 2022 08:35:21 +0100 Subject: [PATCH 4/4] Fix offset calculations and make code more readable --- src/editor/parts.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/editor/parts.ts b/src/editor/parts.ts index d250c79827d..8d98c398b54 100644 --- a/src/editor/parts.ts +++ b/src/editor/parts.ts @@ -132,12 +132,14 @@ abstract class BasePart { // Take a copy as we will be taking chunks off the start of the string as we process them // To only need to grapheme split the bits of the string we're working on. let buffer = str; - let char = ""; - do { - buffer = buffer.slice(char.length); + while (buffer) { // We use lodash's grapheme splitter to avoid breaking apart compound emojis - char = split(buffer, "", 2)[0]; - } while (char && this.acceptsInsertion(char, offset + (str.length - buffer.length - char.length), inputType)); + const [char] = split(buffer, "", 2); + if (!this.acceptsInsertion(char, offset + str.length - buffer.length, inputType)) { + break; + } + buffer = buffer.slice(char.length); + } this._text += str.slice(0, str.length - buffer.length); return buffer || undefined;