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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

fix: invalid autofocus #361

wants to merge 6 commits into from

Conversation

acyza
Copy link

@acyza acyza commented Mar 20, 2023

fix ant-design/ant-design#41239
link #350
react中的autoFocus似乎是react模拟的,并非原生的autofocus。https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js#L522-L528
rc-potral默认的挂载点似乎不太稳定,有可能虚拟dom渲染的时候挂载点还没添加到页面,或者添加后又被移除(visible为false的情况),导致autofocus失效。

@vercel
Copy link

vercel bot commented Mar 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
dialog ❌ Failed (Inspect) Mar 21, 2023 at 1:19AM (UTC)

src/DialogWrap.tsx Outdated Show resolved Hide resolved
@acyza
Copy link
Author

acyza commented Mar 21, 2023

似乎原生dialog关闭重新打开焦点会回到autofocus的元素上,所以上面那个commit是根据第一次打开的焦点来模拟这个特性。

@acyza acyza requested a review from yoyo837 March 21, 2023 00:50
src/Dialog/index.tsx Outdated Show resolved Hide resolved
@@ -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

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

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.

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

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.

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

@acyza acyza requested a review from yoyo837 March 21, 2023 01:48
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #361 (f8eda32) into master (646efc7) will decrease coverage by 0.51%.
The diff coverage is 88.88%.

❗ Current head f8eda32 differs from pull request most recent head a2ab585. Consider uploading reports for the commit a2ab585 to get more accurate results

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   98.12%   97.61%   -0.51%     
==========================================
  Files           7        7              
  Lines         160      168       +8     
  Branches       50       54       +4     
==========================================
+ Hits          157      164       +7     
- Misses          2        3       +1     
  Partials        1        1              
Impacted Files Coverage Δ
src/Dialog/index.tsx 97.26% <83.33%> (-1.27%) ⬇️
src/DialogWrap.tsx 100.00% <100.00%> (ø)
src/util.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zombieJ
Copy link
Member

zombieJ commented Mar 22, 2023

@acyza 加个群?

@acyza acyza marked this pull request as draft March 22, 2023 12:35
@acyza acyza marked this pull request as ready for review March 23, 2023 22:55
@yoyo837 yoyo837 marked this pull request as draft March 24, 2023 08:10
@neongreen
Copy link

Hi, any news for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The autoFocus property doesn't work for the Input component inside the Modal
4 participants