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: try to fix createPortal close case #492

Merged
merged 3 commits into from
Nov 8, 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
8 changes: 8 additions & 0 deletions docs/demos/portal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
title: Portal
nav:
title: Demo
path: /demo
---

<code src="../examples/portal.tsx"></code>
110 changes: 110 additions & 0 deletions docs/examples/portal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/* eslint no-console:0 */

import Trigger from 'rc-trigger';
import React from 'react';
import { createPortal } from 'react-dom';
import '../../assets/index.less';

const builtinPlacements = {
left: {
points: ['cr', 'cl'],
},
right: {
points: ['cl', 'cr'],
},
top: {
points: ['bc', 'tc'],
},
bottom: {
points: ['tc', 'bc'],
},
topLeft: {
points: ['bl', 'tl'],
},
topRight: {
points: ['br', 'tr'],
},
bottomRight: {
points: ['tr', 'br'],
},
bottomLeft: {
points: ['tl', 'bl'],
},
};

const popupBorderStyle = {
border: '1px solid red',
padding: 10,
background: 'rgba(255, 0, 0, 0.1)',
};

const PortalPopup = () =>
createPortal(
<div
style={popupBorderStyle}
onMouseDown={(e) => {
console.log('Portal Down', e);
e.stopPropagation();
e.preventDefault();
}}
>
i am a portal element
</div>,
document.body,
);
Comment on lines +41 to +54
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议改进门户组件的可访问性和事件处理

当前实现存在以下问题:

  1. 缺少必要的 ARIA 属性
  2. 事件处理可能影响键盘访问性

建议进行如下修改:

 const PortalPopup = () =>
   createPortal(
     <div
       style={popupBorderStyle}
+      role="dialog"
+      aria-modal="true"
+      tabIndex={-1}
       onMouseDown={(e) => {
         console.log('Portal Down', e);
-        e.stopPropagation();
-        e.preventDefault();
+        // 只在点击背景时阻止事件
+        if (e.target === e.currentTarget) {
+          e.stopPropagation();
+          e.preventDefault();
+        }
       }}
+      onKeyDown={(e) => {
+        if (e.key === 'Escape') {
+          // 处理 ESC 关闭
+        }
+      }}
     >
       i am a portal element
     </div>,
     document.body,
   );

Committable suggestion skipped: line range outside the PR's diff.


const Test = () => {
const buttonRef = React.useRef<HTMLButtonElement>(null);
React.useEffect(() => {
const button = buttonRef.current;
if (button) {
button.addEventListener('mousedown', (e) => {
console.log('button natives down');
e.stopPropagation();
e.preventDefault();
});
}
}, []);
Comment on lines +58 to +67
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

需要清理事件监听器以防止内存泄漏

useEffect 中添加的事件监听器在组件卸载时没有被移除。

建议修改如下:

 React.useEffect(() => {
   const button = buttonRef.current;
   if (button) {
-    button.addEventListener('mousedown', (e) => {
+    const handleMouseDown = (e: MouseEvent) => {
       console.log('button natives down');
       e.stopPropagation();
       e.preventDefault();
-    });
+    };
+    
+    button.addEventListener('mousedown', handleMouseDown);
+    
+    return () => {
+      button.removeEventListener('mousedown', handleMouseDown);
+    };
   }
 }, []);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
React.useEffect(() => {
const button = buttonRef.current;
if (button) {
button.addEventListener('mousedown', (e) => {
console.log('button natives down');
e.stopPropagation();
e.preventDefault();
});
}
}, []);
React.useEffect(() => {
const button = buttonRef.current;
if (button) {
const handleMouseDown = (e: MouseEvent) => {
console.log('button natives down');
e.stopPropagation();
e.preventDefault();
};
button.addEventListener('mousedown', handleMouseDown);
return () => {
button.removeEventListener('mousedown', handleMouseDown);
};
}
}, []);


return (
<div
style={{
padding: 100,
display: 'flex',
flexDirection: 'column',
alignItems: 'flex-start',
gap: 100,
}}
>
<Trigger
popupPlacement="right"
action={['click']}
builtinPlacements={builtinPlacements}
popup={
<div style={popupBorderStyle}>
i am a click popup
<PortalPopup />
</div>
}
onPopupVisibleChange={(visible) => {
console.log('visible change:', visible);
}}
>
<button>Click Me</button>
</Trigger>
Comment on lines +79 to +94
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议统一事件处理方式

目前组件中混合使用了多种事件处理方式(Trigger props、React 事件、原生事件),建议统一处理方式以提高可维护性。

建议将所有事件处理统一到 React 的方式:

 <Trigger
   popupPlacement="right"
   action={['click']}
   builtinPlacements={builtinPlacements}
+  destroyPopupOnHide
   popup={
     <div style={popupBorderStyle}>
       i am a click popup
       <PortalPopup />
     </div>
   }
   onPopupVisibleChange={(visible) => {
     console.log('visible change:', visible);
+    // 在这里统一处理弹窗状态
   }}
 >
-  <button>Click Me</button>
+  <button onClick={(e) => {
+    // 统一的点击处理
+    console.log('trigger clicked');
+  }}>
+    Click Me
+  </button>
 </Trigger>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Trigger
popupPlacement="right"
action={['click']}
builtinPlacements={builtinPlacements}
popup={
<div style={popupBorderStyle}>
i am a click popup
<PortalPopup />
</div>
}
onPopupVisibleChange={(visible) => {
console.log('visible change:', visible);
}}
>
<button>Click Me</button>
</Trigger>
<Trigger
popupPlacement="right"
action={['click']}
builtinPlacements={builtinPlacements}
destroyPopupOnHide
popup={
<div style={popupBorderStyle}>
i am a click popup
<PortalPopup />
</div>
}
onPopupVisibleChange={(visible) => {
console.log('visible change:', visible);
// 在这里统一处理弹窗状态
}}
>
<button onClick={(e) => {
// 统一的点击处理
console.log('trigger clicked');
}}>
Click Me
</button>
</Trigger>


<button
onMouseDown={(e) => {
console.log('button down');
e.stopPropagation();
e.preventDefault();
}}
>
Stop Pop & Prevent Default
</button>
<button ref={buttonRef}>Native Stop Pop & Prevent Default</button>
</div>
);
};

export default Test;
3 changes: 3 additions & 0 deletions src/Popup/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export interface PopupProps {
onMouseEnter?: React.MouseEventHandler<HTMLDivElement>;
onMouseLeave?: React.MouseEventHandler<HTMLDivElement>;
onPointerEnter?: React.MouseEventHandler<HTMLDivElement>;
onMouseDownCapture?: React.MouseEventHandler<HTMLDivElement>;
zIndex?: number;

mask?: boolean;
Expand Down Expand Up @@ -105,6 +106,7 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => {
onMouseEnter,
onMouseLeave,
onPointerEnter,
onMouseDownCapture,

ready,
offsetX,
Expand Down Expand Up @@ -255,6 +257,7 @@ const Popup = React.forwardRef<HTMLDivElement, PopupProps>((props, ref) => {
onMouseLeave={onMouseLeave}
onPointerEnter={onPointerEnter}
onClick={onClick}
onMouseDownCapture={onMouseDownCapture}
>
{arrow && (
<Arrow
Expand Down
12 changes: 11 additions & 1 deletion src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,9 @@ export function generateTrigger(
React.useState<VoidFunction>(null);

// =========================== Align ============================
const [mousePos, setMousePos] = React.useState<[x: number, y: number] | null>(null);
const [mousePos, setMousePos] = React.useState<
[x: number, y: number] | null
>(null);

const setMousePosByEvent = (
event: Pick<React.MouseEvent, 'clientX' | 'clientY'>,
Expand Down Expand Up @@ -720,6 +722,14 @@ export function generateTrigger(
fresh={fresh}
// Click
onClick={onPopupClick}
onMouseDownCapture={() => {
// Additional check for click to hide
// Since `createPortal` will not included in the popup element
// So we use capture to handle this
if (clickToHide) {
triggerOpen(true);
}
}}
// Mask
mask={mask}
// Motion
Expand Down
35 changes: 33 additions & 2 deletions tests/basic.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { act, cleanup, fireEvent, render } from '@testing-library/react';
import { spyElementPrototypes } from 'rc-util/lib/test/domHook';
import React, { StrictMode, createRef } from 'react';
import ReactDOM from 'react-dom';
import ReactDOM, { createPortal } from 'react-dom';
import Trigger from '../src';
import { awaitFakeTimer, placementAlignMap } from './util';

Expand Down Expand Up @@ -107,7 +107,7 @@ describe('Trigger.Basic', () => {
expect(document.querySelector('.x-content').textContent).toBe('tooltip2');

trigger(container, '.target');
expect(isPopupHidden).toBeTruthy();
expect(isPopupHidden()).toBeTruthy();
});

it('click works with function', () => {
Expand Down Expand Up @@ -1198,4 +1198,35 @@ describe('Trigger.Basic', () => {
trigger(container, '.target');
expect(document.querySelector('.x-content').textContent).toBe('false');
});

it('createPortal should not close', async () => {
const Portal = () =>
createPortal(<div className="portal" />, document.body);

const Demo = () => {
return (
<>
<Trigger action="click" popup={<Portal />}>
<div className="target" />
</Trigger>
<div className="outer" />
</>
);
};

const { container } = render(<Demo />);
fireEvent.click(container.querySelector('.target'));
await awaitFakeTimer();
expect(isPopupHidden()).toBeFalsy();

// Click portal should not close
fireEvent.click(document.querySelector('.portal'));
await awaitFakeTimer();
expect(isPopupHidden()).toBeFalsy();

// Click outside to close
fireEvent.mouseDown(container.querySelector('.outer'));
await awaitFakeTimer();
expect(isPopupHidden()).toBeTruthy();
});
});
Loading