-
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 16 commits
13eafc2
64a2d76
8bf7085
2ed61d1
522b02e
c9fbf32
c93b460
9bcbdf4
2d6fc45
70a2394
9ac2c05
aef324f
2d2649d
77c36bf
f20c922
d0638df
47bc548
d492b1b
3bcc076
04cb377
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 be placed." |
||
| * | ||
| * @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,20 +280,31 @@ 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)), []); | ||
|
|
||
| return ( | ||
| <Overlay isExiting={props.isExiting} portalContainer={UNSTABLE_portalContainer}> | ||
| <dom.div | ||
| {...mergeProps(filterDOMProps(props, {global: true}), underlayProps)} | ||
| {...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] | ||
| ]}> | ||
|
|
@@ -299,16 +328,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 | ||
|
|
@@ -320,6 +350,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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| export { | ||
| isCtrlKeyPressed, | ||
| isKeyboardOpen, | ||
| isKeyboardVisible, | ||
| willOpenKeyboard | ||
| } from '../../../src/utils/isKeyboardVisible'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,6 @@ | ||
| export {isCtrlKeyPressed, willOpenKeyboard} from '../../../src/utils/keyboard'; | ||
| export { | ||
|
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. why the double up on this file? they appear to be the same and are adding to noise in this large PR can we just keep the old one? should cut down on some more noise and prevent a new private subpath
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. No strong reason. I mostly just wanted to rename source file to isKeyboardVisible to align with other file naming in utils. I left the keyboard export file as is and pointed at the new source file to not break anything, but if you dont want to rename we can also just keep the file name as it was or otherwise defer into a chore followup. |
||
| isCtrlKeyPressed, | ||
| isKeyboardOpen, | ||
| isKeyboardVisible, | ||
| willOpenKeyboard | ||
| } from '../../../src/utils/isKeyboardVisible'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export {runAfterKeyboard, runAfterKeyboardTransition} from '../../../src/utils/runAfterKeyboard'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,7 @@ export function useDisclosure( | |
| }); | ||
| }, [ref, state]); | ||
|
|
||
| // @ts-ignore https://github.com/facebook/react/pull/24741 | ||
| // https://github.com/facebook/react/pull/24741 | ||
|
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. irrelevant |
||
| useEvent(ref, 'beforematch', handleBeforeMatch); | ||
|
|
||
| let isExpandedRef = useRef<boolean | null>(null); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -337,8 +337,9 @@ function getMaxHeight( | |
| (position.top != null | ||
| ? position.top | ||
| : containerDimensions[TOTAL_SIZE.height] - (position.bottom ?? 0) - overlayHeight) - | ||
| (containerDimensions.scroll.top ?? 0); | ||
| // calculate the dimentions of the "boundingRect" which is most restrictive top/bottom of the boundaryRect and the visual view port | ||
| (containerDimensions.scroll.top ?? 0) + | ||
| (visualViewport?.offsetTop ?? 0); | ||
|
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. @snowystinger This was the core culprit to the positional issue. A scroll into view motion may divide delta between container scroll and viewport scroll, but only the former was accounted for. See this animation for an explainer https://www.w3.org/TR/cssom-view-1/#example-vvanimation |
||
| // calculate the dimensions of the "boundingRect" which is most restrictive top/bottom of the boundaryRect and the visual view port | ||
| let boundaryToContainerTransformOffset = isContainerDescendentOfBoundary | ||
| ? containerOffsetWithBoundary.top | ||
| : 0; | ||
|
|
||
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?