fix(app): scope message rings and reach the composer by key (#2781)

This commit is contained in:
Hampus
2026-09-14 21:44:12 +02:00
committed by GitHub
parent d0c6146429
commit 1861432a53
9 changed files with 228 additions and 13 deletions
@@ -23,6 +23,7 @@ import {KeyboardShortcutsCheatsheetModal} from '@app/features/input/components/m
import Keybind, {type KeybindCommand} from '@app/features/input/state/InputKeybind';
import MessageEdit from '@app/features/messaging/state/MessageEdit';
import SavedMessages from '@app/features/messaging/state/SavedMessages';
import {focusChannelTextareaFromKeybind} from '@app/features/messaging/utils/ChannelTextareaFocusUtils';
import {openExternalUrlWithWarning} from '@app/features/messaging/utils/ExternalLinkUtils';
import {buildChannelLink} from '@app/features/messaging/utils/MessageLinkUtils';
import {goToMessage} from '@app/features/messaging/utils/MessageNavigator';
@@ -537,7 +538,7 @@ export function registerDefaultKeybindHandlers(host: HandlerHost, i18n: I18n): v
if (type !== 'press') return;
const channelId = host.currentChannelId;
if (!channelId) return;
ComponentBus.dispatch('FOCUS_TEXTAREA', {channelId});
focusChannelTextareaFromKeybind(channelId);
});
host.register('chat_upload', ({type}) => {
if (type !== 'press') return;
@@ -705,7 +705,6 @@ export const Message: React.FC<MessageProps> = observer((props) => {
<>
<FocusRing
enabled={keyboardNavigationEnabled ? keyboardModeEnabled : undefined}
within={keyboardNavigationEnabled}
offset={keyboardNavigationEnabled ? -2 : undefined}
ringTarget={keyboardNavigationEnabled ? focusRingAnchorRef : undefined}
data-flx="channel.message.focus-ring"
@@ -52,6 +52,7 @@ import {shouldAutoAck} from '@app/features/read_state/utils/AutoAckPredicate';
import {remFromPx} from '@app/features/theme/layout/RemFromPx';
import {Button} from '@app/features/ui/button/Button';
import {Scroller} from '@app/features/ui/components/Scroller';
import FocusRingScope from '@app/features/ui/focus_ring/FocusRingScope';
import KeyboardMode from '@app/features/ui/state/KeyboardMode';
import MediaViewer from '@app/features/ui/state/MediaViewer';
import Modal from '@app/features/ui/state/Modal';
@@ -480,6 +481,9 @@ export const Messages = observer(function Messages({
scrollManager.jumpCancel();
ComponentBus.dispatch('FOCUS_TEXTAREA', {channelId: channel.id});
},
onNavigatePastNewest: () => {
ComponentBus.dispatch('FOCUS_TEXTAREA', {channelId: channel.id, enterKeyboardMode: true});
},
allowWhenInactive: true,
});
useEffect(() => {
@@ -666,6 +670,17 @@ export const Messages = observer(function Messages({
/>
</>
);
const keyboardNavigationEnabled = MessageKeyboardFocusRollout.enabled;
const messageListContent = (
<NearViewportSurfaceContext.Provider value={resolveMessageScrollSurface}>
<CollapsedMessageVisibilityProvider
value={collapsedMessageVisibility}
data-flx="channel.messages.collapsed-message-visibility-provider"
>
{scrollerInner}
</CollapsedMessageVisibilityProvider>
</NearViewportSurfaceContext.Provider>
);
return (
<div className={styles.messagesWrapper} style={messagesWrapperStyle} data-flx="channel.messages.messages-wrapper">
<UploadManager
@@ -705,14 +720,13 @@ export const Messages = observer(function Messages({
aria-busy={safeMessages.loadingMore ? true : undefined}
data-flx="channel.messages.scroller-inner"
>
<NearViewportSurfaceContext.Provider value={resolveMessageScrollSurface}>
<CollapsedMessageVisibilityProvider
value={collapsedMessageVisibility}
data-flx="channel.messages.collapsed-message-visibility-provider"
>
{scrollerInner}
</CollapsedMessageVisibilityProvider>
</NearViewportSurfaceContext.Provider>
{keyboardNavigationEnabled ? (
<FocusRingScope containerRef={scrollerInnerRef} data-flx="channel.messages.focus-ring-scope">
{messageListContent}
</FocusRingScope>
) : (
messageListContent
)}
</div>
</div>
</Scroller>
@@ -12,6 +12,7 @@ const messageCss = readSource('../../theme/styles/Message.module.css');
const actionBarCss = readSource('./MessageActionBar.module.css');
const focusRingCss = readSource('../../ui/focus_ring/FocusRing.module.css');
const channelMessageSource = readSource('./ChannelMessage.tsx');
const channelMessagesSource = readSource('./ChannelMessages.tsx');
const messageFocusRing = channelMessageSource.match(/<FocusRing\b[^>]*>/)?.[0] ?? '';
interface CssRule {
@@ -46,7 +47,18 @@ describe('message focus ring contract', () => {
it('only enables the ring in keyboard navigation mode in the experiment arm', () => {
expect(messageFocusRing).toMatch(/enabled=\{keyboardNavigationEnabled \? keyboardModeEnabled : undefined\}/);
expect(messageFocusRing).toMatch(/within=\{keyboardNavigationEnabled\}/);
});
it('leaves focused descendants their own rings instead of drawing the row ring over them', () => {
expect(messageFocusRing).not.toMatch(/\bwithin\b/);
});
it('scopes message list rings inside the scroll content in the experiment arm', () => {
const scopedList = channelMessagesSource.match(
/\{keyboardNavigationEnabled \? \(\s*<FocusRingScope containerRef=\{scrollerInnerRef\}[^>]*>\s*\{messageListContent\}\s*<\/FocusRingScope>\s*\) : \(\s*messageListContent\s*\)\}/,
);
expect(scopedList).not.toBeNull();
expect(channelMessagesSource).toMatch(/const keyboardNavigationEnabled = MessageKeyboardFocusRollout\.enabled;/);
});
it('insets the ring inside the row in the experiment arm and keeps the default geometry in control', () => {
@@ -47,10 +47,22 @@ function focusedRowId(): string | null {
return active instanceof HTMLElement ? (active.dataset.messageId ?? null) : null;
}
function render(onFocusMessage?: (messageId: string) => void): void {
interface EdgeOptions {
onNavigatePastNewest?: () => void;
onLoadMoreAfter?: () => void;
hasMoreAfter?: boolean;
}
function render(onFocusMessage?: (messageId: string) => void, edge: EdgeOptions = {}): void {
const containerRef: RefObject<HTMLElement | null> = {current: viewport};
function Harness(): null {
useMessageListKeyboardNavigation({containerRef, channelId: CHANNEL_ID, onFocusMessage, allowWhenInactive: true});
useMessageListKeyboardNavigation({
containerRef,
channelId: CHANNEL_ID,
onFocusMessage,
allowWhenInactive: true,
...edge,
});
return null;
}
act(() => {
@@ -219,6 +231,52 @@ describe('useMessageListKeyboardNavigation', () => {
});
});
describe('useMessageListKeyboardNavigation past the newest message', () => {
const focusRow = (messageId: string) => {
findMessageElement(document, viewport, CHANNEL_ID, messageId)?.focus({preventScroll: true});
};
it('hands focus to the composer when stepping down past the newest message', () => {
mountRows([
{messageId: '1', idPrefix: CHANNEL_MESSAGE_ID_PREFIX},
{messageId: '2', idPrefix: CHANNEL_MESSAGE_ID_PREFIX},
]);
const onNavigatePastNewest = vi.fn();
render(focusRow, {onNavigatePastNewest});
pressArrow('ArrowDown');
pressArrow('ArrowDown');
expect(focusedRowId()).toBe('2');
expect(onNavigatePastNewest).not.toHaveBeenCalled();
pressArrow('ArrowDown');
expect(onNavigatePastNewest).toHaveBeenCalledTimes(1);
});
it('loads newer messages instead of leaving the list while more exist after it', () => {
mountRows([{messageId: '1', idPrefix: CHANNEL_MESSAGE_ID_PREFIX}]);
const onNavigatePastNewest = vi.fn();
const onLoadMoreAfter = vi.fn();
render(focusRow, {onNavigatePastNewest, onLoadMoreAfter, hasMoreAfter: true});
pressArrow('ArrowDown');
pressArrow('ArrowDown');
expect(onLoadMoreAfter).toHaveBeenCalledTimes(1);
expect(onNavigatePastNewest).not.toHaveBeenCalled();
});
it('stays on the newest message in the control arm', () => {
rolloutMock.enabled = false;
mountRows([{messageId: '1', idPrefix: CHANNEL_MESSAGE_ID_PREFIX}]);
const onNavigatePastNewest = vi.fn();
render(focusRow, {onNavigatePastNewest});
pressArrow('ArrowDown');
pressArrow('ArrowDown');
expect(onNavigatePastNewest).not.toHaveBeenCalled();
});
});
describe('useMessageListKeyboardNavigation control arm', () => {
beforeEach(() => {
rolloutMock.enabled = false;
@@ -18,6 +18,7 @@ interface MessageListKeyboardNavigationOptions {
hasMoreAfter?: boolean;
isLoadingMore?: boolean;
onEscape?: () => void;
onNavigatePastNewest?: () => void;
allowWhenInactive?: boolean;
}
@@ -67,6 +68,7 @@ export function useMessageListKeyboardNavigation(options: MessageListKeyboardNav
hasMoreAfter = false,
isLoadingMore = false,
onEscape,
onNavigatePastNewest,
allowWhenInactive = false,
} = options;
const keyboardModeEnabled = KeyboardMode.keyboardModeEnabled;
@@ -168,6 +170,8 @@ export function useMessageListKeyboardNavigation(options: MessageListKeyboardNav
if (nextIdx >= nodes.length) {
if (hasMoreAfter && onLoadMoreAfter && !isLoadingMore) {
onLoadMoreAfter();
} else if (keyboardNavigationEnabled && !hasMoreAfter && onNavigatePastNewest) {
onNavigatePastNewest();
}
return;
}
@@ -241,6 +245,7 @@ export function useMessageListKeyboardNavigation(options: MessageListKeyboardNav
hasMoreAfter,
isLoadingMore,
onEscape,
onNavigatePastNewest,
allowWhenInactive,
]);
}
@@ -0,0 +1,48 @@
// SPDX-License-Identifier: AGPL-3.0-or-later
import {readFileSync} from 'node:fs';
import {fileURLToPath} from 'node:url';
import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest';
const rolloutMock = {enabled: true};
const dispatch = vi.fn();
vi.mock('@app/features/messaging/state/MessageKeyboardFocusRollout', () => ({default: rolloutMock}));
vi.mock('@app/features/platform/utils/ComponentBus', () => ({ComponentBus: {dispatch}}));
const {focusChannelTextareaFromKeybind} = await import('@app/features/messaging/utils/ChannelTextareaFocusUtils');
const CHANNEL_ID = '900000000000000001';
beforeEach(() => {
rolloutMock.enabled = true;
dispatch.mockReset();
});
afterEach(() => {
vi.restoreAllMocks();
});
describe('focusChannelTextareaFromKeybind', () => {
it('keeps keyboard mode on when the keybind focuses the composer in the experiment arm', () => {
focusChannelTextareaFromKeybind(CHANNEL_ID);
expect(dispatch).toHaveBeenCalledWith('FOCUS_TEXTAREA', {channelId: CHANNEL_ID, enterKeyboardMode: true});
});
it('dispatches the unchanged payload in the control arm', () => {
rolloutMock.enabled = false;
focusChannelTextareaFromKeybind(CHANNEL_ID);
expect(dispatch).toHaveBeenCalledWith('FOCUS_TEXTAREA', {channelId: CHANNEL_ID});
});
it('is what the Tab bound chat_focus_textarea action runs', () => {
const handlers = readFileSync(
fileURLToPath(new URL('../../app/keybindings/keybind_manager/handlers/defaultHandlers.ts', import.meta.url)),
'utf8',
);
const handler = handlers.match(/host\.register\('chat_focus_textarea'[\s\S]*?\n\t\}\);/)?.[0];
expect(handler).toBeDefined();
expect(handler).toMatch(/focusChannelTextareaFromKeybind\(channelId\)/);
expect(handler).not.toMatch(/ComponentBus\.dispatch/);
});
});
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: AGPL-3.0-or-later
import MessageKeyboardFocusRollout from '@app/features/messaging/state/MessageKeyboardFocusRollout';
import {ComponentBus} from '@app/features/platform/utils/ComponentBus';
export function focusChannelTextareaAfterNavigation(channelId: string): void {
@@ -9,3 +10,10 @@ export function focusChannelTextareaAfterNavigation(channelId: string): void {
window.requestAnimationFrame(requestFocus);
window.setTimeout(requestFocus, 300);
}
export function focusChannelTextareaFromKeybind(channelId: string): void {
ComponentBus.dispatch(
'FOCUS_TEXTAREA',
MessageKeyboardFocusRollout.enabled ? {channelId, enterKeyboardMode: true} : {channelId},
);
}
@@ -1,6 +1,7 @@
// @vitest-environment happy-dom
// SPDX-License-Identifier: AGPL-3.0-or-later
import FocusRing from '@app/features/ui/focus_ring/FocusRing';
import FocusRingContext, {type FocusRingContextManager} from '@app/features/ui/focus_ring/FocusRingContext';
import FocusRingManager from '@app/features/ui/focus_ring/FocusRingManager';
import FocusRingScope from '@app/features/ui/focus_ring/FocusRingScope';
@@ -310,3 +311,72 @@ describe('FocusRingScope', () => {
expect(left + width).toBeLessThan(500);
});
});
function NestedRings({outerWithin}: {outerWithin: boolean}) {
const containerRef = useRef<HTMLDivElement>(null);
return (
<div ref={containerRef} data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.div">
<FocusRingScope containerRef={containerRef} data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.scope">
<Capture data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.capture" />
<FocusRing within={outerWithin} data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.row-ring">
<div tabIndex={-1} data-row="true" data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.row">
<div
style={{position: 'absolute', zIndex: 10}}
data-bar="true"
data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.bar"
>
<FocusRing data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.button-ring">
<button type="button" data-flx="ui.focus-ring.focus-ring-scope-test.nested-rings.button">
react
</button>
</FocusRing>
</div>
</div>
</FocusRing>
</FocusRingScope>
</div>
);
}
function requireElement(selector: string): HTMLElement {
const element = container.querySelector<HTMLElement>(selector);
if (element == null) throw new Error(`Missing ${selector}`);
return element;
}
describe('FocusRing nested inside another ring', () => {
test('a focus-within ring replaces the ring of a descendant that draws its own', () => {
act(() => {
root.render(<NestedRings outerWithin={true} />);
});
act(() => {
requireElement('button').focus();
});
expect(requireRingContext().targetElement).toBe(requireElement('[data-row]'));
});
test('a ring without focus-within leaves the descendant its own ring', () => {
act(() => {
root.render(<NestedRings outerWithin={false} />);
});
act(() => {
requireElement('button').focus();
});
expect(requireRingContext().targetElement).toBe(requireElement('button'));
});
test('stacks a descendant ring above an elevated bar and the row ring beneath it', () => {
act(() => {
root.render(<NestedRings outerWithin={false} />);
});
act(() => {
requireElement('button').focus();
});
expect(requireRing().style.zIndex).toBe('11');
act(() => {
requireElement('[data-row]').focus();
});
expect(requireRingContext().targetElement).toBe(requireElement('[data-row]'));
expect(requireRing().style.zIndex).toBe('');
});
});