-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: overlay layout shift when auto-focusing input #10102
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
base: main
Are you sure you want to change the base?
Changes from all commits
13eafc2
64a2d76
8bf7085
2ed61d1
522b02e
c9fbf32
c93b460
9bcbdf4
2d6fc45
70a2394
9ac2c05
aef324f
2d2649d
77c36bf
f20c922
d0638df
47bc548
d492b1b
3bcc076
04cb377
939c182
09afbff
c71dc1d
e98643c
760df6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,6 @@ | |
| */ | ||
|
|
||
| import {AriaModalOverlayProps, useModalOverlay} from 'react-aria/useModalOverlay'; | ||
|
|
||
| import { | ||
| ClassNameOrFunction, | ||
| ContextValue, | ||
|
|
@@ -34,9 +33,19 @@ import { | |
| useOverlayTriggerState | ||
| } from 'react-stately/useOverlayTriggerState'; | ||
| import {OverlayTriggerStateContext} from './Dialog'; | ||
| import React, {createContext, ForwardedRef, forwardRef, useContext, useMemo, useRef} from 'react'; | ||
| import React, { | ||
| createContext, | ||
| ForwardedRef, | ||
| forwardRef, | ||
| useContext, | ||
| useMemo, | ||
| useRef, | ||
| useState | ||
| } from 'react'; | ||
| import {runAfterKeyboard} from 'react-aria/private/utils/runAfterKeyboard'; | ||
| import {useEnterAnimation, useExitAnimation} from 'react-aria/private/utils/animation'; | ||
| import {useIsSSR} from 'react-aria/SSRProvider'; | ||
| import {useLayoutEffect} from 'react-aria/private/utils/useLayoutEffect'; | ||
| import {useObjectRef} from 'react-aria/useObjectRef'; | ||
| import {useViewportSize} from 'react-aria/private/utils/useViewportSize'; | ||
|
|
||
|
|
@@ -75,6 +84,7 @@ export interface ModalOverlayProps | |
| interface InternalModalContextValue { | ||
| modalProps: DOMAttributes; | ||
| modalRef: RefObject<HTMLDivElement | null>; | ||
| isOpen: boolean; | ||
| isExiting: boolean; | ||
| isDismissable?: boolean; | ||
| } | ||
|
|
@@ -83,6 +93,12 @@ export const ModalContext = createContext<ContextValue<ModalOverlayProps, HTMLDi | |
| const InternalModalContext = createContext<InternalModalContextValue | null>(null); | ||
|
|
||
| export interface ModalRenderProps { | ||
| /** | ||
| * Whether the modal is ready to be displayed. Use this to avoid layout shift. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc is a little particular to the issue you're solving, but not really a great example of when to use without the knowledge of this PR and Issue. Have any ideas how we can improve this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestions welcome 😅 Maybe something like ”[…] Use this to hide the modal while it is not yet ready to enter." |
||
| * | ||
| * @selector [data-open] | ||
| */ | ||
| isOpen: boolean; | ||
| /** | ||
| * Whether the modal is currently entering. Use this to apply animations. | ||
| * | ||
|
|
@@ -229,11 +245,13 @@ function ModalOverlayInner({UNSTABLE_portalContainer, ...props}: ModalOverlayInn | |
| let {state} = props; | ||
| let {modalProps, underlayProps} = useModalOverlay(props, state, modalRef); | ||
|
|
||
| let entering = useEnterAnimation(props.overlayRef) || props.isEntering || false; | ||
| let [isOpen, setIsOpen] = useState(false); | ||
| let entering = useEnterAnimation(props.overlayRef, isOpen) || props.isEntering || false; | ||
| let renderProps = useRenderProps({ | ||
| ...props, | ||
| defaultClassName: 'react-aria-ModalOverlay', | ||
| values: { | ||
| isOpen, | ||
| isEntering: entering, | ||
| isExiting: props.isExiting, | ||
| state | ||
|
|
@@ -262,6 +280,10 @@ function ModalOverlayInner({UNSTABLE_portalContainer, ...props}: ModalOverlayInn | |
| '--page-height': pageHeight !== undefined ? pageHeight + 'px' : undefined | ||
| }; | ||
|
|
||
| // Since an auto-focused input may open the OSK, we defer the reveal, as a courtesy, to avoid layout shift. | ||
| // TODO: This can cause native focus scroll-into-view to abort, so we might want to do that manually? | ||
| useLayoutEffect(() => runAfterKeyboard(() => setIsOpen(true)), []); | ||
|
|
||
| // oxlint-disable react/react-compiler | ||
| return ( | ||
| <Overlay isExiting={props.isExiting} portalContainer={UNSTABLE_portalContainer}> | ||
|
|
@@ -270,13 +292,20 @@ function ModalOverlayInner({UNSTABLE_portalContainer, ...props}: ModalOverlayInn | |
| {...renderProps} | ||
| style={style} | ||
| ref={props.overlayRef} | ||
| data-open={isOpen || undefined} | ||
| data-entering={entering || undefined} | ||
| data-exiting={props.isExiting || undefined}> | ||
| <Provider | ||
| values={[ | ||
| [ | ||
| InternalModalContext, | ||
| {modalProps, modalRef, isExiting: props.isExiting, isDismissable: props.isDismissable} | ||
| { | ||
| modalProps, | ||
| modalRef, | ||
| isExiting: props.isExiting, | ||
| isOpen, | ||
| isDismissable: props.isDismissable | ||
| } | ||
| ], | ||
| [OverlayTriggerStateContext, state] | ||
| ]}> | ||
|
|
@@ -301,16 +330,17 @@ interface ModalContentProps | |
| } | ||
|
|
||
| function ModalContent(props: ModalContentProps) { | ||
| let {modalProps, modalRef, isExiting, isDismissable} = useContext(InternalModalContext)!; | ||
| let {modalProps, modalRef, isExiting, isOpen, isDismissable} = useContext(InternalModalContext)!; | ||
| let state = useContext(OverlayTriggerStateContext)!; | ||
| let mergedRefs = useMemo(() => mergeRefs(props.modalRef, modalRef), [props.modalRef, modalRef]); | ||
|
|
||
| let ref = useObjectRef(mergedRefs); | ||
| let entering = useEnterAnimation(ref); | ||
| let entering = useEnterAnimation(ref, isOpen); | ||
| let renderProps = useRenderProps({ | ||
| ...props, | ||
| defaultClassName: 'react-aria-Modal', | ||
| values: { | ||
| isOpen, | ||
| isEntering: entering, | ||
| isExiting, | ||
| state | ||
|
|
@@ -322,6 +352,7 @@ function ModalContent(props: ModalContentProps) { | |
| {...mergeProps(filterDOMProps(props, {global: true}), modalProps)} | ||
| {...renderProps} | ||
| ref={ref} | ||
| data-open={isOpen || undefined} | ||
| data-entering={entering || undefined} | ||
| data-exiting={isExiting || undefined}> | ||
| {isDismissable && <DismissButton onDismiss={state.close} />} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,6 @@ | ||
| export {isCtrlKeyPressed, willOpenKeyboard} from '../../../src/utils/keyboard'; | ||
| export { | ||
|
nwidynski marked this conversation as resolved.
|
||
| isCtrlKeyPressed, | ||
| isKeyboardOpen, | ||
| isKeyboardVisible, | ||
| willOpenKeyboard | ||
| } from '../../../src/utils/keyboard'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export {runAfterKeyboard, runAfterKeyboardTransition} from '../../../src/utils/runAfterKeyboard'; |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly concerned about user land style, when transitions are used for enter animation. Now that entering is delayed by at least a frame, users should be adding something like this to avoid a flash of content. This would be an existing issue in our Popover example styling as well, due opacity not being 0 while awaiting placement, if it were not for the early resize in Popover's layout effect.
I haven't thought of a way to avoid this without forcing an opinionated hidden inline style. Maybe somebody else got ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will think about this more now that we have a concrete example of what happens #10102 (comment)
I think as it is, we'd consider this breaking
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured. I added back the hidden styling for now until someone has a better idea. Kinda makes the data attribute obsolete, although there may be an argument to keep it, to encourage migration to safe styling.
You can test that this gets the job done by removing the changes in
Modal.cssand verifying that no flash of content is happening in/react-aria/Modal.html