Skip to content
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"dependencies": {
"@rc-component/motion": "^1.1.4",
"@rc-component/portal": "^2.1.3",
"@rc-component/util": "^1.7.0",
"@rc-component/util": "^1.9.0",
"clsx": "^2.1.1"
},
"devDependencies": {
Expand Down
4 changes: 2 additions & 2 deletions src/DrawerPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface DrawerPanelEvents {
onClick?: React.MouseEventHandler<HTMLDivElement>;
onKeyDown?: React.KeyboardEventHandler<HTMLDivElement>;
onKeyUp?: React.KeyboardEventHandler<HTMLDivElement>;
onFocus?: React.FocusEventHandler<HTMLDivElement>;
}

export type DrawerPanelAccessibility = Pick<
Expand All @@ -23,8 +24,7 @@ export type DrawerPanelAccessibility = Pick<
>;

export interface DrawerPanelProps
extends DrawerPanelEvents,
DrawerPanelAccessibility {
extends DrawerPanelEvents, DrawerPanelAccessibility {
prefixCls: string;
className?: string;
id?: string;
Expand Down
11 changes: 10 additions & 1 deletion src/DrawerPopup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,13 @@ const DrawerPopup: React.ForwardRefRenderFunction<
React.useImperativeHandle(ref, () => panelRef.current);

// ========================= Focusable ==========================
useFocusable(() => panelRef.current, open, autoFocus, focusTrap, mask);
const ignoreElement = useFocusable(
() => panelRef.current,
open,
autoFocus,
focusTrap,
mask,
);

// ============================ Push ============================
const [pushed, setPushed] = React.useState(false);
Expand Down Expand Up @@ -305,6 +311,9 @@ const DrawerPopup: React.ForwardRefRenderFunction<
onClick,
onKeyDown,
onKeyUp,
onFocus: (e: React.FocusEvent<HTMLDivElement>) => {
ignoreElement(e.target);
},
Comment on lines +314 to +316
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The e.target of a focus event is of type EventTarget, which is not assignable to the HTMLElement type expected by ignoreElement. This could lead to runtime errors. It's safer to add a type check to ensure e.target is an HTMLElement before passing it to ignoreElement.

    onFocus: (e: React.FocusEvent<HTMLDivElement>) => {
      if (e.target instanceof HTMLElement) {
        ignoreElement(e.target);
      }
    },

};

// =========================== Render ==========================
Expand Down
4 changes: 3 additions & 1 deletion src/hooks/useFocusable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ export default function useFocusable(
const mergedFocusTrap = focusTrap ?? mask !== false;

// Focus lock
useLockFocus(open && mergedFocusTrap, getContainer);
const [ignoreElement] = useLockFocus(open && mergedFocusTrap, getContainer);

// Auto Focus
React.useEffect(() => {
if (open && autoFocus === true) {
getContainer()?.focus({ preventScroll: true });
}
}, [open]);

return ignoreElement;
}
61 changes: 61 additions & 0 deletions tests/focus.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { cleanup, render, act } from '@testing-library/react';
import React from 'react';
import ReactDOM from 'react-dom';
import Drawer from '../src';

// Mock useLockFocus to track calls
jest.mock('@rc-component/util/lib/Dom/focus', () => {
const actual = jest.requireActual('@rc-component/util/lib/Dom/focus');

const useLockFocus = (visible: boolean, ...rest: any[]) => {
(globalThis as any).__useLockFocusVisible = visible;
const hooks = actual.useLockFocus(visible, ...rest);
const hooksArray = Array.isArray(hooks) ? hooks : [hooks];
const proxyIgnoreElement = (ele: HTMLElement) => {
(globalThis as any).__ignoredElement = ele;
hooksArray[0](ele);
};
return [proxyIgnoreElement, ...hooksArray.slice(1)] as ReturnType<
typeof actual.useLockFocus
>;
Comment on lines +12 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The implementation of the mocked useLockFocus can be simplified. Since @rc-component/util@1.9.0's useLockFocus is guaranteed to return a tuple [ignore, restore], you can use array destructuring directly instead of the defensive Array.isArray check. This makes the mock cleaner and easier to understand.

    const [ignore, ...otherHooks] = actual.useLockFocus(visible, ...rest);
    const proxyIgnoreElement = (ele: HTMLElement) => {
      (globalThis as any).__ignoredElement = ele;
      ignore(ele);
    };
    return [proxyIgnoreElement, ...otherHooks] as ReturnType<
      typeof actual.useLockFocus
    >;

};

return {
...actual,
useLockFocus,
};
});

describe('Drawer.Focus', () => {
beforeEach(() => {
jest.useFakeTimers();
});

afterEach(() => {
jest.useRealTimers();
cleanup();
});
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The mock for useLockFocus sets properties on globalThis for testing purposes. It's a good practice to clean up these global properties in an afterEach block to avoid test pollution, where state from one test can leak into and affect subsequent tests.

  afterEach(() => {
    jest.useRealTimers();
    cleanup();
    delete (globalThis as any).__ignoredElement;
    delete (globalThis as any).__useLockFocusVisible;
  });


it('should call ignoreElement when input in portal is focused', () => {
render(
<Drawer open>
<input type="text" id="drawer-input" />
{ReactDOM.createPortal(
<input type="text" id="portal-input" />,
document.body,
)}
</Drawer>,
);

act(() => {
jest.runAllTimers();
});

const input = document.getElementById('portal-input') as HTMLElement;
act(() => {
input.focus();
});

expect((globalThis as any).__ignoredElement).toBe(input);
});
});