From 372bb8f206bc37572f24c51aff0917799c8bbbc6 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 09:18:14 +0100 Subject: [PATCH 01/17] feat: Updates ref update handling in `usePositioning` Updates the ref handling done in `usePositioning` so that scroll listeners are only added when both container, target elements are set correctly and the behaviour is `enabled`. This ensures that the browser reflow caused by `getScrollParent` is called as little as possible. Also updates `usePortalMountNode` to create all the attributes of the mount node during the render phase in `useMemo` to avoid any css variable changes during render time which can cause strange styling bugs due to changes in CSS variables. Since our portals are already broken in react 18 strict mode, we're not making that worse with these changes --- .../src/components/Button/useButtonStyles.ts | 2 +- .../components/Portal/usePortalMountNode.ts | 33 +-- .../react-positioning/src/types.ts | 29 ++- .../react-positioning/src/usePositioning.ts | 221 +++++++----------- .../react-positioning/src/utils/index.ts | 2 + .../src/utils/writeArrowUpdates.ts | 18 ++ .../src/utils/writeContainerupdates.ts | 56 +++++ 7 files changed, 195 insertions(+), 166 deletions(-) create mode 100644 packages/react-components/react-positioning/src/utils/writeArrowUpdates.ts create mode 100644 packages/react-components/react-positioning/src/utils/writeContainerupdates.ts diff --git a/packages/react-components/react-button/src/components/Button/useButtonStyles.ts b/packages/react-components/react-button/src/components/Button/useButtonStyles.ts index 148daa26a7d2d8..aff3303e42d925 100644 --- a/packages/react-components/react-button/src/components/Button/useButtonStyles.ts +++ b/packages/react-components/react-button/src/components/Button/useButtonStyles.ts @@ -70,7 +70,7 @@ const useRootStyles = makeStyles({ // Transition styles transition: { - transitionDuration: '100ms', + transitionDuration: '2000ms', transitionProperty: 'background, border, color', transitionTimingFunction: 'cubic-bezier(0.33, 0, 0.67, 1)', diff --git a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts index 89558727cc0b94..4f91a8b9100a74 100644 --- a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts +++ b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts @@ -1,5 +1,4 @@ import * as React from 'react'; -import { useIsomorphicLayoutEffect } from '@fluentui/react-utilities'; import { useThemeClassName_unstable as useThemeClassName, useFluent_unstable as useFluent, @@ -33,37 +32,29 @@ export const usePortalMountNode = (options: UsePortalMountNodeOptions): HTMLElem const className = mergeClasses(themeClassName, classes.root); - const element = React.useMemo(() => { + const mountNode = React.useMemo(() => { if (targetDocument === undefined || options.disabled) { return null; } - const newElement = targetDocument.createElement('div'); - targetDocument.body.appendChild(newElement); + const element = targetDocument.createElement('div'); + focusVisibleRef.current = element; - return newElement; - }, [targetDocument, options.disabled]); + const classesToApply = className.split(' ').filter(Boolean); + element.classList.add(...classesToApply); - useIsomorphicLayoutEffect(() => { - if (element) { - const classesToApply = className.split(' ').filter(Boolean); + element.setAttribute('dir', dir); - element.classList.add(...classesToApply); - element.setAttribute('dir', dir); - focusVisibleRef.current = element; + targetDocument.body.appendChild(element); - return () => { - element.classList.remove(...classesToApply); - element.removeAttribute('dir'); - }; - } - }, [className, dir, element, focusVisibleRef]); + return element; + }, [targetDocument, options.disabled, focusVisibleRef, className, dir]); React.useEffect(() => { return () => { - element?.parentElement?.removeChild(element); + mountNode?.remove(); }; - }, [element]); + }, [mountNode]); - return element; + return mountNode; }; diff --git a/packages/react-components/react-positioning/src/types.ts b/packages/react-components/react-positioning/src/types.ts index 7e5d84a69c5a1a..c9f233f4527b31 100644 --- a/packages/react-components/react-positioning/src/types.ts +++ b/packages/react-components/react-positioning/src/types.ts @@ -14,6 +14,31 @@ export type OffsetFunctionParam = { alignment?: Alignment; }; +export type TargetType = HTMLElement | PositioningVirtualElement | null; + +/** + * @internal + */ +export interface UsePositioningOptions extends PositioningProps { + /** + * If false, does not position anything + */ + enabled?: boolean; +} + +export interface UsePositioningReturn { + // React refs are supposed to be contravariant + // (allows a more general type to be passed rather than a more specific one) + // However, Typescript currently can't infer that fact for refs + // See https://github.com/microsoft/TypeScript/issues/30748 for more information + // eslint-disable-next-line @typescript-eslint/no-explicit-any + targetRef: React.MutableRefObject; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + containerRef: React.MutableRefObject; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + arrowRef: React.MutableRefObject; +} + export type OffsetObject = { crossAxis?: number; mainAxis: number }; export type OffsetShorthand = number; @@ -40,7 +65,7 @@ export type PositioningImperativeRef = { * Sets the target and updates positioning imperatively. * Useful for avoiding double renders with the target option. */ - setTarget: (target: HTMLElement | PositioningVirtualElement) => void; + setTarget: (target: TargetType) => void; }; export type PositioningVirtualElement = { @@ -133,7 +158,7 @@ export interface PositioningProps /** * Manual override for the target element. Useful for scenarios where a component accepts user prop to override target */ - target?: HTMLElement | PositioningVirtualElement | null; + target?: TargetType; } export type PositioningShorthandValue = diff --git a/packages/react-components/react-positioning/src/usePositioning.ts b/packages/react-components/react-positioning/src/usePositioning.ts index 5c054c6d14f928..c5371d81d8972b 100644 --- a/packages/react-components/react-positioning/src/usePositioning.ts +++ b/packages/react-components/react-positioning/src/usePositioning.ts @@ -1,17 +1,25 @@ import { computePosition, hide as hideMiddleware, arrow as arrowMiddleware } from '@floating-ui/dom'; -import type { Middleware, Strategy, Placement, Coords, MiddlewareData } from '@floating-ui/dom'; +import type { Middleware, Strategy } from '@floating-ui/dom'; import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts'; import { canUseDOM, useIsomorphicLayoutEffect } from '@fluentui/react-utilities'; import { useEventCallback } from '@fluentui/react-utilities'; import * as React from 'react'; -import type { PositioningOptions, PositioningProps, PositioningVirtualElement } from './types'; +import type { + PositioningOptions, + PositioningVirtualElement, + TargetType, + UsePositioningOptions, + UsePositioningReturn, +} from './types'; import { useCallbackRef, toFloatingUIPlacement, - toggleScrollListener, hasAutofocusFilter, debounce, hasScrollParent, + getScrollParent, + writeArrowUpdates, + writeContainerUpdates, } from './utils'; import { shift as shiftMiddleware, @@ -21,46 +29,27 @@ import { offset as offsetMiddleware, intersecting as intersectingMiddleware, } from './middleware'; -import { - DATA_POSITIONING_ESCAPED, - DATA_POSITIONING_INTERSECTING, - DATA_POSITIONING_HIDDEN, - DATA_POSITIONING_PLACEMENT, -} from './constants'; /** * @internal */ -export function usePositioning( - options: UsePositioningOptions, -): { - // React refs are supposed to be contravariant - // (allows a more general type to be passed rather than a more specific one) - // However, Typescript currently can't infer that fact for refs - // See https://github.com/microsoft/TypeScript/issues/30748 for more information - // eslint-disable-next-line @typescript-eslint/no-explicit-any - targetRef: React.MutableRefObject; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - containerRef: React.MutableRefObject; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - arrowRef: React.MutableRefObject; -} { +export function usePositioning(options: UsePositioningOptions): UsePositioningReturn { + const containerRef = React.useRef(null); + const targetRef = React.useRef(null); + const arrowRef = React.useRef(null); + const { targetDocument } = useFluent(); + const { enabled = true } = options; const resolvePositioningOptions = usePositioningOptions(options); + const { placement, middleware, strategy } = resolvePositioningOptions(containerRef.current, arrowRef.current); const forceUpdate = useEventCallback(() => { - const target = overrideTargetRef.current ?? targetRef.current; + const target = targetRef.current; if (!canUseDOM || !enabled || !target || !containerRef.current) { return; } - const { placement, middleware, strategy } = resolvePositioningOptions( - target, - containerRef.current, - arrowRef.current, - ); - // Container is always initialized with `position: fixed` to avoid scroll jumps // Before computing the positioned coordinates, revert the container to the deisred positioning strategy Object.assign(containerRef.current.style, { position: strategy }); @@ -93,16 +82,51 @@ export function usePositioning( const updatePosition = React.useState(() => debounce(forceUpdate))[0]; - const targetRef = useTargetRef(updatePosition); - const overrideTargetRef = useTargetRef(updatePosition); - const containerRef = useContainerRef(updatePosition, enabled); - const arrowRef = useArrowRef(updatePosition); + const cleanupScrollListenersRef = React.useRef<() => void>(() => null); + const handleRefUpdate = React.useCallback(() => { + if (containerRef.current) { + // When the container is first resolved, set position `fixed` to avoid scroll jumps. + // Without this scroll jumps can occur when the element is rendered initially and receives focus + Object.assign(containerRef.current.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); + } + + cleanupScrollListenersRef.current(); + const scrollParents: Set = new Set(); + if (enabled && containerRef.current && targetRef.current) { + // `getScrollParent` will cause reflow, running it when when enabled, container and target + // are all correctly set will make sure that it is run as little as possible + scrollParents.add(getScrollParent(containerRef.current)); + if (targetRef.current instanceof HTMLElement) { + scrollParents.add(getScrollParent(targetRef.current)); + } + + scrollParents.forEach(scrollParent => { + scrollParent.addEventListener('scroll', updatePosition); + }); + + cleanupScrollListenersRef.current = () => { + scrollParents.forEach(scrollParent => { + scrollParent.removeEventListener('scroll', updatePosition); + }); + }; + } + + updatePosition(); + }, [enabled, updatePosition]); + + const overrideTarget = React.useCallback( + (target: TargetType) => { + targetRef.current = target; + handleRefUpdate(); + }, + [handleRefUpdate], + ); React.useImperativeHandle( options.positioningRef, () => ({ updatePosition, - setTarget: (target: HTMLElement | PositioningVirtualElement) => { + setTarget: (target: TargetType) => { if (options.target && process.env.NODE_ENV !== 'production') { const err = new Error(); // eslint-disable-next-line no-console @@ -111,7 +135,7 @@ export function usePositioning( console.warn(err.stack); } - overrideTargetRef.current = target; + overrideTarget(target); }, }), // Missing deps: @@ -123,8 +147,10 @@ export function usePositioning( ); useIsomorphicLayoutEffect(() => { - overrideTargetRef.current = options.target ?? null; - }, [options.target, overrideTargetRef, containerRef]); + if (options.target) { + overrideTarget(options.target); + } + }, [options.target, overrideTarget]); useIsomorphicLayoutEffect(() => { updatePosition(); @@ -186,14 +212,21 @@ export function usePositioning( }, []); } - return { targetRef, containerRef, arrowRef }; -} + const setTargetElement = useCallbackRef(null, target => { + targetRef.current = target; + handleRefUpdate(); + }); + const setContainerElement = useCallbackRef(null, container => { + containerRef.current = container; + handleRefUpdate(); + }); + const setArrowElement = useCallbackRef(null, arrow => { + containerRef.current = arrow; + handleRefUpdate(); + }); -interface UsePositioningOptions extends PositioningProps { - /** - * If false, does not position anything - */ - enabled?: boolean; + // Users should consume callback refs so they can set them like 'standard' HTML refs + return { targetRef: setTargetElement, containerRef: setContainerElement, arrowRef: setArrowElement }; } function usePositioningOptions(options: PositioningOptions) { @@ -216,11 +249,7 @@ function usePositioningOptions(options: PositioningOptions) { const strategy: Strategy = positionFixed ? 'fixed' : 'absolute'; return React.useCallback( - ( - target: HTMLElement | PositioningVirtualElement | null, - container: HTMLElement | null, - arrow: HTMLElement | null, - ) => { + (container: HTMLElement | null, arrow: HTMLElement | null) => { const hasScrollableElement = hasScrollParent(container); const placement = toFloatingUIPlacement(align, position, isRtl); @@ -258,95 +287,3 @@ function usePositioningOptions(options: PositioningOptions) { ], ); } - -function useContainerRef(updatePosition: () => void, enabled: boolean) { - return useCallbackRef(null, (container, prevContainer) => { - if (container && enabled) { - // When the container is first resolved, set position `fixed` to avoid scroll jumps. - // Without this scroll jumps can occur when the element is rendered initially and receives focus - Object.assign(container.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); - } - - toggleScrollListener(container, prevContainer, updatePosition); - - updatePosition(); - }); -} - -function useTargetRef(updatePosition: () => void) { - return useCallbackRef(null, (target, prevTarget) => { - toggleScrollListener(target, prevTarget, updatePosition); - - updatePosition(); - }); -} - -function useArrowRef(updatePosition: () => void) { - return useCallbackRef(null, updatePosition); -} - -/** - * Writes all DOM element updates after position is computed - */ -function writeContainerUpdates(options: { - container: HTMLElement | null; - placement: Placement; - middlewareData: MiddlewareData; - /** - * Layer acceleration can disable subpixel rendering which causes slightly - * blurry text on low PPI displays, so we want to use 2D transforms - * instead - */ - lowPPI: boolean; - strategy: Strategy; - coordinates: Coords; -}) { - const { - container, - placement, - middlewareData, - strategy, - lowPPI, - coordinates: { x, y }, - } = options; - if (!container) { - return; - } - container.setAttribute(DATA_POSITIONING_PLACEMENT, placement); - container.removeAttribute(DATA_POSITIONING_INTERSECTING); - if (middlewareData.intersectionObserver.intersecting) { - container.setAttribute(DATA_POSITIONING_INTERSECTING, ''); - } - - container.removeAttribute(DATA_POSITIONING_ESCAPED); - if (middlewareData.hide?.escaped) { - container.setAttribute(DATA_POSITIONING_ESCAPED, ''); - } - - container.removeAttribute(DATA_POSITIONING_HIDDEN); - if (middlewareData.hide?.referenceHidden) { - container.setAttribute(DATA_POSITIONING_HIDDEN, ''); - } - - Object.assign(container.style, { - transform: lowPPI ? `translate(${x}px, ${y}px)` : `translate3d(${x}px, ${y}px, 0)`, - position: strategy, - }); -} - -/** - * Writes all DOM element updates after position is computed - */ -function writeArrowUpdates(options: { arrow: HTMLElement | null; middlewareData: MiddlewareData }) { - const { arrow, middlewareData } = options; - if (!middlewareData.arrow || !arrow) { - return; - } - - const { x: arrowX, y: arrowY } = middlewareData.arrow; - - Object.assign(arrow.style, { - left: `${arrowX}px`, - top: `${arrowY}px`, - }); -} diff --git a/packages/react-components/react-positioning/src/utils/index.ts b/packages/react-components/react-positioning/src/utils/index.ts index f8d4270df5536b..8b4f71faaf70fb 100644 --- a/packages/react-components/react-positioning/src/utils/index.ts +++ b/packages/react-components/react-positioning/src/utils/index.ts @@ -10,3 +10,5 @@ export * from './useCallbackRef'; export * from './debounce'; export * from './toggleScrollListener'; export * from './hasAutoFocusFilter'; +export * from './writeArrowUpdates'; +export * from './writeContainerupdates'; diff --git a/packages/react-components/react-positioning/src/utils/writeArrowUpdates.ts b/packages/react-components/react-positioning/src/utils/writeArrowUpdates.ts new file mode 100644 index 00000000000000..87b1ebc30c7baa --- /dev/null +++ b/packages/react-components/react-positioning/src/utils/writeArrowUpdates.ts @@ -0,0 +1,18 @@ +import { MiddlewareData } from '@floating-ui/dom'; + +/** + * Writes all DOM element updates after position is computed + */ +export function writeArrowUpdates(options: { arrow: HTMLElement | null; middlewareData: MiddlewareData }) { + const { arrow, middlewareData } = options; + if (!middlewareData.arrow || !arrow) { + return; + } + + const { x: arrowX, y: arrowY } = middlewareData.arrow; + + Object.assign(arrow.style, { + left: `${arrowX}px`, + top: `${arrowY}px`, + }); +} diff --git a/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts new file mode 100644 index 00000000000000..86026375f02ce8 --- /dev/null +++ b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts @@ -0,0 +1,56 @@ +import type { Placement, MiddlewareData, Strategy, Coords } from '@floating-ui/dom'; +import { + DATA_POSITIONING_ESCAPED, + DATA_POSITIONING_HIDDEN, + DATA_POSITIONING_INTERSECTING, + DATA_POSITIONING_PLACEMENT, +} from '../constants'; + +/** + * Writes all container element position updates after the position is computed + */ +export function writeContainerUpdates(options: { + container: HTMLElement | null; + placement: Placement; + middlewareData: MiddlewareData; + /** + * Layer acceleration can disable subpixel rendering which causes slightly + * blurry text on low PPI displays, so we want to use 2D transforms + * instead + */ + lowPPI: boolean; + strategy: Strategy; + coordinates: Coords; +}) { + const { + container, + placement, + middlewareData, + strategy, + lowPPI, + coordinates: { x, y }, + } = options; + if (!container) { + return; + } + container.setAttribute(DATA_POSITIONING_PLACEMENT, placement); + container.removeAttribute(DATA_POSITIONING_INTERSECTING); + if (middlewareData.intersectionObserver.intersecting) { + container.setAttribute(DATA_POSITIONING_INTERSECTING, ''); + } + + container.removeAttribute(DATA_POSITIONING_ESCAPED); + if (middlewareData.hide?.escaped) { + container.setAttribute(DATA_POSITIONING_ESCAPED, ''); + } + + container.removeAttribute(DATA_POSITIONING_HIDDEN); + if (middlewareData.hide?.referenceHidden) { + container.setAttribute(DATA_POSITIONING_HIDDEN, ''); + } + + Object.assign(container.style, { + transform: lowPPI ? `translate(${x}px, ${y}px)` : `translate3d(${x}px, ${y}px, 0)`, + position: strategy, + }); +} From 3cad896457cc30c918b5ed7d7282f57317bbdd67 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 09:23:59 +0100 Subject: [PATCH 02/17] revert button changes --- .../react-button/src/components/Button/useButtonStyles.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-components/react-button/src/components/Button/useButtonStyles.ts b/packages/react-components/react-button/src/components/Button/useButtonStyles.ts index aff3303e42d925..148daa26a7d2d8 100644 --- a/packages/react-components/react-button/src/components/Button/useButtonStyles.ts +++ b/packages/react-components/react-button/src/components/Button/useButtonStyles.ts @@ -70,7 +70,7 @@ const useRootStyles = makeStyles({ // Transition styles transition: { - transitionDuration: '2000ms', + transitionDuration: '100ms', transitionProperty: 'background, border, color', transitionTimingFunction: 'cubic-bezier(0.33, 0, 0.67, 1)', From 0e498a74136b2393ef9f0bd5b59af77e51baaaf7 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 09:25:44 +0100 Subject: [PATCH 03/17] changefiles --- ...-react-portal-323fba1d-79e4-4b55-af4f-cfa9444a2bcf.json | 7 +++++++ ...t-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json | 7 +++++++ 2 files changed, 14 insertions(+) create mode 100644 change/@fluentui-react-portal-323fba1d-79e4-4b55-af4f-cfa9444a2bcf.json create mode 100644 change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json diff --git a/change/@fluentui-react-portal-323fba1d-79e4-4b55-af4f-cfa9444a2bcf.json b/change/@fluentui-react-portal-323fba1d-79e4-4b55-af4f-cfa9444a2bcf.json new file mode 100644 index 00000000000000..6d6ebf4c917df8 --- /dev/null +++ b/change/@fluentui-react-portal-323fba1d-79e4-4b55-af4f-cfa9444a2bcf.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "refactor: usePortalMount node updates mount node attributes in memo", + "packageName": "@fluentui/react-portal", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "patch" +} diff --git a/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json b/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json new file mode 100644 index 00000000000000..7ccf21895ff05c --- /dev/null +++ b/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "refactor: modifies target and container ref handling to reduce getScrollParent calls", + "packageName": "@fluentui/react-positioning", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "patch" +} From 16af62cbae8d49f978e55eb9c069ddf4f95fe3a0 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 09:26:35 +0100 Subject: [PATCH 04/17] update md --- .../react-positioning/etc/react-positioning.api.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/react-components/react-positioning/etc/react-positioning.api.md b/packages/react-components/react-positioning/etc/react-positioning.api.md index 417c02185fd894..8f64f421154ae6 100644 --- a/packages/react-components/react-positioning/etc/react-positioning.api.md +++ b/packages/react-components/react-positioning/etc/react-positioning.api.md @@ -68,13 +68,13 @@ export type Position = 'above' | 'below' | 'before' | 'after'; // @public (undocumented) export type PositioningImperativeRef = { updatePosition: () => void; - setTarget: (target: HTMLElement | PositioningVirtualElement) => void; + setTarget: (target: TargetType) => void; }; // @public (undocumented) export interface PositioningProps extends Omit { positioningRef?: React_2.Ref; - target?: HTMLElement | PositioningVirtualElement | null; + target?: TargetType; } // @public (undocumented) @@ -105,11 +105,7 @@ export function resolvePositioningShorthand(shorthand: PositioningShorthand | un export type SetVirtualMouseTarget = (event: React_2.MouseEvent | MouseEvent | undefined | null) => void; // @internal (undocumented) -export function usePositioning(options: UsePositioningOptions): { - targetRef: React_2.MutableRefObject; - containerRef: React_2.MutableRefObject; - arrowRef: React_2.MutableRefObject; -}; +export function usePositioning(options: UsePositioningOptions): UsePositioningReturn; // @internal export const usePositioningMouseTarget: (initialState?: PositioningVirtualElement | (() => PositioningVirtualElement) | undefined) => readonly [PositioningVirtualElement | undefined, SetVirtualMouseTarget]; From 3b606534bf612493cb5167facb4fec60cfd62d8f Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 09:44:42 +0100 Subject: [PATCH 05/17] fix ssr --- .../react-components/react-positioning/src/usePositioning.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-positioning/src/usePositioning.ts b/packages/react-components/react-positioning/src/usePositioning.ts index c5371d81d8972b..a19f6b9b3ccd6d 100644 --- a/packages/react-components/react-positioning/src/usePositioning.ts +++ b/packages/react-components/react-positioning/src/usePositioning.ts @@ -42,7 +42,6 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe const { enabled = true } = options; const resolvePositioningOptions = usePositioningOptions(options); - const { placement, middleware, strategy } = resolvePositioningOptions(containerRef.current, arrowRef.current); const forceUpdate = useEventCallback(() => { const target = targetRef.current; @@ -50,6 +49,8 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe return; } + const { placement, middleware, strategy } = resolvePositioningOptions(containerRef.current, arrowRef.current); + // Container is always initialized with `position: fixed` to avoid scroll jumps // Before computing the positioned coordinates, revert the container to the deisred positioning strategy Object.assign(containerRef.current.style, { position: strategy }); From a4db4d91538507486e0ea041a38454cb66393e70 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 13:12:37 +0100 Subject: [PATCH 06/17] update --- .../components/Portal/usePortalMountNode.ts | 33 +-- .../etc/react-positioning.api.md | 4 +- .../src/createPositionManager.ts | 119 +++++++++++ .../react-positioning/src/types.ts | 14 +- .../react-positioning/src/usePositioning.ts | 192 ++++++------------ 5 files changed, 213 insertions(+), 149 deletions(-) create mode 100644 packages/react-components/react-positioning/src/createPositionManager.ts diff --git a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts index 4f91a8b9100a74..a70301a48142dd 100644 --- a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts +++ b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts @@ -1,4 +1,5 @@ import * as React from 'react'; +import { useIsomorphicLayoutEffect } from '@fluentui/react-utilities'; import { useThemeClassName_unstable as useThemeClassName, useFluent_unstable as useFluent, @@ -32,29 +33,37 @@ export const usePortalMountNode = (options: UsePortalMountNodeOptions): HTMLElem const className = mergeClasses(themeClassName, classes.root); - const mountNode = React.useMemo(() => { + const element = React.useMemo(() => { if (targetDocument === undefined || options.disabled) { return null; } - const element = targetDocument.createElement('div'); - focusVisibleRef.current = element; + const newElement = targetDocument.createElement('div'); + targetDocument.body.appendChild(newElement); - const classesToApply = className.split(' ').filter(Boolean); - element.classList.add(...classesToApply); + return newElement; + }, [targetDocument, options.disabled]); - element.setAttribute('dir', dir); + React.useMemo(() => { + if (element) { + const classesToApply = className.split(' ').filter(Boolean); - targetDocument.body.appendChild(element); + element.classList.add(...classesToApply); + element.setAttribute('dir', dir); + focusVisibleRef.current = element; - return element; - }, [targetDocument, options.disabled, focusVisibleRef, className, dir]); + return () => { + element.classList.remove(...classesToApply); + element.removeAttribute('dir'); + }; + } + }, [className, dir, element, focusVisibleRef]); React.useEffect(() => { return () => { - mountNode?.remove(); + element?.parentElement?.removeChild(element); }; - }, [mountNode]); + }, [element]); - return mountNode; + return element; }; diff --git a/packages/react-components/react-positioning/etc/react-positioning.api.md b/packages/react-components/react-positioning/etc/react-positioning.api.md index 8f64f421154ae6..fa0c2308d12ba4 100644 --- a/packages/react-components/react-positioning/etc/react-positioning.api.md +++ b/packages/react-components/react-positioning/etc/react-positioning.api.md @@ -68,13 +68,13 @@ export type Position = 'above' | 'below' | 'before' | 'after'; // @public (undocumented) export type PositioningImperativeRef = { updatePosition: () => void; - setTarget: (target: TargetType) => void; + setTarget: (target: TargetElement) => void; }; // @public (undocumented) export interface PositioningProps extends Omit { positioningRef?: React_2.Ref; - target?: TargetType; + target?: TargetElement; } // @public (undocumented) diff --git a/packages/react-components/react-positioning/src/createPositionManager.ts b/packages/react-components/react-positioning/src/createPositionManager.ts new file mode 100644 index 00000000000000..35bd60e39039dd --- /dev/null +++ b/packages/react-components/react-positioning/src/createPositionManager.ts @@ -0,0 +1,119 @@ +import { computePosition } from '@floating-ui/dom'; +import type { Middleware, Placement, Strategy } from '@floating-ui/dom'; +import type { PositionManager, TargetElement } from './types'; +import { debounce, writeArrowUpdates, writeContainerUpdates, getScrollParent } from './utils'; + +interface PositionManagerOptions { + /** + * The positioned element + */ + container: HTMLElement; + /** + * Element that the container will be anchored to + */ + target: TargetElement; + /** + * Arrow that points from the container to the target + */ + arrow: HTMLElement | null; + /** + * The value of the css `position` property + * @default absolute + */ + strategy: Strategy; + /** + * [Floating UI middleware](https://floating-ui.com/docs/middleware) + */ + middleware: Middleware[]; + /** + * [Floating UI placement](https://floating-ui.com/docs/computePosition#placement) + */ + placement?: Placement; +} + +/** + * @returns manager that handles positioning out of the react lifecycle + */ +export function createPositionManager(options: PositionManagerOptions): PositionManager { + const { container, target, arrow, strategy, middleware, placement } = options; + let isFirstUpdate = true; + const scrollParents: Set = new Set(); + const defaultView = container.ownerDocument.defaultView; + + const forceUpdate = () => { + if (!target || !container) { + return; + } + + if (isFirstUpdate) { + scrollParents.add(getScrollParent(container)); + if (target instanceof HTMLElement) { + scrollParents.add(getScrollParent(target)); + } + + scrollParents.forEach(scrollParent => { + scrollParent.addEventListener('scroll', updatePosition); + }); + + // When the container is first resolved, set position `fixed` to avoid scroll jumps. + // Without this scroll jumps can occur when the element is rendered initially and receives focus + Object.assign(container.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); + isFirstUpdate = false; + } + + const targetDocument = container.ownerDocument; + + Object.assign(container.style, { position: strategy }); + computePosition(target, container, { placement, middleware, strategy }) + .then(({ x, y, middlewareData, placement: computedPlacement }) => { + writeArrowUpdates({ arrow, middlewareData }); + writeContainerUpdates({ + container, + middlewareData, + placement: computedPlacement, + coordinates: { x, y }, + lowPPI: (targetDocument?.defaultView?.devicePixelRatio || 1) <= 1, + strategy, + }); + }) + .catch(err => { + // https://github.com/floating-ui/floating-ui/issues/1845 + // FIXME for node > 14 + // node 15 introduces promise rejection which means that any components + // tests need to be `it('', async () => {})` otherwise there can be race conditions with + // JSDOM being torn down before this promise is resolved so globals like `window` and `document` don't exist + // Unless all tests that ever use `usePositioning` are turned into async tests, any logging during testing + // will actually be counter productive + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.error('[usePositioning]: Failed to calculate position', err); + } + }); + }; + + const updatePosition = debounce(() => forceUpdate()); + + const dispose = () => { + if (defaultView) { + defaultView.removeEventListener('scroll', updatePosition); + defaultView.removeEventListener('resize', updatePosition); + } + + scrollParents.forEach(scrollParent => { + scrollParent.removeEventListener('scroll', updatePosition); + }); + }; + + if (defaultView) { + defaultView.addEventListener('scroll', updatePosition); + defaultView.addEventListener('resize', updatePosition); + } + + // Update the position on initialization + updatePosition(); + + return { + updatePosition, + dispose, + }; +} diff --git a/packages/react-components/react-positioning/src/types.ts b/packages/react-components/react-positioning/src/types.ts index c9f233f4527b31..e39f76f1e11a1f 100644 --- a/packages/react-components/react-positioning/src/types.ts +++ b/packages/react-components/react-positioning/src/types.ts @@ -14,7 +14,7 @@ export type OffsetFunctionParam = { alignment?: Alignment; }; -export type TargetType = HTMLElement | PositioningVirtualElement | null; +export type TargetElement = HTMLElement | PositioningVirtualElement; /** * @internal @@ -26,6 +26,14 @@ export interface UsePositioningOptions extends PositioningProps { enabled?: boolean; } +/** + * @internal + */ +export interface PositionManager { + updatePosition: () => void; + dispose: () => void; +} + export interface UsePositioningReturn { // React refs are supposed to be contravariant // (allows a more general type to be passed rather than a more specific one) @@ -65,7 +73,7 @@ export type PositioningImperativeRef = { * Sets the target and updates positioning imperatively. * Useful for avoiding double renders with the target option. */ - setTarget: (target: TargetType) => void; + setTarget: (target: TargetElement) => void; }; export type PositioningVirtualElement = { @@ -158,7 +166,7 @@ export interface PositioningProps /** * Manual override for the target element. Useful for scenarios where a component accepts user prop to override target */ - target?: TargetType; + target?: TargetElement; } export type PositioningShorthandValue = diff --git a/packages/react-components/react-positioning/src/usePositioning.ts b/packages/react-components/react-positioning/src/usePositioning.ts index a19f6b9b3ccd6d..6d4f544aa3f711 100644 --- a/packages/react-components/react-positioning/src/usePositioning.ts +++ b/packages/react-components/react-positioning/src/usePositioning.ts @@ -1,26 +1,16 @@ -import { computePosition, hide as hideMiddleware, arrow as arrowMiddleware } from '@floating-ui/dom'; +import { hide as hideMiddleware, arrow as arrowMiddleware } from '@floating-ui/dom'; import type { Middleware, Strategy } from '@floating-ui/dom'; import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts'; import { canUseDOM, useIsomorphicLayoutEffect } from '@fluentui/react-utilities'; -import { useEventCallback } from '@fluentui/react-utilities'; import * as React from 'react'; import type { PositioningOptions, - PositioningVirtualElement, - TargetType, - UsePositioningOptions, + PositioningProps, + PositionManager, + TargetElement, UsePositioningReturn, } from './types'; -import { - useCallbackRef, - toFloatingUIPlacement, - hasAutofocusFilter, - debounce, - hasScrollParent, - getScrollParent, - writeArrowUpdates, - writeContainerUpdates, -} from './utils'; +import { useCallbackRef, toFloatingUIPlacement, hasAutofocusFilter, hasScrollParent } from './utils'; import { shift as shiftMiddleware, flip as flipMiddleware, @@ -29,105 +19,49 @@ import { offset as offsetMiddleware, intersecting as intersectingMiddleware, } from './middleware'; +import { createPositionManager } from './createPositionManager'; /** * @internal */ export function usePositioning(options: UsePositioningOptions): UsePositioningReturn { + const managerRef = React.useRef(null); + const targetRef = React.useRef(null); + const overrideTargetRef = React.useRef(null); const containerRef = React.useRef(null); - const targetRef = React.useRef(null); const arrowRef = React.useRef(null); - const { targetDocument } = useFluent(); - const { enabled = true } = options; const resolvePositioningOptions = usePositioningOptions(options); - - const forceUpdate = useEventCallback(() => { - const target = targetRef.current; - if (!canUseDOM || !enabled || !target || !containerRef.current) { - return; + const updatePositionManager = React.useCallback(() => { + if (managerRef.current) { + managerRef.current.dispose(); } - - const { placement, middleware, strategy } = resolvePositioningOptions(containerRef.current, arrowRef.current); - - // Container is always initialized with `position: fixed` to avoid scroll jumps - // Before computing the positioned coordinates, revert the container to the deisred positioning strategy - Object.assign(containerRef.current.style, { position: strategy }); - computePosition(target, containerRef.current, { placement, middleware, strategy }) - .then(({ x, y, middlewareData, placement: computedPlacement }) => { - writeArrowUpdates({ arrow: arrowRef.current, middlewareData }); - writeContainerUpdates({ - container: containerRef.current, - middlewareData, - placement: computedPlacement, - coordinates: { x, y }, - lowPPI: (targetDocument?.defaultView?.devicePixelRatio || 1) <= 1, - strategy, - }); - }) - .catch(err => { - // https://github.com/floating-ui/floating-ui/issues/1845 - // FIXME for node > 14 - // node 15 introduces promise rejection which means that any components - // tests need to be `it('', async () => {})` otherwise there can be race conditions with - // JSDOM being torn down before this promise is resolved so globals like `window` and `document` don't exist - // Unless all tests that ever use `usePositioning` are turned into async tests, any logging during testing - // will actually be counter productive - if (process.env.NODE_ENV === 'development') { - // eslint-disable-next-line no-console - console.error('[usePositioning]: Failed to calculate position', err); - } + managerRef.current = null; + + if (enabled && canUseDOM() && targetRef.current && containerRef.current) { + managerRef.current = createPositionManager({ + container: containerRef.current, + target: overrideTargetRef.current ?? targetRef.current, + arrow: arrowRef.current, + ...resolvePositioningOptions(containerRef.current, arrowRef.current), }); - }); - - const updatePosition = React.useState(() => debounce(forceUpdate))[0]; - - const cleanupScrollListenersRef = React.useRef<() => void>(() => null); - const handleRefUpdate = React.useCallback(() => { - if (containerRef.current) { - // When the container is first resolved, set position `fixed` to avoid scroll jumps. - // Without this scroll jumps can occur when the element is rendered initially and receives focus - Object.assign(containerRef.current.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); } + }, [enabled, resolvePositioningOptions]); - cleanupScrollListenersRef.current(); - const scrollParents: Set = new Set(); - if (enabled && containerRef.current && targetRef.current) { - // `getScrollParent` will cause reflow, running it when when enabled, container and target - // are all correctly set will make sure that it is run as little as possible - scrollParents.add(getScrollParent(containerRef.current)); - if (targetRef.current instanceof HTMLElement) { - scrollParents.add(getScrollParent(targetRef.current)); - } - - scrollParents.forEach(scrollParent => { - scrollParent.addEventListener('scroll', updatePosition); - }); - - cleanupScrollListenersRef.current = () => { - scrollParents.forEach(scrollParent => { - scrollParent.removeEventListener('scroll', updatePosition); - }); - }; - } - - updatePosition(); - }, [enabled, updatePosition]); - - const overrideTarget = React.useCallback( - (target: TargetType) => { - targetRef.current = target; - handleRefUpdate(); + const setOverrideTarget = React.useCallback( + (target: TargetElement | null) => { + overrideTargetRef.current = target; + updatePositionManager(); }, - [handleRefUpdate], + [updatePositionManager], ); React.useImperativeHandle( options.positioningRef, () => ({ - updatePosition, - setTarget: (target: TargetType) => { + updatePosition: () => managerRef.current?.updatePosition(), + setTarget: (target: TargetElement) => { if (options.target && process.env.NODE_ENV !== 'production') { const err = new Error(); // eslint-disable-next-line no-console @@ -136,40 +70,19 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe console.warn(err.stack); } - overrideTarget(target); + setOverrideTarget(target); }, }), - // Missing deps: - // options.target - only used for a runtime warning - // overrideTargetRef - Stable between renders - // updatePosition - Stable between renders - // eslint-disable-next-line react-hooks/exhaustive-deps - [], + [options.target, setOverrideTarget], ); useIsomorphicLayoutEffect(() => { - if (options.target) { - overrideTarget(options.target); - } - }, [options.target, overrideTarget]); + setOverrideTarget(options.target ?? null); + }, [options.target, setOverrideTarget]); useIsomorphicLayoutEffect(() => { - updatePosition(); - }, [enabled, resolvePositioningOptions, updatePosition]); - - // Add window resize and scroll listeners to update position - useIsomorphicLayoutEffect(() => { - const win = targetDocument?.defaultView; - if (win) { - win.addEventListener('resize', updatePosition); - win.addEventListener('scroll', updatePosition); - - return () => { - win.removeEventListener('resize', updatePosition); - win.removeEventListener('scroll', updatePosition); - }; - } - }, [updatePosition, targetDocument]); + updatePositionManager(); + }, [enabled, resolvePositioningOptions, updatePositionManager]); if (process.env.NODE_ENV !== 'production') { // This checked should run only in development mode @@ -213,21 +126,36 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe }, []); } - const setTargetElement = useCallbackRef(null, target => { - targetRef.current = target; - handleRefUpdate(); + const setTarget = useCallbackRef(null, target => { + if (targetRef.current !== target) { + targetRef.current = target; + updatePositionManager(); + } }); - const setContainerElement = useCallbackRef(null, container => { - containerRef.current = container; - handleRefUpdate(); + + const setContainer = useCallbackRef(null, container => { + if (containerRef.current !== container) { + containerRef.current = container; + updatePositionManager(); + } }); - const setArrowElement = useCallbackRef(null, arrow => { - containerRef.current = arrow; - handleRefUpdate(); + + const setArrow = useCallbackRef(null, arrow => { + if (arrowRef.current !== arrow) { + arrowRef.current = arrow; + updatePositionManager(); + } }); - // Users should consume callback refs so they can set them like 'standard' HTML refs - return { targetRef: setTargetElement, containerRef: setContainerElement, arrowRef: setArrowElement }; + // Let users use callback refs so they feel like 'normal' DOM refs + return { targetRef: setTarget, containerRef: setContainer, arrowRef: setArrow }; +} + +interface UsePositioningOptions extends PositioningProps { + /** + * If false, does not position anything + */ + enabled?: boolean; } function usePositioningOptions(options: PositioningOptions) { From 92e4f17324fd1a459de4dce8b930a5621025207b Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 13:20:57 +0100 Subject: [PATCH 07/17] fix blurriness --- .../react-positioning/src/utils/writeContainerupdates.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts index 86026375f02ce8..bae424c1de1b4b 100644 --- a/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts +++ b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts @@ -49,8 +49,11 @@ export function writeContainerUpdates(options: { container.setAttribute(DATA_POSITIONING_HIDDEN, ''); } + // decmial translate values can cause blurriness + const floorX = Math.floor(x); + const floorY = Math.floor(y); Object.assign(container.style, { - transform: lowPPI ? `translate(${x}px, ${y}px)` : `translate3d(${x}px, ${y}px, 0)`, + transform: lowPPI ? `translate(${floorX}px, ${floorY}px)` : `translate3d(${floorX}px, ${floorY}px, 0)`, position: strategy, }); } From c211a6d9aee6bd8541a4b6ad0bafe0bbfa28d6e9 Mon Sep 17 00:00:00 2001 From: ling1726 Date: Tue, 1 Nov 2022 13:24:40 +0100 Subject: [PATCH 08/17] Update change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json --- ...-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json b/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json index 7ccf21895ff05c..cfb0b890bbe45e 100644 --- a/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json +++ b/change/@fluentui-react-positioning-b6d61cdf-e434-43ff-a859-ab563e6a47f5.json @@ -1,6 +1,6 @@ { "type": "minor", - "comment": "refactor: modifies target and container ref handling to reduce getScrollParent calls", + "comment": "feat: position updates are handled out of react lifecycle", "packageName": "@fluentui/react-positioning", "email": "lingfangao@hotmail.com", "dependentChangeType": "patch" From 91cca723503d722e225c0b5c23b435ccc13a4415 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:13:38 +0100 Subject: [PATCH 09/17] document usePortalMountNode memo --- .../react-portal/src/components/Portal/usePortalMountNode.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts index a70301a48142dd..f96d9fe6eb57f4 100644 --- a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts +++ b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts @@ -44,6 +44,10 @@ export const usePortalMountNode = (options: UsePortalMountNodeOptions): HTMLElem return newElement; }, [targetDocument, options.disabled]); + // This useMemo call is intentional + // We don't want to re-create the portal element when its attributes change. + // This also should not be done in an effect because, changing the value of css variables + // after initial mount can trigger interesting CSS side effects like transitions. React.useMemo(() => { if (element) { const classesToApply = className.split(' ').filter(Boolean); From f191e8b4539956e2ac954a7ee57fe9da76ca584f Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:15:14 +0100 Subject: [PATCH 10/17] fix breaking api --- .../react-positioning/etc/react-positioning.api.md | 2 +- packages/react-components/react-positioning/src/types.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-components/react-positioning/etc/react-positioning.api.md b/packages/react-components/react-positioning/etc/react-positioning.api.md index fa0c2308d12ba4..202975793eea06 100644 --- a/packages/react-components/react-positioning/etc/react-positioning.api.md +++ b/packages/react-components/react-positioning/etc/react-positioning.api.md @@ -74,7 +74,7 @@ export type PositioningImperativeRef = { // @public (undocumented) export interface PositioningProps extends Omit { positioningRef?: React_2.Ref; - target?: TargetElement; + target?: TargetElement | null; } // @public (undocumented) diff --git a/packages/react-components/react-positioning/src/types.ts b/packages/react-components/react-positioning/src/types.ts index e39f76f1e11a1f..44eaec449deed4 100644 --- a/packages/react-components/react-positioning/src/types.ts +++ b/packages/react-components/react-positioning/src/types.ts @@ -166,7 +166,7 @@ export interface PositioningProps /** * Manual override for the target element. Useful for scenarios where a component accepts user prop to override target */ - target?: TargetElement; + target?: TargetElement | null; } export type PositioningShorthandValue = From 22160da88ddb8e8589066f8e2ba8cdac4eb745fd Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:16:11 +0100 Subject: [PATCH 11/17] rename to targetWindow --- .../src/createPositionManager.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/react-components/react-positioning/src/createPositionManager.ts b/packages/react-components/react-positioning/src/createPositionManager.ts index 35bd60e39039dd..af1a82e23413e6 100644 --- a/packages/react-components/react-positioning/src/createPositionManager.ts +++ b/packages/react-components/react-positioning/src/createPositionManager.ts @@ -38,7 +38,7 @@ export function createPositionManager(options: PositionManagerOptions): Position const { container, target, arrow, strategy, middleware, placement } = options; let isFirstUpdate = true; const scrollParents: Set = new Set(); - const defaultView = container.ownerDocument.defaultView; + const targetWindow = container.ownerDocument.defaultView; const forceUpdate = () => { if (!target || !container) { @@ -61,8 +61,6 @@ export function createPositionManager(options: PositionManagerOptions): Position isFirstUpdate = false; } - const targetDocument = container.ownerDocument; - Object.assign(container.style, { position: strategy }); computePosition(target, container, { placement, middleware, strategy }) .then(({ x, y, middlewareData, placement: computedPlacement }) => { @@ -72,7 +70,7 @@ export function createPositionManager(options: PositionManagerOptions): Position middlewareData, placement: computedPlacement, coordinates: { x, y }, - lowPPI: (targetDocument?.defaultView?.devicePixelRatio || 1) <= 1, + lowPPI: (targetWindow?.devicePixelRatio || 1) <= 1, strategy, }); }) @@ -94,9 +92,9 @@ export function createPositionManager(options: PositionManagerOptions): Position const updatePosition = debounce(() => forceUpdate()); const dispose = () => { - if (defaultView) { - defaultView.removeEventListener('scroll', updatePosition); - defaultView.removeEventListener('resize', updatePosition); + if (targetWindow) { + targetWindow.removeEventListener('scroll', updatePosition); + targetWindow.removeEventListener('resize', updatePosition); } scrollParents.forEach(scrollParent => { @@ -104,9 +102,9 @@ export function createPositionManager(options: PositionManagerOptions): Position }); }; - if (defaultView) { - defaultView.addEventListener('scroll', updatePosition); - defaultView.addEventListener('resize', updatePosition); + if (targetWindow) { + targetWindow.addEventListener('scroll', updatePosition); + targetWindow.addEventListener('resize', updatePosition); } // Update the position on initialization From 6251c90ab982828fed4f37b8e01c281d9ce03ed3 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:16:34 +0100 Subject: [PATCH 12/17] createPositionManager is internal --- .../react-positioning/src/createPositionManager.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-components/react-positioning/src/createPositionManager.ts b/packages/react-components/react-positioning/src/createPositionManager.ts index af1a82e23413e6..0afd8beb52f4c4 100644 --- a/packages/react-components/react-positioning/src/createPositionManager.ts +++ b/packages/react-components/react-positioning/src/createPositionManager.ts @@ -32,6 +32,7 @@ interface PositionManagerOptions { } /** + * @internal * @returns manager that handles positioning out of the react lifecycle */ export function createPositionManager(options: PositionManagerOptions): PositionManager { From 5dd23799081be8c5163d453401498b6a71912de7 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:17:43 +0100 Subject: [PATCH 13/17] short circuit --- .../react-positioning/src/createPositionManager.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-components/react-positioning/src/createPositionManager.ts b/packages/react-components/react-positioning/src/createPositionManager.ts index 0afd8beb52f4c4..bd159d0267b258 100644 --- a/packages/react-components/react-positioning/src/createPositionManager.ts +++ b/packages/react-components/react-positioning/src/createPositionManager.ts @@ -37,15 +37,18 @@ interface PositionManagerOptions { */ export function createPositionManager(options: PositionManagerOptions): PositionManager { const { container, target, arrow, strategy, middleware, placement } = options; + if (!target || !container) { + return { + updatePosition: () => undefined, + dispose: () => undefined, + }; + } + let isFirstUpdate = true; const scrollParents: Set = new Set(); const targetWindow = container.ownerDocument.defaultView; const forceUpdate = () => { - if (!target || !container) { - return; - } - if (isFirstUpdate) { scrollParents.add(getScrollParent(container)); if (target instanceof HTMLElement) { From 2c1605b2c179951c04abf76acb87cd02d2c0e8de Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:19:06 +0100 Subject: [PATCH 14/17] update check to include override target --- .../react-positioning/src/usePositioning.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react-components/react-positioning/src/usePositioning.ts b/packages/react-components/react-positioning/src/usePositioning.ts index 6d4f544aa3f711..37b1e4e92360d8 100644 --- a/packages/react-components/react-positioning/src/usePositioning.ts +++ b/packages/react-components/react-positioning/src/usePositioning.ts @@ -39,10 +39,12 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe } managerRef.current = null; - if (enabled && canUseDOM() && targetRef.current && containerRef.current) { + const target = overrideTargetRef.current ?? targetRef.current; + + if (enabled && canUseDOM() && target && containerRef.current) { managerRef.current = createPositionManager({ container: containerRef.current, - target: overrideTargetRef.current ?? targetRef.current, + target, arrow: arrowRef.current, ...resolvePositioningOptions(containerRef.current, arrowRef.current), }); From d9a2d53774f4ed46511f2a598fdd07d0ac3faded Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:22:01 +0100 Subject: [PATCH 15/17] remove useless improt --- .../react-portal/src/components/Portal/usePortalMountNode.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts index f96d9fe6eb57f4..04c6f0ebdf1f3c 100644 --- a/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts +++ b/packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts @@ -1,5 +1,4 @@ import * as React from 'react'; -import { useIsomorphicLayoutEffect } from '@fluentui/react-utilities'; import { useThemeClassName_unstable as useThemeClassName, useFluent_unstable as useFluent, From 294aa77391c7b51b45666edc41ae2ba0ccaeda7e Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Tue, 1 Nov 2022 22:24:05 +0100 Subject: [PATCH 16/17] revert blurriness change --- .../react-positioning/src/utils/writeContainerupdates.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts index bae424c1de1b4b..86026375f02ce8 100644 --- a/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts +++ b/packages/react-components/react-positioning/src/utils/writeContainerupdates.ts @@ -49,11 +49,8 @@ export function writeContainerUpdates(options: { container.setAttribute(DATA_POSITIONING_HIDDEN, ''); } - // decmial translate values can cause blurriness - const floorX = Math.floor(x); - const floorY = Math.floor(y); Object.assign(container.style, { - transform: lowPPI ? `translate(${floorX}px, ${floorY}px)` : `translate3d(${floorX}px, ${floorY}px, 0)`, + transform: lowPPI ? `translate(${x}px, ${y}px)` : `translate3d(${x}px, ${y}px, 0)`, position: strategy, }); } From 96ad42a30a91004f5dcf90f05527a0273114dd56 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 2 Nov 2022 11:01:40 +0100 Subject: [PATCH 17/17] fix position fixed and effect --- .../react-positioning/src/createPositionManager.ts | 7 ++++--- .../react-positioning/src/usePositioning.ts | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/react-components/react-positioning/src/createPositionManager.ts b/packages/react-components/react-positioning/src/createPositionManager.ts index bd159d0267b258..4cc1b421cec73a 100644 --- a/packages/react-components/react-positioning/src/createPositionManager.ts +++ b/packages/react-components/react-positioning/src/createPositionManager.ts @@ -48,6 +48,10 @@ export function createPositionManager(options: PositionManagerOptions): Position const scrollParents: Set = new Set(); const targetWindow = container.ownerDocument.defaultView; + // When the container is first resolved, set position `fixed` to avoid scroll jumps. + // Without this scroll jumps can occur when the element is rendered initially and receives focus + Object.assign(container.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); + const forceUpdate = () => { if (isFirstUpdate) { scrollParents.add(getScrollParent(container)); @@ -59,9 +63,6 @@ export function createPositionManager(options: PositionManagerOptions): Position scrollParent.addEventListener('scroll', updatePosition); }); - // When the container is first resolved, set position `fixed` to avoid scroll jumps. - // Without this scroll jumps can occur when the element is rendered initially and receives focus - Object.assign(container.style, { position: 'fixed', left: 0, top: 0, margin: 0 }); isFirstUpdate = false; } diff --git a/packages/react-components/react-positioning/src/usePositioning.ts b/packages/react-components/react-positioning/src/usePositioning.ts index 37b1e4e92360d8..2c29ec2b73688c 100644 --- a/packages/react-components/react-positioning/src/usePositioning.ts +++ b/packages/react-components/react-positioning/src/usePositioning.ts @@ -84,7 +84,7 @@ export function usePositioning(options: UsePositioningOptions): UsePositioningRe useIsomorphicLayoutEffect(() => { updatePositionManager(); - }, [enabled, resolvePositioningOptions, updatePositionManager]); + }, [updatePositionManager]); if (process.env.NODE_ENV !== 'production') { // This checked should run only in development mode