From e8f7be9147fc9a76673d2d158785cdce5a84c371 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 13 Oct 2022 10:44:36 +0100 Subject: [PATCH 1/7] Automatically focus the WYSIWYG composer when you enter a room --- .../wysiwyg_composer/WysiwygComposer.tsx | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx index e45324982d8..fd8027c6e7b 100644 --- a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx +++ b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx @@ -15,12 +15,16 @@ limitations under the License. */ import React, { useCallback, useState } from 'react'; -import { useWysiwyg } from "@matrix-org/matrix-wysiwyg"; import { IEventRelation, MatrixEvent } from 'matrix-js-sdk/src/models/event'; +import { useWysiwyg } from "@matrix-org/matrix-wysiwyg"; -import { useRoomContext } from '../../../../contexts/RoomContext'; -import { sendMessage } from './message'; +import defaultDispatcher from '../../../../dispatcher/dispatcher'; +import { Action } from '../../../../dispatcher/actions'; +import { ActionPayload } from '../../../../dispatcher/payloads'; import { RoomPermalinkCreator } from '../../../../utils/permalinks/Permalinks'; +import { TimelineRenderingType, useRoomContext } from '../../../../contexts/RoomContext'; +import { sendMessage } from './message'; +import { useDispatcher } from "../../../../hooks/useDispatcher"; import { useMatrixClientContext } from '../../../../contexts/MatrixClientContext'; interface WysiwygProps { @@ -51,6 +55,32 @@ export function WysiwygComposer( ref.current?.focus(); }, [content, mxClient, roomContext, wysiwyg, props, ref]); + useDispatcher(defaultDispatcher, (payload: ActionPayload) => { + // don't let the user into the composer if it is disabled - all of these branches lead + // to the cursor being in the composer + if (disabled) return; + + const context = payload.context ?? TimelineRenderingType.Room; + + switch (payload.action) { + case 'reply_to_event': + case Action.FocusSendMessageComposer: + if (context === roomContext.timelineRenderingType) { + // Immediately set the focus, so if you start typing it + // will appear in the composer + ref.current?.focus(); + // If we call focus immediate, the focus _is_ in the right + // place, but the cursor is invisible, presumably because + // some other event is still processing. + // The following line ensures that the cursor is actually + // visible in composer. + setTimeout(() => ref.current?.focus(), 200); + } + break; + // TODO: case Action.ComposerInsert: - see SendMessageComposer + } + }); + return (
From f3e6ce9d7b762a403f29abf526da388e7618503d Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 13 Oct 2022 14:03:29 +0100 Subject: [PATCH 2/7] Cancel the composer focus timeout --- .../views/rooms/wysiwyg_composer/WysiwygComposer.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx index fd8027c6e7b..8aa72db287f 100644 --- a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx +++ b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { useCallback, useState } from 'react'; +import React, { useCallback, useRef, useState } from 'react'; import { IEventRelation, MatrixEvent } from 'matrix-js-sdk/src/models/event'; import { useWysiwyg } from "@matrix-org/matrix-wysiwyg"; @@ -42,6 +42,7 @@ export function WysiwygComposer( ) { const roomContext = useRoomContext(); const mxClient = useMatrixClientContext(); + const timeoutId = useRef(); const [content, setContent] = useState(); const { ref, isWysiwygReady, wysiwyg } = useWysiwyg({ onChange: (_content) => { @@ -74,7 +75,10 @@ export function WysiwygComposer( // some other event is still processing. // The following line ensures that the cursor is actually // visible in composer. - setTimeout(() => ref.current?.focus(), 200); + if (timeoutId.current) { + clearTimeout(timeoutId.current); + } + timeoutId.current = setTimeout(() => ref.current?.focus(), 200); } break; // TODO: case Action.ComposerInsert: - see SendMessageComposer From 8c2413251b79f7826744ab4105ca2b224d0d2ef9 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 13 Oct 2022 15:22:53 +0100 Subject: [PATCH 3/7] Extract wysiwyg dispatcher logic into a separate hook --- .../wysiwyg_composer/WysiwygComposer.tsx | 39 +--------- .../useWysiwygActionHandler.ts | 74 +++++++++++++++++++ 2 files changed, 78 insertions(+), 35 deletions(-) create mode 100644 src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts diff --git a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx index 8aa72db287f..ce3d01335b1 100644 --- a/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx +++ b/src/components/views/rooms/wysiwyg_composer/WysiwygComposer.tsx @@ -14,18 +14,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { useCallback, useRef, useState } from 'react'; +import React, { useCallback, useState } from 'react'; import { IEventRelation, MatrixEvent } from 'matrix-js-sdk/src/models/event'; import { useWysiwyg } from "@matrix-org/matrix-wysiwyg"; -import defaultDispatcher from '../../../../dispatcher/dispatcher'; -import { Action } from '../../../../dispatcher/actions'; -import { ActionPayload } from '../../../../dispatcher/payloads'; import { RoomPermalinkCreator } from '../../../../utils/permalinks/Permalinks'; -import { TimelineRenderingType, useRoomContext } from '../../../../contexts/RoomContext'; import { sendMessage } from './message'; -import { useDispatcher } from "../../../../hooks/useDispatcher"; import { useMatrixClientContext } from '../../../../contexts/MatrixClientContext'; +import { useRoomContext } from '../../../../contexts/RoomContext'; +import { useWysiwygActionHandler } from './useWysiwygActionHandler'; interface WysiwygProps { disabled?: boolean; @@ -42,7 +39,6 @@ export function WysiwygComposer( ) { const roomContext = useRoomContext(); const mxClient = useMatrixClientContext(); - const timeoutId = useRef(); const [content, setContent] = useState(); const { ref, isWysiwygReady, wysiwyg } = useWysiwyg({ onChange: (_content) => { @@ -56,34 +52,7 @@ export function WysiwygComposer( ref.current?.focus(); }, [content, mxClient, roomContext, wysiwyg, props, ref]); - useDispatcher(defaultDispatcher, (payload: ActionPayload) => { - // don't let the user into the composer if it is disabled - all of these branches lead - // to the cursor being in the composer - if (disabled) return; - - const context = payload.context ?? TimelineRenderingType.Room; - - switch (payload.action) { - case 'reply_to_event': - case Action.FocusSendMessageComposer: - if (context === roomContext.timelineRenderingType) { - // Immediately set the focus, so if you start typing it - // will appear in the composer - ref.current?.focus(); - // If we call focus immediate, the focus _is_ in the right - // place, but the cursor is invisible, presumably because - // some other event is still processing. - // The following line ensures that the cursor is actually - // visible in composer. - if (timeoutId.current) { - clearTimeout(timeoutId.current); - } - timeoutId.current = setTimeout(() => ref.current?.focus(), 200); - } - break; - // TODO: case Action.ComposerInsert: - see SendMessageComposer - } - }); + useWysiwygActionHandler(disabled, ref); return (
diff --git a/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts b/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts new file mode 100644 index 00000000000..3cbea4c2160 --- /dev/null +++ b/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts @@ -0,0 +1,74 @@ +/* +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 { useRef } from "react"; + +import defaultDispatcher from "../../../../dispatcher/dispatcher"; +import { Action } from "../../../../dispatcher/actions"; +import { ActionPayload } from "../../../../dispatcher/payloads"; +import { IRoomState } from "../../../structures/RoomView"; +import { TimelineRenderingType } from "../../../../contexts/RoomContext"; +import { useDispatcher } from "../../../../hooks/useDispatcher"; +import { useRoomContext } from "../../../../contexts/RoomContext"; + +export function useWysiwygActionHandler( + disabled: boolean, + composerElement: React.MutableRefObject, +) { + const roomContext = useRoomContext(); + const timeoutId = useRef(); + + useDispatcher(defaultDispatcher, (payload: ActionPayload) => { + // don't let the user into the composer if it is disabled - all of these branches lead + // to the cursor being in the composer + if (disabled) return; + + const context = payload.context ?? TimelineRenderingType.Room; + + switch (payload.action) { + case "reply_to_event": + case Action.FocusSendMessageComposer: + focusComposer(composerElement, context, roomContext, timeoutId); + break; + // TODO: case Action.ComposerInsert: - see SendMessageComposer + } + }); +} + +function focusComposer( + composerElement: React.MutableRefObject, + renderingType: TimelineRenderingType, + roomContext: IRoomState, + timeoutId: React.MutableRefObject, +) { + if (renderingType === roomContext.timelineRenderingType) { + // Immediately set the focus, so if you start typing it + // will appear in the composer + composerElement.current?.focus(); + // If we call focus immediate, the focus _is_ in the right + // place, but the cursor is invisible, presumably because + // some other event is still processing. + // The following line ensures that the cursor is actually + // visible in composer. + if (timeoutId.current) { + clearTimeout(timeoutId.current); + } + timeoutId.current = setTimeout( + () => composerElement.current?.focus(), + 200, + ); + } +} From 596d0c98fdb3485516fbce7add9c6b17a9239971 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 14 Oct 2022 10:31:54 +0100 Subject: [PATCH 4/7] Test that composer gets focus when the relevant action happens --- .../wysiwyg_composer/WysiwygComposer-test.tsx | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx index 171455bfbc6..c32e0098c12 100644 --- a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx @@ -14,16 +14,18 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React from "react"; -import { act, render, screen } from "@testing-library/react"; import "@testing-library/jest-dom"; +import React from "react"; +import { act, render, screen, waitFor } from "@testing-library/react"; -import { IRoomState } from "../../../../../src/components/structures/RoomView"; +import MatrixClientContext from "../../../../../src/contexts/MatrixClientContext"; import RoomContext, { TimelineRenderingType } from "../../../../../src/contexts/RoomContext"; +import defaultDispatcher from "../../../../../src/dispatcher/dispatcher"; +import { Action } from "../../../../../src/dispatcher/actions"; +import { IRoomState } from "../../../../../src/components/structures/RoomView"; import { Layout } from "../../../../../src/settings/enums/Layout"; -import { createTestClient, mkEvent, mkStubRoom } from "../../../../test-utils"; -import MatrixClientContext from "../../../../../src/contexts/MatrixClientContext"; import { WysiwygComposer } from "../../../../../src/components/views/rooms/wysiwyg_composer/WysiwygComposer"; +import { createTestClient, mkEvent, mkStubRoom } from "../../../../test-utils"; let callOnChange: (content: string) => void; @@ -95,7 +97,7 @@ describe('WysiwygComposer', () => { }; let sendMessage: () => void; - const customRender = (onChange = (content: string) => void 0, disabled = false) => { + const customRender = (onChange = (_content: string) => void 0, disabled = false) => { return render( @@ -144,5 +146,20 @@ describe('WysiwygComposer', () => { expect(mockClient.sendMessage).toBeCalledWith('myfakeroom', null, expectedContent); expect(screen.getByRole('textbox')).toHaveFocus(); }); + + it('Should focus when receiving an Action.FocusSendMessageComposer action', async () => { + // Given we don't have focus + customRender(null, false).container; + expect(screen.getByRole('textbox')).not.toHaveFocus(); + + // When we send the right action + defaultDispatcher.dispatch({ + action: Action.FocusSendMessageComposer, + context: null, + }); + + // Then the component gets the focus + await waitFor(() => expect(screen.getByRole('textbox')).toHaveFocus()); + }); }); From bc0bb0b2e5d5ed37e204c51c42bd4aaf17361a7f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 17 Oct 2022 10:20:12 +0100 Subject: [PATCH 5/7] Pass a dummy onChange method to customRender --- .../views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx index 6d4a7533495..539c45fffaf 100644 --- a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx @@ -146,7 +146,7 @@ describe('WysiwygComposer', () => { it('Should focus when receiving an Action.FocusSendMessageComposer action', async () => { // Given we don't have focus - customRender(null, false).container; + customRender(() => {}, false).container; expect(screen.getByRole('textbox')).not.toHaveFocus(); // When we send the right action From a6e399d7634dae6ea07baef927e3f318f8811c68 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 17 Oct 2022 10:20:34 +0100 Subject: [PATCH 6/7] Remove unneeded .container --- .../views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx index 539c45fffaf..6b7e9fa9520 100644 --- a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx @@ -146,7 +146,7 @@ describe('WysiwygComposer', () => { it('Should focus when receiving an Action.FocusSendMessageComposer action', async () => { // Given we don't have focus - customRender(() => {}, false).container; + customRender(() => {}, false); expect(screen.getByRole('textbox')).not.toHaveFocus(); // When we send the right action From b071289c3bb46ace8901c7dcaaa7f1468a27260a Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 17 Oct 2022 12:04:19 +0100 Subject: [PATCH 7/7] Improve test coverage --- .../useWysiwygActionHandler.ts | 3 +- .../wysiwyg_composer/WysiwygComposer-test.tsx | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts b/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts index 3cbea4c2160..683498d485e 100644 --- a/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts +++ b/src/components/views/rooms/wysiwyg_composer/useWysiwygActionHandler.ts @@ -20,9 +20,8 @@ import defaultDispatcher from "../../../../dispatcher/dispatcher"; import { Action } from "../../../../dispatcher/actions"; import { ActionPayload } from "../../../../dispatcher/payloads"; import { IRoomState } from "../../../structures/RoomView"; -import { TimelineRenderingType } from "../../../../contexts/RoomContext"; +import { TimelineRenderingType, useRoomContext } from "../../../../contexts/RoomContext"; import { useDispatcher } from "../../../../hooks/useDispatcher"; -import { useRoomContext } from "../../../../contexts/RoomContext"; export function useWysiwygActionHandler( disabled: boolean, diff --git a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx index 6b7e9fa9520..b0aa838879b 100644 --- a/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx +++ b/test/components/views/rooms/wysiwyg_composer/WysiwygComposer-test.tsx @@ -158,5 +158,43 @@ describe('WysiwygComposer', () => { // Then the component gets the focus await waitFor(() => expect(screen.getByRole('textbox')).toHaveFocus()); }); + + it('Should focus when receiving a reply_to_event action', async () => { + // Given we don't have focus + customRender(() => {}, false); + expect(screen.getByRole('textbox')).not.toHaveFocus(); + + // When we send the right action + defaultDispatcher.dispatch({ + action: "reply_to_event", + context: null, + }); + + // Then the component gets the focus + await waitFor(() => expect(screen.getByRole('textbox')).toHaveFocus()); + }); + + it('Should not focus when disabled', async () => { + // Given we don't have focus and we are disabled + customRender(() => {}, true); + expect(screen.getByRole('textbox')).not.toHaveFocus(); + + // When we send an action that would cause us to get focus + defaultDispatcher.dispatch({ + action: Action.FocusSendMessageComposer, + context: null, + }); + // (Send a second event to exercise the clearTimeout logic) + defaultDispatcher.dispatch({ + action: Action.FocusSendMessageComposer, + context: null, + }); + + // Wait for event dispatch to happen + await new Promise((r) => setTimeout(r, 200)); + + // Then we don't get it because we are disabled + expect(screen.getByRole('textbox')).not.toHaveFocus(); + }); });