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.

Sign up for GitHub

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

Jump to bottom

Fix emoji in RTE editing #9827

Merged
merged 6 commits into from
Jan 3, 2023
Merged

Fix emoji in RTE editing #9827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e06139c
Fix emoji in RTE edition
florianduros Dec 22, 2022
d3864aa
Merge branch 'develop' into florianduros/fix/emoji-edition-rte
florianduros Jan 2, 2023
3ffcefc
Add test for emoji in edition mode
florianduros Jan 2, 2023
915d178
Merge branch 'develop' into florianduros/fix/emoji-edition-rte
florianduros Jan 2, 2023
f0192e5
Add comments to EditWysiwygComposer-test.tsx
florianduros Jan 3, 2023
06aa2af
Merge branch 'develop' into florianduros/fix/emoji-edition-rte
florianduros Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/components/views/rooms/wysiwyg_composer/EditWysiwygComposer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { forwardRef, RefObject, useRef } from "react";
import React, { ForwardedRef, forwardRef, MutableRefObject, useRef } from "react";
import classNames from "classnames";

import EditorStateTransfer from "../../../../utils/EditorStateTransfer";
Expand All @@ -24,16 +24,18 @@ import { useWysiwygEditActionHandler } from "./hooks/useWysiwygEditActionHandler
import { useEditing } from "./hooks/useEditing";
import { useInitialContent } from "./hooks/useInitialContent";
import { ComposerContext, getDefaultContextValue } from "./ComposerContext";
import { ComposerFunctions } from "./types";

interface ContentProps {
disabled: boolean;
disabled?: boolean;
composerFunctions: ComposerFunctions;
}

const Content = forwardRef<HTMLElement, ContentProps>(function Content(
{ disabled }: ContentProps,
forwardRef: RefObject<HTMLElement>,
{ disabled = false, composerFunctions }: ContentProps,
forwardRef: ForwardedRef<HTMLElement>,
) {
useWysiwygEditActionHandler(disabled, forwardRef);
useWysiwygEditActionHandler(disabled, forwardRef as MutableRefObject<HTMLElement>, composerFunctions);
return null;
});

Expand Down Expand Up @@ -65,9 +67,9 @@ export default function EditWysiwygComposer({ editorStateTransfer, className, ..
onSend={editMessage}
{...props}
>
{(ref) => (
{(ref, composerFunctions) => (
<>
<Content disabled={props.disabled} ref={ref} />
<Content disabled={props.disabled} ref={ref} composerFunctions={composerFunctions} />
<EditionButtons
onCancelClick={endEditing}
onSaveClick={editMessage}
Expand Down
15 changes: 5 additions & 10 deletions src/components/views/rooms/wysiwyg_composer/components/Emoji.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,23 @@ import dis from "../../../../../dispatcher/dispatcher";
import { ComposerInsertPayload } from "../../../../../dispatcher/payloads/ComposerInsertPayload";
import { Action } from "../../../../../dispatcher/actions";
import { useRoomContext } from "../../../../../contexts/RoomContext";
import { useComposerContext } from "../ComposerContext";
import { setSelection } from "../utils/selection";

interface EmojiProps {
menuPosition: AboveLeftOf;
}

export function Emoji({ menuPosition }: EmojiProps) {
const roomContext = useRoomContext();
const composerContext = useComposerContext();

return (
<EmojiButton
menuPosition={menuPosition}
addEmoji={(emoji) => {
setSelection(composerContext.selection).then(() =>
dis.dispatch<ComposerInsertPayload>({
action: Action.ComposerInsert,
text: emoji,
timelineRenderingType: roomContext.timelineRenderingType,
}),
);
dis.dispatch<ComposerInsertPayload>({
action: Action.ComposerInsert,
text: emoji,
timelineRenderingType: roomContext.timelineRenderingType,
});
return true;
}}
/>
Expand Down
21 changes: 19 additions & 2 deletions src/components/views/rooms/wysiwyg_composer/hooks/useWysiwygEditActionHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,18 @@ import { ActionPayload } from "../../../../../dispatcher/payloads";
import { TimelineRenderingType, useRoomContext } from "../../../../../contexts/RoomContext";
import { useDispatcher } from "../../../../../hooks/useDispatcher";
import { focusComposer } from "./utils";
import { ComposerType } from "../../../../../dispatcher/payloads/ComposerInsertPayload";
import { ComposerFunctions } from "../types";
import { setSelection } from "../utils/selection";
import { useComposerContext } from "../ComposerContext";

export function useWysiwygEditActionHandler(disabled: boolean, composerElement: RefObject<HTMLElement>) {
export function useWysiwygEditActionHandler(
disabled: boolean,
composerElement: RefObject<HTMLElement>,
composerFunctions: ComposerFunctions,
) {
const roomContext = useRoomContext();
const composerContext = useComposerContext();
const timeoutId = useRef<number | null>(null);

const handler = useCallback(
Expand All @@ -39,9 +48,17 @@ export function useWysiwygEditActionHandler(disabled: boolean, composerElement:
case Action.FocusEditMessageComposer:
focusComposer(composerElement, context, roomContext, timeoutId);
break;
case Action.ComposerInsert:
if (payload.timelineRenderingType !== roomContext.timelineRenderingType) break;
if (payload.composerType !== ComposerType.Edit) break;

if (payload.text) {
setSelection(composerContext.selection).then(() => composerFunctions.insertText(payload.text));
}
break;
Comment on lines +51 to +58
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComposerInsert is documented as:

Inserts content into the active composer. Should be used with ComposerInsertPayload.

It doesn't mention anything about changing the selection or checking the timeline rendering type. Maybe it should?

Copy link
Contributor Author

@florianduros florianduros Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous composer, the selection change is made in the EditorModel, in the call of insertText.

Why ? Because when we are clicking or using the modal, the composer loses the focus and its selection. The selection is saved on blur and set when needed (here for example with emoji modal).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine. Just saying, maybe the documentation for ComposerInsert should be updated to reflect the current behaviour, even if that means noting the different behaviour for the two composers.

}
},
[disabled, composerElement, timeoutId, roomContext],
[disabled, composerElement, composerFunctions, timeoutId, roomContext, composerContext],
);

useDispatcher(defaultDispatcher, handler);
Expand Down
7 changes: 5 additions & 2 deletions src/components/views/rooms/wysiwyg_composer/hooks/useWysiwygSendActionHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,16 @@ import { useDispatcher } from "../../../../../hooks/useDispatcher";
import { focusComposer } from "./utils";
import { ComposerFunctions } from "../types";
import { ComposerType } from "../../../../../dispatcher/payloads/ComposerInsertPayload";
import { useComposerContext } from "../ComposerContext";
import { setSelection } from "../utils/selection";

export function useWysiwygSendActionHandler(
disabled: boolean,
composerElement: MutableRefObject<HTMLElement>,
composerFunctions: ComposerFunctions,
) {
const roomContext = useRoomContext();
const composerContext = useComposerContext();
const timeoutId = useRef<number | null>(null);

const handler = useCallback(
Expand Down Expand Up @@ -59,12 +62,12 @@ export function useWysiwygSendActionHandler(
} else if (payload.event) {
// TODO insert quote message - see SendMessageComposer
} else if (payload.text) {
composerFunctions.insertText(payload.text);
setSelection(composerContext.selection).then(() => composerFunctions.insertText(payload.text));
}
break;
}
},
[disabled, composerElement, composerFunctions, timeoutId, roomContext],
[disabled, composerElement, roomContext, composerFunctions, composerContext],
);

useDispatcher(defaultDispatcher, handler);
Expand Down
46 changes: 46 additions & 0 deletions test/components/views/rooms/wysiwyg_composer/EditWysiwygComposer-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ import { IRoomState } from "../../../../../src/components/structures/RoomView";
import { createTestClient, flushPromises, getRoomContext, mkEvent, mkStubRoom } from "../../../../test-utils";
import { EditWysiwygComposer } from "../../../../../src/components/views/rooms/wysiwyg_composer";
import EditorStateTransfer from "../../../../../src/utils/EditorStateTransfer";
import { Emoji } from "../../../../../src/components/views/rooms/wysiwyg_composer/components/Emoji";
import { ChevronFace } from "../../../../../src/components/structures/ContextMenu";
import dis from "../../../../../src/dispatcher/dispatcher";
import { ComposerInsertPayload, ComposerType } from "../../../../../src/dispatcher/payloads/ComposerInsertPayload";
import { ActionPayload } from "../../../../../src/dispatcher/payloads";
import * as EmojiButton from "../../../../../src/components/views/rooms/EmojiButton";

describe("EditWysiwygComposer", () => {
afterEach(() => {
Expand Down Expand Up @@ -269,4 +275,44 @@ describe("EditWysiwygComposer", () => {
// Then we don't get it because we are disabled
expect(screen.getByRole("textbox")).not.toHaveFocus();
});

it("Should add emoji", async () => {
// When

// We are not testing here the emoji button (open modal, select emoji ...)
// Instead we are directly firing an emoji to make the test easier to write
jest.spyOn(EmojiButton, "EmojiButton").mockImplementation(
({ addEmoji }: { addEmoji: (emoji: string) => void }) => {
return (
<button aria-label="Emoji" type="button" onClick={() => addEmoji("🦫")}>
Emoji
</button>
);
},
);
render(
<MatrixClientContext.Provider value={mockClient}>
<RoomContext.Provider value={defaultRoomContext}>
<EditWysiwygComposer editorStateTransfer={editorStateTransfer} />
<Emoji menuPosition={{ chevronFace: ChevronFace.Top }} />
</RoomContext.Provider>
</MatrixClientContext.Provider>,
);
// Same behavior as in RoomView.tsx
// RoomView is re-dispatching the composer messages.
// It adds the composerType fields where the value refers if the composer is in editing or not
// The listeners in the RTE ignore the message if the composerType is missing in the payload
const dispatcherRef = dis.register((payload: ActionPayload) => {
dis.dispatch<ComposerInsertPayload>({
...(payload as ComposerInsertPayload),
composerType: ComposerType.Edit,
});
});

screen.getByLabelText("Emoji").click();

// Then
await waitFor(() => expect(screen.getByRole("textbox")).toHaveTextContent(/🦫/));
dis.unregister(dispatcherRef);
});
});
2 changes: 1 addition & 1 deletion test/components/views/rooms/wysiwyg_composer/SendWysiwygComposer-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ describe("SendWysiwygComposer", () => {
});

const textNode = screen.getByRole("textbox").firstChild;
setSelection({
await setSelection({
anchorNode: textNode,
anchorOffset: 2,
focusNode: textNode,
Expand Down