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.

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

fix: press space to unmute #18544

Merged
merged 2 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const APP_NOTIFICATION_SELECTOR = '#app-notification';
// Small delay to ensure rendering is complete.
// In some cases (switching between windows) the notification is not displayed without this delay.
// It's caused by the rendering behavior of the toast library (injecting the <Toaster/> into the DOM node).
const ACTION_DELAY_MS = 1;
const ACTION_DELAY_MS = 100;

// Stores React roots for different windows (activeWindow).
// Each window (identified by its 'name' property or 'default' if not available) gets its own root.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,14 @@ export const usePressSpaceToUnmute = ({callState, toggleMute, isMuted, enabled}:
holdDelayMs: HOLD_DELAY_MS,
onHold: () => {
if (!isMuted()) {
return;
return false;
}

toggleMute(false);
micOnNotification.show();
amplify.publish(WebAppEvents.ANALYTICS.EVENT, EventName.CALLING.PRESS_SPACE_TO_UNMUTE);

return true;
},
onRelease: () => {
toggleMute(true);
Expand Down
25 changes: 20 additions & 5 deletions src/script/hooks/useKeyPressAndHold/useKeyPressAndHold.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {useKeyPressAndHold} from './useKeyPressAndHold';
jest.useFakeTimers();

describe('useKeyPressAndHold', () => {
const mockOnHold = jest.fn();
const mockOnHold = jest.fn().mockReturnValue(true);
const mockOnRelease = jest.fn();
const defaultProps = {
key: KEY.SPACE,
Expand Down Expand Up @@ -67,22 +67,37 @@ describe('useKeyPressAndHold', () => {
expect(mockOnRelease).toHaveBeenCalledTimes(1);
});

it("doesn't trigger onHold if key is released before holdDelay", () => {
it('calls onRelease only when onHold returns true', () => {
renderHook(() => useKeyPressAndHold(defaultProps));

act(() => {
window.dispatchEvent(new KeyboardEvent('keydown', {key: KEY.SPACE}));
window.dispatchEvent(new KeyboardEvent('keyup', {key: KEY.SPACE}));
});

expect(mockOnRelease).not.toHaveBeenCalled();

act(() => {
jest.advanceTimersByTime(100);
window.dispatchEvent(new KeyboardEvent('keydown', {key: KEY.SPACE}));
jest.advanceTimersByTime(200);
window.dispatchEvent(new KeyboardEvent('keyup', {key: KEY.SPACE}));
});

expect(mockOnHold).not.toHaveBeenCalled();
expect(mockOnHold).toHaveBeenCalledTimes(1);
expect(mockOnRelease).toHaveBeenCalledTimes(1);
});

it("doesn't call onRelease only when onHold returns false", () => {
renderHook(() => useKeyPressAndHold({...defaultProps, onHold: jest.fn().mockReturnValue(false)}));

act(() => {
window.dispatchEvent(new KeyboardEvent('keydown', {key: KEY.SPACE}));
jest.advanceTimersByTime(200);
window.dispatchEvent(new KeyboardEvent('keyup', {key: KEY.SPACE}));
});

expect(mockOnRelease).toHaveBeenCalledTimes(0);
});

it("doesn't respond to key events when disabled", () => {
renderHook(() => useKeyPressAndHold({...defaultProps, enabled: false}));

Expand Down
14 changes: 11 additions & 3 deletions src/script/hooks/useKeyPressAndHold/useKeyPressAndHold.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {KEY} from 'Util/KeyboardUtil';

interface KeyPressAndHoldParams {
key: (typeof KEY)[keyof typeof KEY];
onHold: () => void;
onHold: () => boolean;
onRelease: () => void;
holdDelayMs: number;
enabled?: boolean;
Expand All @@ -41,6 +41,7 @@ export const useKeyPressAndHold = ({
const hasTriggeredRef = useRef(false);
const holdTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const isKeyDownRef = useRef(false);
const isPressHandledRef = useRef(false);

const handlePress = useCallback(() => {
if (holdTimeoutRef.current) {
Expand All @@ -52,8 +53,9 @@ export const useKeyPressAndHold = ({
return;
}

onHold();
const called = onHold();
hasTriggeredRef.current = true;
isPressHandledRef.current = called;
}, holdDelayMs);
}, [onHold, holdDelayMs]);

Expand All @@ -66,8 +68,14 @@ export const useKeyPressAndHold = ({

const handleRelease = useCallback(() => {
clearHoldTimeout();
onRelease();
hasTriggeredRef.current = false;

if (!isPressHandledRef.current) {
return;
}

onRelease();
isPressHandledRef.current = false;
}, [onRelease, clearHoldTimeout]);

const handleKeyDown = useCallback(
Expand Down
Loading