diff --git a/CHANGELOG.md b/CHANGELOG.md index 5316a7f65d..c4e6076063 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Update iconOnly button hover, focus styles and add new `background5` and `backgroundHover2` design tokens in Teams theme @codepretty ([#2211](https://github.com/microsoft/fluent-ui-react/pull/2211)) - Fix `forceUpdate` to get synced updates in React's Concurrent mode @layershifter ([#2268](https://github.com/microsoft/fluent-ui-react/pull/2268)) - Adding actionable items into `Carousel` @kolaps33 ([#2271](https://github.com/microsoft/fluent-ui-react/pull/2271)) +- Fix positioning fixes for `actions` in `ChatMessage` @layershifter ([#2300](https://github.com/microsoft/fluent-ui-react/pull/2300)) ### Features - Allow `useRef` hook used for storing debugging data to be defined in any order with other hooks in functional components @layershifter, @mnajdova ([#2236](https://github.com/microsoft/fluent-ui-react/pull/2236)) diff --git a/docs/src/examples/components/Chat/Content/ChatExampleActions.shorthand.steps.ts b/docs/src/examples/components/Chat/Content/ChatExampleActions.shorthand.steps.ts new file mode 100644 index 0000000000..bbd5b7798f --- /dev/null +++ b/docs/src/examples/components/Chat/Content/ChatExampleActions.shorthand.steps.ts @@ -0,0 +1,16 @@ +import { ChatMessage } from '@fluentui/react' + +const selectors = { + message: `.${ChatMessage.className}`, +} + +const config: ScreenerTestsConfig = { + themes: ['teams', 'teamsDark', 'teamsHighContrast'], + steps: [ + builder => builder.hover(selectors.message).snapshot('Hovers the first message'), + builder => builder.click(selectors.message).snapshot('Focus the first message via mouse click'), + (builder, keys) => builder.keys('body', keys.tab).snapshot('Focuses a message via keyboard'), + ], +} + +export default config diff --git a/docs/src/examples/components/Chat/Usage/ChatExampleInScrollable.shorthand.tsx b/docs/src/examples/components/Chat/Usage/ChatExampleInScrollable.shorthand.tsx index 768fb9df11..006939644a 100644 --- a/docs/src/examples/components/Chat/Usage/ChatExampleInScrollable.shorthand.tsx +++ b/docs/src/examples/components/Chat/Usage/ChatExampleInScrollable.shorthand.tsx @@ -27,7 +27,7 @@ const ChatExampleInScrollableShorthand = () => { }) const [width] = useRangeKnob({ name: 'width', - initialValue: '400px', + initialValue: '500px', min: '100px', max: '500px', step: 10, @@ -81,6 +81,22 @@ const ChatExampleInScrollableShorthand = () => { ), key: 'message-3', }, + { + message: ( + + ), + key: 'message-4', + }, + { + message: ( + + ), + key: 'message-5', + }, ] return ( diff --git a/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ChatWithPopover.tsx b/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ChatWithPopover.tsx index 74b4266d8f..2bc5c1a635 100644 --- a/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ChatWithPopover.tsx +++ b/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ChatWithPopover.tsx @@ -17,18 +17,16 @@ const reactions: ShorthandCollection = [ content: '1K', key: 'likes', variables: { meReacting: true }, + children: (Component, props) => , }, { icon: 'emoji', content: 2, key: 'smiles', + children: (Component, props) => , }, ] -const reactionsWithPopup = reactions.map(reaction => render => - render(reaction, (Component, props) => ), -) - const janeAvatar = { image: 'public/images/avatar/small/ade.jpg', status: { color: 'green', icon: 'check' }, @@ -95,7 +93,7 @@ const ChatWithPopover = () => { } reactionGroup={{ - items: reactionsWithPopup, + items: reactions, }} timestamp="Yesterday, 10:15 PM" /> @@ -113,7 +111,7 @@ const ChatWithPopover = () => { } reactionGroup={{ - items: reactionsWithPopup, + items: reactions, }} timestamp="Yesterday, 10:15 PM" /> diff --git a/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ReactionPopup.tsx b/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ReactionPopup.tsx index 6aec0b05f7..ea096538f6 100644 --- a/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ReactionPopup.tsx +++ b/docs/src/prototypes/chatMessages/ChatMessageWithPopover/ReactionPopup.tsx @@ -36,16 +36,19 @@ class ReactionPopup extends React.Component { aria-label={getAriaLabel(this.props)} /> } - content={ - - } + content={{ + children: () => ( + + ), + }} inline on="hover" + position="below" open={this.state.open} onOpenChange={this.handleOpenChange} /> diff --git a/packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts b/packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts index e0b3e7bf9a..3690ede409 100644 --- a/packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts +++ b/packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts @@ -8,6 +8,7 @@ import { import { ChatMessageVariables } from './chatMessageVariables' import { screenReaderContainerStyles } from '../../../../utils/accessibility/Styles/accessibilityStyles' import { pxToRem } from '../../../../utils' +import initialPopperStyles from '../../../../utils/positioner/initialStyles' import getBorderFocusStyles from '../../getBorderFocusStyles' const chatMessageStyles: ComponentSlotStylesPrepared< @@ -95,6 +96,8 @@ const chatMessageStyles: ComponentSlotStylesPrepared< // we need higher zIndex for the action menu in order to be displayed above the focus border of the chat message zIndex: 1000, + ...(initialPopperStyles as ICSSInJSStyle), + ...(_.isNil(v.showActionMenu) && { overflow: p.focused ? 'visible' : 'hidden', // hide and squash actions menu to prevent accidental hovers over its invisible area diff --git a/packages/react/src/themes/teams/components/Popup/popupStyles.ts b/packages/react/src/themes/teams/components/Popup/popupStyles.ts index 8ad693c618..8587ad6fd3 100644 --- a/packages/react/src/themes/teams/components/Popup/popupStyles.ts +++ b/packages/react/src/themes/teams/components/Popup/popupStyles.ts @@ -1,6 +1,7 @@ import { ComponentSlotStylesPrepared, ICSSInJSStyle } from '@fluentui/styles' import { PopupProps } from '../../../../components/Popup/Popup' import { PopupVariables } from './popupVariables' +import initialPopperStyles from '../../../../utils/positioner/initialStyles' const popupStyles: ComponentSlotStylesPrepared = { root: (): ICSSInJSStyle => ({}), @@ -11,13 +12,7 @@ const popupStyles: ComponentSlotStylesPrepared = { color: v.contentColor, background: v.contentBackgroundColor, - // Prevents scroll issue, waiting for Popper.js to add this style once initiated. - // https://github.com/mui-org/material-ui/issues/16740 - position: 'fixed', - // Fix Popper.js initial positioning display issue - // https://github.com/mui-org/material-ui/issues/17774 - top: 0, - left: '0px /* @noflip */', + ...(initialPopperStyles as ICSSInJSStyle), }), } diff --git a/packages/react/src/utils/positioner/Popper.tsx b/packages/react/src/utils/positioner/Popper.tsx index fa1a7e6f50..6a280f15f7 100644 --- a/packages/react/src/utils/positioner/Popper.tsx +++ b/packages/react/src/utils/positioner/Popper.tsx @@ -1,11 +1,30 @@ -import * as React from 'react' +import { Ref, isRefObject } from '@fluentui/react-component-ref' import * as _ from 'lodash' import PopperJS, * as _PopperJS from 'popper.js' -import { Ref, isRefObject } from '@fluentui/react-component-ref' +import * as React from 'react' -import { getPlacement, applyRtlToOffset } from './positioningHelper' -import { PopperProps, PopperChildrenFn } from './types' import getScrollParent from './getScrollParent' +import { getPlacement, applyRtlToOffset } from './positioningHelper' +import { PopperProps } from './types' + +/** + * Memoize a result using deep equality. This hook has two advantages over + * React.useMemo: it uses deep equality to compare memo keys, and it guarantees + * that the memo function will only be called if the keys are unequal. + * React.useMemo cannot be relied on to do this, since it is only a performance + * optimization (see https://reactjs.org/docs/hooks-reference.html#usememo). + * + * Copied from https://github.com/apollographql/react-apollo/blob/master/packages/hooks/src/utils/useDeepMemo.ts. + */ +function useDeepMemo(memoFn: () => TValue, key: TKey): TValue { + const ref = React.useRef<{ key: TKey; value: TValue }>() + + if (!ref.current || !_.isEqual(key, ref.current.key)) { + ref.current = { key, value: memoFn() } + } + + return ref.current.value +} // `popper.js` has a UMD build without `.default`, it breaks CJS builds: // https://github.com/rollup/rollup/issues/1267#issuecomment-446681320 @@ -35,6 +54,7 @@ const createPopper = ( return instance } + /** * Popper relies on the 3rd party library [Popper.js](https://github.com/FezVrasta/popper.js) for positioning. */ @@ -58,18 +78,61 @@ const Popper: React.FunctionComponent = props => { const popperRef = React.useRef() const contentRef = React.useRef(null) + const latestPlacement = React.useRef(proposedPlacement) const [computedPlacement, setComputedPlacement] = React.useState( proposedPlacement, ) - const computedModifiers: PopperJS.Modifiers = React.useMemo( + const hasScrollableElement = React.useMemo(() => { + const scrollParentElement = getScrollParent(contentRef.current) + + return scrollParentElement !== scrollParentElement.ownerDocument.body + }, [contentRef]) + // Is a broken dependency and can cause potential bugs, we should rethink this as all other refs + // in this component. + + const computedModifiers: PopperJS.Modifiers = useDeepMemo( () => - offset && { - offset: { offset: rtl ? applyRtlToOffset(offset, position) : offset }, - keepTogether: { enabled: false }, - }, - [rtl, offset, position], + _.merge( + /** + * This prevents blurrines in chrome, when the coordinates are odd numbers alternative + * would be to use `fn` and manipulate the computed style or ask popper to fix it but + * since there is presumably only handful of poppers displayed on the page, the + * performance impact should be minimal. + */ + { computeStyle: { gpuAcceleration: false } }, + + { flip: { padding: 0, flipVariationsByContent: true } }, + { preventOverflow: { padding: 0 } }, + + offset && { + offset: { offset: rtl ? applyRtlToOffset(offset, position) : offset }, + keepTogether: { enabled: false }, + }, + + /** + * When the popper box is placed in the context of a scrollable element, we need to set + * preventOverflow.escapeWithReference to true and flip.boundariesElement to 'scrollParent' + * (default is 'viewport') so that the popper box will stick with the targetRef when we + * scroll targetRef out of the viewport. + */ + hasScrollableElement && { + preventOverflow: { escapeWithReference: true }, + flip: { boundariesElement: 'scrollParent' }, + }, + + userModifiers, + + /** + * unstable_pinned disables the flip modifier by setting flip.enabled to false; this + * disables automatic repositioning of the popper box; it will always be placed according to + * the values of `align` and `position` props, regardless of the size of the component, the + * reference element or the viewport. + */ + unstable_pinned && { flip: { enabled: false } }, + ), + [hasScrollableElement, position, offset, rtl, unstable_pinned, userModifiers], ) const scheduleUpdate = React.useCallback(() => { @@ -102,48 +165,6 @@ const Popper: React.FunctionComponent = props => { return } - const pointerTargetRefElement = pointerTargetRef && pointerTargetRef.current - const scrollParentElement = getScrollParent(contentRef.current) - const popperHasScrollableParent = scrollParentElement !== scrollParentElement.ownerDocument.body - - const modifiers: PopperJS.Modifiers = _.merge( - { preventOverflow: { padding: 0 } }, - { flip: { padding: 0, flipVariationsByContent: true } }, - /** - * This prevents blurrines in chrome, when the coordinates are odd numbers - * alternative would be to use `fn`, call _PopperJS.default.Defaults.modifiers.computeStyle.fn(data, options) - * and manipulate the computeed style or ask popper to fix it - * but since there is presumably only handful of poppers displayed on the page, the performance impact should be minimal - */ - { computeStyle: { gpuAcceleration: false } }, - /** - * When the popper box is placed in the context of a scrollable element, we need to set - * preventOverflow.escapeWithReference to true and flip.boundariesElement to 'scrollParent' (default is 'viewport') - * so that the popper box will stick with the targetRef when we scroll targetRef out of the viewport. - */ - popperHasScrollableParent && { - preventOverflow: { escapeWithReference: true }, - flip: { boundariesElement: 'scrollParent' }, - }, - /** - * unstable_pinned disables the flip modifier by setting flip.enabled to false; this disables automatic - * repositioning of the popper box; it will always be placed according to the values of `align` and - * `position` props, regardless of the size of the component, the reference element or the viewport. - */ - unstable_pinned && { flip: { enabled: false } }, - computedModifiers, - userModifiers, - /** - * This modifier is necessary in order to render the pointer. - */ - { - arrow: { - enabled: !!pointerTargetRefElement, - element: pointerTargetRefElement, - }, - }, - ) - const handleUpdate = (data: PopperJS.Data) => { // PopperJS performs computations that might update the computed placement: auto positioning, flipping the // placement in case the popper box should be rendered at the edge of the viewport and does not fit @@ -156,7 +177,18 @@ const Popper: React.FunctionComponent = props => { const options: PopperJS.PopperOptions = { placement: proposedPlacement, positionFixed, - modifiers, + modifiers: { + ...computedModifiers, + + /** + * This modifier is necessary in order to render the pointer. Refs are resolved in effects, so it can't be + * placed under computed modifiers. Deep merge is not required as this modifier has only these properties. + */ + arrow: { + enabled: !!(pointerTargetRef && pointerTargetRef.current), + element: pointerTargetRef && pointerTargetRef.current, + }, + }, onCreate: handleUpdate, onUpdate: handleUpdate, } @@ -164,13 +196,13 @@ const Popper: React.FunctionComponent = props => { popperRef.current = createPopper(reference, contentRef.current, options) }, [ // TODO review dependencies for popperHasScrollableParent - computedModifiers, enabled, - userModifiers, + computedModifiers, + pointerTargetRef, positionFixed, proposedPlacement, - unstable_pinned, targetRef, + unstable_pinned, ]) React.useLayoutEffect(() => { @@ -182,13 +214,10 @@ const Popper: React.FunctionComponent = props => { const child = typeof children === 'function' - ? (children as PopperChildrenFn)({ - placement: computedPlacement, - scheduleUpdate, - }) - : children + ? children({ placement: computedPlacement, scheduleUpdate }) + : (children as React.ReactElement) - return {React.Children.only(child) as React.ReactElement} + return {React.Children.only(child)} } Popper.defaultProps = { diff --git a/packages/react/src/utils/positioner/initialStyles.ts b/packages/react/src/utils/positioner/initialStyles.ts new file mode 100644 index 0000000000..8aad407cbb --- /dev/null +++ b/packages/react/src/utils/positioner/initialStyles.ts @@ -0,0 +1,13 @@ +import * as React from 'react' + +const initialStyles: React.CSSProperties = { + // Prevents scroll issue, waiting for Popper.js to add this style once initiated. + // https://github.com/mui-org/material-ui/issues/16740 + position: 'fixed', + // Fix Popper.js initial positioning display issue + // https://github.com/mui-org/material-ui/issues/17774 + top: 0, + left: '0px /* @noflip */', +} + +export default initialStyles diff --git a/packages/react/src/utils/positioner/types.ts b/packages/react/src/utils/positioner/types.ts index c2f67c3ab3..694f7e2f22 100644 --- a/packages/react/src/utils/positioner/types.ts +++ b/packages/react/src/utils/positioner/types.ts @@ -4,7 +4,7 @@ import PopperJS from 'popper.js' export type Position = 'above' | 'below' | 'before' | 'after' export type Alignment = 'top' | 'bottom' | 'start' | 'end' | 'center' -export type PopperChildrenFn = (props: PopperChildrenProps) => React.ReactNode +export type PopperChildrenFn = (props: PopperChildrenProps) => React.ReactElement export interface BasicPositioningProps { /** @@ -47,7 +47,7 @@ export interface PopperProps extends PositioningProps { /** * The content of the Popper box (the element that is going to be repositioned). */ - children: PopperChildrenFn | React.ReactNode + children: PopperChildrenFn | React.ReactElement /** * Enables events (resize, scroll).