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.

Sign up for GitHub

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: invalid autofocus #361

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+14 −5
Draft

fix: invalid autofocus #361

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
568ac95
fix: invalid autofocus
acyza Mar 20, 2023
3de72fd
fix: test fail
acyza Mar 20, 2023
5e11aa6
chore: update
acyza Mar 20, 2023
6d0f7bb
chore: update
acyza Mar 21, 2023
da7a732
chore: update
acyza Mar 21, 2023
a2ab585
chore: update
acyza Mar 21, 2023
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
6 changes: 5 additions & 1 deletion src/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default function Dialog(props: IDialogPropTypes) {
const contentRef = useRef<ContentRef>();

const [animatedVisible, setAnimatedVisible] = React.useState(visible);
const [originFocusEl, setOriginFocusEl] = React.useState<HTMLElement>();

// ========================== Init ==========================
const ariaId = useId();
Expand All @@ -59,7 +60,10 @@ export default function Dialog(props: IDialogPropTypes) {
}

function focusDialogContent() {
if (!contains(wrapperRef.current, document.activeElement)) {
if (originFocusEl) return originFocusEl.focus();
acyza marked this conversation as resolved.
Show resolved Hide resolved
if (contains(wrapperRef.current, document.activeElement)) {
setOriginFocusEl(document.activeElement as HTMLElement);
} else {
contentRef.current?.focus();
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/DialogWrap.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as React from 'react';
import Portal from '@rc-component/portal';
import * as React from 'react';
import Dialog from './Dialog';
import type { IDialogPropTypes } from './IDialogPropTypes';
import { isNil } from './util';

// fix issue #10656
/*
Expand Down Expand Up @@ -41,7 +42,7 @@ const DialogWrap: React.FC<IDialogPropTypes> = (props: IDialogPropTypes) => {
<Portal
open={visible || forceRender || animatedVisible}
autoDestroy={false}
getContainer={getContainer}
getContainer={isNil(getContainer) ? 'body' : getContainer}
Copy link
Member

@yoyo837 yoyo837 Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This should not be a necessary change, Portal will check it's correct internally.

Copy link
Author

@acyza acyza Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

rc-potral默认的挂载点似乎不太稳定，有可能虚拟dom渲染的时候挂载点还没添加到页面，或者添加后又被移除（visible为false的情况），导致autofocus失效。
这里相当于改用body作为默认挂载点。

Copy link
Author

Choose a reason for hiding this comment

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

相关代码：https://github.com/react-component/portal/blob/master/src/useDom.tsx#L64-L71

Copy link
Member

Choose a reason for hiding this comment

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

那是不是分两个问题修复? 后者在 Portal 修复?

Copy link
Author

@acyza acyza Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

portal好像不太好修，可以把这个当作临时方案。

Copy link
Member

Choose a reason for hiding this comment

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

@zombieJ 呼叫巨佬

Copy link
Member

Choose a reason for hiding this comment

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

我也觉得这是两件事。这个 PR 应该只管 Input focus 的问题。

Copy link
Author

Choose a reason for hiding this comment

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

也许得先优化一下portal
https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js#L522-L528
看出react的autofocus是用js模拟的，如何让react在https://github.com/react-component/portal/blob/master/src/useDom.tsx#L68 append之后模拟autofocus?

Copy link
Author

Choose a reason for hiding this comment

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

也没法用querySelector('[autofocus]').focus()，react没给原生input加autofocus。图片
图片

Copy link
Member

Choose a reason for hiding this comment

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

拉你进群了，这种需要沟通比较多的问题到群里沟通吧

autoLock={visible || animatedVisible}
>
<Dialog
Expand Down
4 changes: 4 additions & 0 deletions src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,7 @@ export function offset(el: Element) {
pos.top += getScroll(w, true);
return pos;
}

export function isNil(val: any) {
return val === undefined || val === null;
}
3 changes: 1 addition & 2 deletions tests/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,7 @@ describe('dialog', () => {
expect(wrapper.find('.rc-dialog-footer').text()).toBe('test');
});

// 失效了，需要修复
it.skip('support input autoFocus', () => {
it('support input autoFocus', () => {
render(
<Dialog visible>
<input autoFocus />
Expand Down