From c2b0507264638762dc5e5e207ec376a09abae4bc Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Wed, 10 May 2023 16:24:07 +0100 Subject: [PATCH 01/15] insert mentions as links styled as pills --- .../hooks/usePlainTextListeners.ts | 15 +++++-- .../wysiwyg_composer/hooks/useSuggestion.ts | 39 ++++++++++++------- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts index 8369d2684fb..c91248b9d18 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts @@ -83,11 +83,18 @@ export function usePlainTextListeners( }, [ref, onSend]); const setText = useCallback( - (text: string) => { - setContent(text); - onChange?.(text); + (text?: string) => { + if (text !== undefined) { + setContent(text); + onChange?.(text); + } else if (ref && ref.current) { + // if called with no argument, read the current innerHTML from the ref if it is non-null + const currentRefContent = ref.current.innerHTML; + setContent(currentRefContent); + onChange?.(currentRefContent); + } }, - [onChange], + [onChange, ref], ); // For separation of concerns, the suggestion handling is kept in a separate hook but is diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index e8d1833cc89..6bf5760effd 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -49,7 +49,7 @@ type SuggestionState = Suggestion | null; */ export function useSuggestion( editorRef: React.RefObject, - setText: (text: string) => void, + setText: (text?: string) => void, ): { handleMention: (href: string, displayName: string, attributes: Attributes) => void; handleCommand: (text: string) => void; @@ -153,19 +153,28 @@ export function processMention( const { node } = suggestionData; - const textBeforeReplacement = node.textContent?.slice(0, suggestionData.startOffset) ?? ""; - const textAfterReplacement = node.textContent?.slice(suggestionData.endOffset) ?? ""; + // create an element with the required attributes to allow us to display the mention as a pill + const link = document.createElement("a"); + link.setAttribute("href", href); + link.setAttribute("contenteditable", "false"); + Object.entries(attributes).forEach(([attr, value]) => link.setAttribute(attr, value ?? "")); + link.innerText = displayName; - // TODO replace this markdown style text insertion with a pill representation - const newText = `[${displayName}](<${href}>) `; - const newCursorOffset = textBeforeReplacement.length + newText.length; - const newContent = textBeforeReplacement + newText + textAfterReplacement; + // create a text node that will follow the inserted link (we may be inserting into the middle of a node) + const endNode = document.createTextNode(` ${node.textContent?.slice(suggestion.endOffset) ?? ""}`); - // insert the new text, move the cursor, set the text state, clear the suggestion state - node.textContent = newContent; - document.getSelection()?.setBaseAndExtent(node, newCursorOffset, node, newCursorOffset); - setText(newContent); - setSuggestionData(null); + // take the starting node, truncate the content from start to the startOffset of the suggestion + // and then append the link element followed by the text node + node.textContent = node.textContent?.slice(0, suggestion.startOffset) ?? ""; + node.parentNode?.appendChild(link); + node.parentNode?.appendChild(endNode); + + // move the selection to after the leading space in the text node following the link + document.getSelection()?.setBaseAndExtent(endNode, 1, endNode, 1); + + // set the text content to be the innerHTML of the current editor ref and clear the suggestion state + setText(); + setSuggestion(null); } /** @@ -179,9 +188,9 @@ export function processMention( */ export function processCommand( replacementText: string, - suggestionData: SuggestionState, - setSuggestionData: React.Dispatch>, - setText: (text: string) => void, + suggestion: SuggestionState, + setSuggestion: React.Dispatch>, + setText: (text?: string) => void, ): void { // if we do not have a suggestion, return early if (suggestionData === null) { From e6e229034b84b657123790177e8bcdc4f8acda65 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Wed, 10 May 2023 17:12:52 +0100 Subject: [PATCH 02/15] post merge fix and update test --- .../wysiwyg_composer/hooks/useSuggestion.ts | 16 ++++---- .../hooks/useSuggestion-test.tsx | 40 +++++++++++-------- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 6bf5760effd..4752f5568a0 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -144,7 +144,7 @@ export function processMention( attributes: Attributes, // these will be used when formatting the link as a pill suggestionData: SuggestionState, setSuggestionData: React.Dispatch>, - setText: (text: string) => void, + setText: (text?: string) => void, ): void { // if we do not have a suggestion, return early if (suggestionData === null) { @@ -157,15 +157,17 @@ export function processMention( const link = document.createElement("a"); link.setAttribute("href", href); link.setAttribute("contenteditable", "false"); - Object.entries(attributes).forEach(([attr, value]) => link.setAttribute(attr, value ?? "")); + Object.entries(attributes).forEach(([attr, value]) => value && link.setAttribute(attr, value)); link.innerText = displayName; // create a text node that will follow the inserted link (we may be inserting into the middle of a node) - const endNode = document.createTextNode(` ${node.textContent?.slice(suggestion.endOffset) ?? ""}`); + const endNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); // take the starting node, truncate the content from start to the startOffset of the suggestion // and then append the link element followed by the text node - node.textContent = node.textContent?.slice(0, suggestion.startOffset) ?? ""; + + // default to a zwsp if we'd have no preceding text + node.textContent = node.textContent?.slice(0, suggestionData.startOffset) || "\u200b"; node.parentNode?.appendChild(link); node.parentNode?.appendChild(endNode); @@ -174,7 +176,7 @@ export function processMention( // set the text content to be the innerHTML of the current editor ref and clear the suggestion state setText(); - setSuggestion(null); + setSuggestionData(null); } /** @@ -188,8 +190,8 @@ export function processMention( */ export function processCommand( replacementText: string, - suggestion: SuggestionState, - setSuggestion: React.Dispatch>, + suggestionData: SuggestionState, + setSuggestionData: React.Dispatch>, setText: (text?: string) => void, ): void { // if we do not have a suggestion, return early diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx index 568bc613466..b2dc531c049 100644 --- a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx @@ -78,34 +78,42 @@ describe("processMention", () => { expect(mockSetText).not.toHaveBeenCalled(); }); - it("can insert a mention into an empty text node", () => { - // make an empty text node, set the cursor inside it and then append to the document - const textNode = document.createTextNode(""); - document.body.appendChild(textNode); - document.getSelection()?.setBaseAndExtent(textNode, 0, textNode, 0); + it("can insert a mention into a text node", () => { + // make a text node and an editor div, set the cursor inside the text node and then + // append node to editor, then editor to document + const textNode = document.createTextNode("@a"); + const mockEditor = document.createElement("div"); + mockEditor.appendChild(textNode); + document.body.appendChild(mockEditor); + document.getSelection()?.setBaseAndExtent(textNode, 1, textNode, 1); // call the util function const href = "href"; const displayName = "displayName"; - const mockSetSuggestion = jest.fn(); + const mockSetSuggestionData = jest.fn(); const mockSetText = jest.fn(); processMention( href, displayName, - {}, - { node: textNode, startOffset: 0, endOffset: 0 } as unknown as Suggestion, - mockSetSuggestion, + { "data-test-attribute": "test" }, + { node: textNode, startOffset: 0, endOffset: 2 } as unknown as Suggestion, + mockSetSuggestionData, mockSetText, ); - // placeholder testing for the changed content - these tests will all be changed - // when the mention is inserted as an tagfs - const { textContent } = textNode; - expect(textContent!.includes(href)).toBe(true); - expect(textContent!.includes(displayName)).toBe(true); + // check that the editor has a single child + expect(mockEditor.children).toHaveLength(1); + const linkElement = mockEditor.firstElementChild as HTMLElement; - expect(mockSetText).toHaveBeenCalledWith(expect.stringContaining(displayName)); - expect(mockSetSuggestion).toHaveBeenCalledWith(null); + // and that the child is an tag with the expected attributes + expect(linkElement).toBeInstanceOf(HTMLAnchorElement); + expect(linkElement).toHaveAttribute(href, href); + expect(linkElement).toHaveAttribute("contenteditable", "false"); + expect(linkElement).toHaveAttribute("data-test-attribute", "test"); + expect(linkElement.innerText).toBe(displayName); + + expect(mockSetText).toHaveBeenCalledWith(); + expect(mockSetSuggestionData).toHaveBeenCalledWith(null); }); }); From ce170ed6cf2b2e37dd9b1445e189cc3fe1e32633 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 14:35:36 +0100 Subject: [PATCH 03/15] update comments, move typeguard out --- src/Typeguards.ts | 4 ++++ .../wysiwyg_composer/hooks/useSuggestion.ts | 18 ++++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/Typeguards.ts b/src/Typeguards.ts index 78869097c60..397cbbd29b1 100644 --- a/src/Typeguards.ts +++ b/src/Typeguards.ts @@ -17,3 +17,7 @@ limitations under the License. export function isNotNull(arg: T): arg is Exclude { return arg !== null; } + +export function isNotUndefined(arg: T): arg is Exclude { + return arg !== undefined; +} diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 4752f5568a0..2f28d22dab9 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -17,6 +17,8 @@ limitations under the License. import { Attributes, MappedSuggestion } from "@matrix-org/matrix-wysiwyg"; import { SyntheticEvent, useState } from "react"; +import { isNotUndefined } from "../../../../../Typeguards"; + /** * Information about the current state of the `useSuggestion` hook. */ @@ -153,25 +155,25 @@ export function processMention( const { node } = suggestionData; - // create an element with the required attributes to allow us to display the mention as a pill + // create an element with the required attributes to allow us to interpret the mention as being a pill const link = document.createElement("a"); link.setAttribute("href", href); link.setAttribute("contenteditable", "false"); - Object.entries(attributes).forEach(([attr, value]) => value && link.setAttribute(attr, value)); + Object.entries(attributes).forEach(([attr, value]) => isNotUndefined(value) && link.setAttribute(attr, value)); link.innerText = displayName; - // create a text node that will follow the inserted link (we may be inserting into the middle of a node) + // create a text node that will follow the inserted link (as we may be inserting into the middle of a node) const endNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); - // take the starting node, truncate the content from start to the startOffset of the suggestion - // and then append the link element followed by the text node - - // default to a zwsp if we'd have no preceding text + // now amend the current node text content to be only the text from start up to the suggestion startOffset + // (default to a zwsp if we'd have no preceding text to allow cursor positioning before a lone pill) node.textContent = node.textContent?.slice(0, suggestionData.startOffset) || "\u200b"; + + // then append the newly created link and the ending text node containing the space that follows the link node.parentNode?.appendChild(link); node.parentNode?.appendChild(endNode); - // move the selection to after the leading space in the text node following the link + // move the selection to after the space that follows the link document.getSelection()?.setBaseAndExtent(endNode, 1, endNode, 1); // set the text content to be the innerHTML of the current editor ref and clear the suggestion state From e8bb00e272d5c920eef8df568cff52650828258d Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 14:43:22 +0100 Subject: [PATCH 04/15] create a text node instead of setting innerText --- .../views/rooms/wysiwyg_composer/hooks/useSuggestion.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 2f28d22dab9..42bf2d41c7d 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -157,10 +157,11 @@ export function processMention( // create an element with the required attributes to allow us to interpret the mention as being a pill const link = document.createElement("a"); + const linkText = document.createTextNode(displayName); link.setAttribute("href", href); link.setAttribute("contenteditable", "false"); Object.entries(attributes).forEach(([attr, value]) => isNotUndefined(value) && link.setAttribute(attr, value)); - link.innerText = displayName; + link.appendChild(linkText); // create a text node that will follow the inserted link (as we may be inserting into the middle of a node) const endNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); From 9260c006cdf8a2c8f7496fdd03169aa442b6f942 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 14:43:34 +0100 Subject: [PATCH 05/15] update test --- .../hooks/useSuggestion-test.tsx | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx index b2dc531c049..401d22fc721 100644 --- a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx @@ -105,12 +105,24 @@ describe("processMention", () => { expect(mockEditor.children).toHaveLength(1); const linkElement = mockEditor.firstElementChild as HTMLElement; - // and that the child is an tag with the expected attributes + // and that the child is an tag with the expected attributes and content expect(linkElement).toBeInstanceOf(HTMLAnchorElement); expect(linkElement).toHaveAttribute(href, href); expect(linkElement).toHaveAttribute("contenteditable", "false"); expect(linkElement).toHaveAttribute("data-test-attribute", "test"); - expect(linkElement.innerText).toBe(displayName); + expect(mockEditor).toMatchInlineSnapshot(` +
+ ​ + + displayName + + +
+ `); expect(mockSetText).toHaveBeenCalledWith(); expect(mockSetSuggestionData).toHaveBeenCalledWith(null); From 8e2bf0dc735b2f4878b8c5071061f9084e31ac53 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 14:45:33 +0100 Subject: [PATCH 06/15] update test --- .../wysiwyg_composer/hooks/useSuggestion-test.tsx | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx index 401d22fc721..11f545ffda4 100644 --- a/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/hooks/useSuggestion-test.tsx @@ -110,19 +110,7 @@ describe("processMention", () => { expect(linkElement).toHaveAttribute(href, href); expect(linkElement).toHaveAttribute("contenteditable", "false"); expect(linkElement).toHaveAttribute("data-test-attribute", "test"); - expect(mockEditor).toMatchInlineSnapshot(` -
- ​ - - displayName - - -
- `); + expect(linkElement.textContent).toBe(displayName); expect(mockSetText).toHaveBeenCalledWith(); expect(mockSetSuggestionData).toHaveBeenCalledWith(null); From 72917f91089bac30d2d2fa946159c9d7b11c441a Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 15:37:07 +0100 Subject: [PATCH 07/15] fix broken cypress test, remove .only --- cypress/e2e/composer/composer.spec.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cypress/e2e/composer/composer.spec.ts b/cypress/e2e/composer/composer.spec.ts index 1013885b3d7..2b49b5e32e0 100644 --- a/cypress/e2e/composer/composer.spec.ts +++ b/cypress/e2e/composer/composer.spec.ts @@ -225,9 +225,10 @@ describe("Composer", () => { }); // ...inserts the username into the composer cy.findByRole("textbox").within(() => { - // TODO update this test when the mentions are inserted as pills, instead - // of as text - cy.findByText(otherUserName, { exact: false }).should("exist"); + cy.findByText(otherUserName, { exact: false }) + .should("exist") + .should("have.attr", "contenteditable", "false") + .should("have.attr", "data-mention-type", "user"); }); // Send the message to clear the composer @@ -250,9 +251,10 @@ describe("Composer", () => { // Selecting the autocomplete option using Enter inserts it into the composer cy.findByRole("textbox").type(`{Enter}`); cy.findByRole("textbox").within(() => { - // TODO update this test when the mentions are inserted as pills, instead - // of as text - cy.findByText(otherUserName, { exact: false }).should("exist"); + cy.findByText(otherUserName, { exact: false }) + .should("exist") + .should("have.attr", "contenteditable", "false") + .should("have.attr", "data-mention-type", "user"); }); }); }); From 9fbbf2facad8f5c3462ca533a34e2630aea59668 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Thu, 11 May 2023 18:10:45 +0100 Subject: [PATCH 08/15] make it able to deal with inserting in middle of blank lines --- .../wysiwyg_composer/hooks/useSuggestion.ts | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 42bf2d41c7d..7c1abeedae7 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -156,26 +156,28 @@ export function processMention( const { node } = suggestionData; // create an element with the required attributes to allow us to interpret the mention as being a pill - const link = document.createElement("a"); - const linkText = document.createTextNode(displayName); - link.setAttribute("href", href); - link.setAttribute("contenteditable", "false"); - Object.entries(attributes).forEach(([attr, value]) => isNotUndefined(value) && link.setAttribute(attr, value)); - link.appendChild(linkText); - - // create a text node that will follow the inserted link (as we may be inserting into the middle of a node) - const endNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); - - // now amend the current node text content to be only the text from start up to the suggestion startOffset - // (default to a zwsp if we'd have no preceding text to allow cursor positioning before a lone pill) - node.textContent = node.textContent?.slice(0, suggestionData.startOffset) || "\u200b"; - - // then append the newly created link and the ending text node containing the space that follows the link - node.parentNode?.appendChild(link); - node.parentNode?.appendChild(endNode); - - // move the selection to after the space that follows the link - document.getSelection()?.setBaseAndExtent(endNode, 1, endNode, 1); + const linkElement = document.createElement("a"); + const linkTextNode = document.createTextNode(displayName); + linkElement.setAttribute("href", href); + linkElement.setAttribute("contenteditable", "false"); + Object.entries(attributes).forEach( + ([attr, value]) => isNotUndefined(value) && linkElement.setAttribute(attr, value), + ); + linkElement.appendChild(linkTextNode); + + // create text nodes to go before and after the link + const leadingTextNode = document.createTextNode(node.textContent?.slice(0, suggestionData.startOffset) || "\u200b"); + const trailingTextNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); + + // now add the leading node, link element and trailing node before removing the node we are replacing + const parentNode = node.parentNode; + parentNode.insertBefore(leadingTextNode, node); + parentNode.insertBefore(linkElement, node); + parentNode.insertBefore(trailingTextNode, node); + parentNode.removeChild(node); + + // move the selection to the trailing text node + document.getSelection()?.setBaseAndExtent(trailingTextNode, 1, trailingTextNode, 1); // set the text content to be the innerHTML of the current editor ref and clear the suggestion state setText(); From ba1c47fbce7bf13c64cc19c9b6a9b4962b4ceeef Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 08:49:25 +0100 Subject: [PATCH 09/15] update comment --- .../views/rooms/wysiwyg_composer/hooks/useSuggestion.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 7c1abeedae7..7cb460e1a9c 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -169,7 +169,7 @@ export function processMention( const leadingTextNode = document.createTextNode(node.textContent?.slice(0, suggestionData.startOffset) || "\u200b"); const trailingTextNode = document.createTextNode(` ${node.textContent?.slice(suggestionData.endOffset) ?? ""}`); - // now add the leading node, link element and trailing node before removing the node we are replacing + // now add the leading text node, link element and trailing text node before removing the node we are replacing const parentNode = node.parentNode; parentNode.insertBefore(leadingTextNode, node); parentNode.insertBefore(linkElement, node); From cbfe5de4278de03659784e6080fe305073eea36b Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 08:50:07 +0100 Subject: [PATCH 10/15] fix strict null error --- .../rooms/wysiwyg_composer/hooks/useSuggestion.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts index 7cb460e1a9c..3e514f9e27d 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/useSuggestion.ts @@ -17,7 +17,7 @@ limitations under the License. import { Attributes, MappedSuggestion } from "@matrix-org/matrix-wysiwyg"; import { SyntheticEvent, useState } from "react"; -import { isNotUndefined } from "../../../../../Typeguards"; +import { isNotNull, isNotUndefined } from "../../../../../Typeguards"; /** * Information about the current state of the `useSuggestion` hook. @@ -171,10 +171,12 @@ export function processMention( // now add the leading text node, link element and trailing text node before removing the node we are replacing const parentNode = node.parentNode; - parentNode.insertBefore(leadingTextNode, node); - parentNode.insertBefore(linkElement, node); - parentNode.insertBefore(trailingTextNode, node); - parentNode.removeChild(node); + if (isNotNull(parentNode)) { + parentNode.insertBefore(leadingTextNode, node); + parentNode.insertBefore(linkElement, node); + parentNode.insertBefore(trailingTextNode, node); + parentNode.removeChild(node); + } // move the selection to the trailing text node document.getSelection()?.setBaseAndExtent(trailingTextNode, 1, trailingTextNode, 1); From 2b95c57154a30f350e143c9b456a242050545fa9 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 08:55:59 +0100 Subject: [PATCH 11/15] use typeguard --- .../rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts index c91248b9d18..054675e1081 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts @@ -22,6 +22,7 @@ import { IS_MAC, Key } from "../../../../../Keyboard"; import Autocomplete from "../../Autocomplete"; import { handleEventWithAutocomplete } from "./utils"; import { useSuggestion } from "./useSuggestion"; +import { isNotUndefined } from "../../../../../Typeguards"; function isDivElement(target: EventTarget): target is HTMLDivElement { return target instanceof HTMLDivElement; @@ -84,7 +85,7 @@ export function usePlainTextListeners( const setText = useCallback( (text?: string) => { - if (text !== undefined) { + if (isNotUndefined(text)) { setContent(text); onChange?.(text); } else if (ref && ref.current) { From 6499042f61d40d40ddecd9318df8e15a81a18b62 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 08:57:27 +0100 Subject: [PATCH 12/15] avoid implicit truth check --- .../rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts index 054675e1081..93e51138bf6 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts @@ -22,7 +22,7 @@ import { IS_MAC, Key } from "../../../../../Keyboard"; import Autocomplete from "../../Autocomplete"; import { handleEventWithAutocomplete } from "./utils"; import { useSuggestion } from "./useSuggestion"; -import { isNotUndefined } from "../../../../../Typeguards"; +import { isNotNull, isNotUndefined } from "../../../../../Typeguards"; function isDivElement(target: EventTarget): target is HTMLDivElement { return target instanceof HTMLDivElement; @@ -88,8 +88,8 @@ export function usePlainTextListeners( if (isNotUndefined(text)) { setContent(text); onChange?.(text); - } else if (ref && ref.current) { - // if called with no argument, read the current innerHTML from the ref if it is non-null + } else if (isNotNull(ref) && isNotNull(ref.current)) { + // if called with no argument, read the current innerHTML from the ref const currentRefContent = ref.current.innerHTML; setContent(currentRefContent); onChange?.(currentRefContent); From e5bd2da7cdf95b2116a1c5cc1e7f98433f85cae7 Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 10:17:09 +0100 Subject: [PATCH 13/15] add hook tests --- .../hooks/usePlainTextListeners.ts | 2 +- .../hooks/usePlainTextListeners-test.tsx | 66 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx diff --git a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts index 93e51138bf6..2bccfc444ae 100644 --- a/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts +++ b/src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners.ts @@ -66,7 +66,7 @@ export function usePlainTextListeners( onInput(event: SyntheticEvent): void; onPaste(event: SyntheticEvent): void; onKeyDown(event: KeyboardEvent): void; - setContent(text: string): void; + setContent(text?: string): void; handleMention: (link: string, text: string, attributes: Attributes) => void; handleCommand: (text: string) => void; onSelect: (event: SyntheticEvent) => void; diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx new file mode 100644 index 00000000000..641d9d9cd71 --- /dev/null +++ b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx @@ -0,0 +1,66 @@ +/* +Copyright 2023 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 { renderHook } from "@testing-library/react-hooks"; +import { act } from "@testing-library/react"; + +import { usePlainTextListeners } from "../../../../../../src/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners"; + +describe("setContent", () => { + it("calling with a string calls the onChange argument", () => { + const mockOnChange = jest.fn(); + const { result } = renderHook(() => usePlainTextListeners("initialContent", mockOnChange)); + + const newContent = "new content"; + act(() => { + result.current.setContent(newContent); + }); + + expect(mockOnChange).toHaveBeenCalledWith(newContent); + }); + + it("calling with no argument and no editor ref does not call onChange", () => { + const mockOnChange = jest.fn(); + const { result } = renderHook(() => usePlainTextListeners("initialContent", mockOnChange)); + + act(() => { + result.current.setContent(); + }); + + expect(mockOnChange).toHaveBeenCalledTimes(0); + }); + + it("calling with no argument and a valid editor ref calls onChange with the editorRef innerHTML", () => { + const mockOnChange = jest.fn(); + + // create a div to represent the editor and append some content + const mockEditor = document.createElement("div"); + const mockEditorText = "some text content"; + const textNode = document.createTextNode(mockEditorText); + mockEditor.appendChild(textNode); + + const { result } = renderHook(() => usePlainTextListeners("initialContent", mockOnChange)); + + // @ts-ignore + result.current.ref.current = mockEditor; + + act(() => { + result.current.setContent(); + }); + + expect(mockOnChange).toHaveBeenCalledWith(mockEditor.innerHTML); + }); +}); From b7d6b3bb0d318cb52b1be08089a88a83d8b79c0a Mon Sep 17 00:00:00 2001 From: Alun Turner Date: Fri, 12 May 2023 10:21:32 +0100 Subject: [PATCH 14/15] add comment --- .../rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx index 641d9d9cd71..35ad24ff195 100644 --- a/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx @@ -54,7 +54,7 @@ describe("setContent", () => { const { result } = renderHook(() => usePlainTextListeners("initialContent", mockOnChange)); - // @ts-ignore + // @ts-ignore in order to allow us to reassign the ref without complaint result.current.ref.current = mockEditor; act(() => { From 58d443a1778da1acdb18f067b809273012feb48f Mon Sep 17 00:00:00 2001 From: alunturner <56027671+alunturner@users.noreply.github.com> Date: Tue, 16 May 2023 11:20:29 +0100 Subject: [PATCH 15/15] Update test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx Co-authored-by: Andy Balaam --- .../rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx index 35ad24ff195..779cca73334 100644 --- a/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/hooks/usePlainTextListeners-test.tsx @@ -40,7 +40,7 @@ describe("setContent", () => { result.current.setContent(); }); - expect(mockOnChange).toHaveBeenCalledTimes(0); + expect(mockOnChange).not.toHaveBeenCalled(); }); it("calling with no argument and a valid editor ref calls onChange with the editorRef innerHTML", () => {