-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: measure overlay natural width before positioning #10295
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
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 |
|---|---|---|
|
|
@@ -88,6 +88,91 @@ function Trigger(props: { | |
| ); | ||
| } | ||
|
|
||
| function GrowingContentTrigger(props: {placement: Placement}): JSX.Element { | ||
| const {placement} = props; | ||
| const targetRef = React.useRef<HTMLButtonElement>(null); | ||
| const state = useOverlayTriggerState({ | ||
| defaultOpen: false | ||
| }); | ||
| const {triggerProps, overlayProps} = useOverlayTrigger( | ||
| { | ||
| type: 'menu' | ||
| }, | ||
| state, | ||
| targetRef | ||
| ); | ||
|
|
||
| return ( | ||
| // Pin the trigger to the right viewport edge regardless of story decorators, | ||
| // so the overlay has to fit its width against the page boundary (issue #10050). | ||
| <div style={{position: 'fixed', top: 8, right: 8}}> | ||
| <button ref={targetRef} {...triggerProps} onClick={() => state.toggle()}> | ||
|
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. This file is pretty old, it looks like we never updated it after some changes because the new stories all render an error overlay due to onPress/etc coming through triggerProps. Easiest thing to do would be to make button a component using useButton and forward ref Something like: |
||
| Trigger (open: {`${state.isOpen}`}) | ||
| </button> | ||
| {state.isOpen && | ||
| ReactDOM.createPortal( | ||
| <GrowingContentOverlay | ||
| targetRef={targetRef} | ||
| overlayProps={overlayProps} | ||
| placement={placement} />, | ||
| document.body | ||
| )} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // The overlay is a separate component that mounts when the trigger opens, like | ||
| // usePopover-based components, so that useOverlayPosition's ResizeObserver is | ||
| // attached to the overlay element and repositions it when its content changes. | ||
| function GrowingContentOverlay(props: { | ||
| overlayProps: React.HTMLAttributes<HTMLElement>; | ||
| placement: Placement; | ||
| targetRef: React.RefObject<HTMLButtonElement | null>; | ||
| }): JSX.Element { | ||
| const {targetRef, overlayProps, placement} = props; | ||
| const overlayRef = React.useRef<HTMLDivElement>(null); | ||
| const {overlayProps: overlayPositionProps} = useOverlayPosition({ | ||
| targetRef, | ||
| overlayRef, | ||
| placement | ||
| }); | ||
|
|
||
| // Simulate content that renders after the initial positioning pass, | ||
| // e.g. menu items with descriptions populating on a second render (issue #10050). | ||
| const [showDescriptions, setShowDescriptions] = React.useState(false); | ||
| React.useEffect(() => { | ||
| const timeout = setTimeout(() => setShowDescriptions(true), 1500); | ||
| return () => clearTimeout(timeout); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={overlayRef} | ||
| {...mergeProps(overlayProps, overlayPositionProps)} | ||
| style={{ | ||
| ...overlayPositionProps.style, | ||
| boxShadow: '0 0 4px 0 rgba(0,0,0,0.25)', | ||
| backgroundColor: 'white', | ||
| overflow: 'auto' | ||
| }}> | ||
| <ul | ||
| style={{ | ||
| padding: 10, | ||
| margin: 0, | ||
| listStyleType: 'none' | ||
| }}> | ||
| {[...Array(3)].map((_, i) => ( | ||
| <li key={i}> | ||
| Menu item {i} | ||
| {showDescriptions && | ||
| ' — a longer description that widens the overlay after it has been positioned'} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| export default { | ||
| title: 'UseOverlayPosition' | ||
| }; | ||
|
|
@@ -147,3 +232,19 @@ export const MaxHeight200ContainerTop: TriggerStory = () => ( | |
| MaxHeight200ContainerTop.story = { | ||
| name: 'maxHeight=200 container top' | ||
| }; | ||
|
|
||
| export const GrowingContentRightEdge: StoryFn<typeof GrowingContentTrigger> = () => ( | ||
| <GrowingContentTrigger placement="bottom end" /> | ||
| ); | ||
|
|
||
| GrowingContentRightEdge.story = { | ||
| name: 'growing content at right page edge (#10050)' | ||
| }; | ||
|
|
||
| export const GrowingContentRightEdgeStart: StoryFn<typeof GrowingContentTrigger> = () => ( | ||
| <GrowingContentTrigger placement="start top" /> | ||
| ); | ||
|
|
||
| GrowingContentRightEdgeStart.story = { | ||
| name: 'growing content at right page edge, start top (#10050)' | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||
| */ | ||||||||
|
|
||||||||
| import {fireEvent, render} from '@react-spectrum/test-utils-internal'; | ||||||||
| import {I18nProvider} from '../../src/i18n/I18nProvider'; | ||||||||
| import React, {useRef} from 'react'; | ||||||||
| import {useOverlayPosition} from '../../src/overlays/useOverlayPosition'; | ||||||||
|
|
||||||||
|
|
@@ -47,6 +48,31 @@ function Example({ | |||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| function AutoWidthExample({triggerLeft = 360, ...props}) { | ||||||||
| let targetRef = useRef(null); | ||||||||
| let overlayRef = useRef(null); | ||||||||
| let {overlayProps, placement} = useOverlayPosition({ | ||||||||
| targetRef, | ||||||||
| overlayRef, | ||||||||
| ...props | ||||||||
| }); | ||||||||
| return ( | ||||||||
| <React.Fragment> | ||||||||
| <div | ||||||||
| ref={targetRef} | ||||||||
| data-testid="trigger" | ||||||||
| style={{left: triggerLeft, top: 250, width: 100, height: 100}}> | ||||||||
| Trigger | ||||||||
| </div> | ||||||||
| <div data-testid="container" style={{width: 500, height: 768}}> | ||||||||
| <div ref={overlayRef} data-testid="overlay" style={{height: 200, ...overlayProps.style}}> | ||||||||
| placement: {placement} | ||||||||
| </div> | ||||||||
| </div> | ||||||||
| </React.Fragment> | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| let original = window.HTMLElement.prototype.getBoundingClientRect; | ||||||||
| HTMLElement.prototype.getBoundingClientRect = function () { | ||||||||
| let rect = original.apply(this); | ||||||||
|
|
@@ -260,6 +286,172 @@ describe('useOverlayPosition', function () { | |||||||
| expect(arrow).toHaveAttribute('aria-hidden', 'true'); | ||||||||
| expect(arrow).toHaveAttribute('role', 'presentation'); | ||||||||
| }); | ||||||||
|
|
||||||||
| describe('auto-width overlay (shrink-to-fit)', function () { | ||||||||
| // The natural (preferred) width of the overlay's content. Mutated by tests to | ||||||||
| // simulate content that grows after the initial positioning pass (e.g. RAC | ||||||||
| // collections populating on a second render). | ||||||||
| let overlayNaturalWidth = 60; | ||||||||
|
|
||||||||
| beforeEach(() => { | ||||||||
| overlayNaturalWidth = 60; | ||||||||
|
|
||||||||
| // setupTests.js defines document.documentElement.clientWidth = 1024, which would | ||||||||
| // disagree with the 500px visualViewport/body used by this suite. Align them so | ||||||||
| // boundary, containing block, and viewport all share one width. | ||||||||
| Object.defineProperty(document.documentElement, 'clientWidth', { | ||||||||
| writable: true, | ||||||||
| configurable: true, | ||||||||
| value: 500 | ||||||||
| }); | ||||||||
|
|
||||||||
| // Simulate the browser's shrink-to-fit sizing of a position: absolute element | ||||||||
| // with width: auto: the available width is capped by the applied left/right | ||||||||
| // insets against a 500px containing block, and setting both insets stretches | ||||||||
| // the element between them. | ||||||||
| jest | ||||||||
| .spyOn(HTMLElement.prototype, 'offsetWidth', 'get') | ||||||||
| .mockImplementation(function (this: HTMLElement) { | ||||||||
| if (this.getAttribute?.('data-testid') === 'overlay') { | ||||||||
| let containingBlockWidth = 500; | ||||||||
| let hasLeft = this.style.left !== ''; | ||||||||
| let hasRight = this.style.right !== ''; | ||||||||
| if (hasLeft && hasRight) { | ||||||||
| return Math.max( | ||||||||
| containingBlockWidth - | ||||||||
| (parseInt(this.style.left, 10) || 0) - | ||||||||
| (parseInt(this.style.right, 10) || 0), | ||||||||
| 0 | ||||||||
| ); | ||||||||
| } | ||||||||
| let inset = hasLeft | ||||||||
| ? parseInt(this.style.left, 10) || 0 | ||||||||
| : hasRight | ||||||||
| ? parseInt(this.style.right, 10) || 0 | ||||||||
| : 0; | ||||||||
| return Math.min(overlayNaturalWidth, containingBlockWidth - inset); | ||||||||
| } | ||||||||
| return parseInt(this.style.width, 10) || 0; | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('should measure natural width when the content grows, not the width clamped by the stale position', function () { | ||||||||
| let res = render(<AutoWidthExample placement="bottom end" />); | ||||||||
| let overlay = res.getByTestId('overlay'); | ||||||||
|
|
||||||||
| // 60px wide overlay aligned to the trigger's right edge (360 + 100 - 60). | ||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| left: 400px; | ||||||||
| top: 350px; | ||||||||
| `); | ||||||||
|
|
||||||||
| // The content grows (e.g. menu items with descriptions render). In the browser | ||||||||
| // this fires the overlay's ResizeObserver; in jsdom useResizeObserver falls | ||||||||
| // back to the window resize event. | ||||||||
| overlayNaturalWidth = 300; | ||||||||
| fireEvent(window, new Event('resize')); | ||||||||
|
|
||||||||
| // The overlay must be measured at its natural width (300px) and repositioned in | ||||||||
| // a single pass (360 + 100 - 300). With a stale left of 400px the measurement | ||||||||
| // is clamped to 100px and the overlay only creeps toward its final position. | ||||||||
|
Comment on lines
+355
to
+356
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.
Suggested change
What did that sentence mean? |
||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| left: 160px; | ||||||||
| top: 350px; | ||||||||
| `); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('should not measure the overlay stretched between a stale right inset and the measurement reset', function () { | ||||||||
| // A physical left placement positions the overlay via a `right` inset. | ||||||||
| let res = render(<AutoWidthExample placement="left top" triggerLeft={180} />); | ||||||||
| let overlay = res.getByTestId('overlay'); | ||||||||
|
|
||||||||
| // 60px wide overlay anchored to the left of the trigger: right = 500 - 180. | ||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| right: 320px; | ||||||||
| top: 250px; | ||||||||
| `); | ||||||||
| expect(overlay).toHaveTextContent('placement: left'); | ||||||||
|
|
||||||||
| // The content grows, but still fits to the left of the trigger (100px < 168px | ||||||||
| // of available space), so the overlay must stay on the left. | ||||||||
| overlayNaturalWidth = 100; | ||||||||
| fireEvent(window, new Event('resize')); | ||||||||
|
|
||||||||
| // If the measurement reset sets left: 0px while the stale right: 320px is still | ||||||||
| // applied, the overlay is stretched between both insets (500 - 0 - 320 = 180px), | ||||||||
| // over-measuring its width and spuriously flipping it to the right. | ||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| right: 320px; | ||||||||
| top: 250px; | ||||||||
| `); | ||||||||
| expect(overlay).toHaveTextContent('placement: left'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('should measure natural width when the user provides a maxHeight', function () { | ||||||||
| // A user maxHeight only affects the vertical measurement reset; the horizontal | ||||||||
| // reset must happen regardless. | ||||||||
| let res = render(<AutoWidthExample placement="bottom end" maxHeight={300} />); | ||||||||
| let overlay = res.getByTestId('overlay'); | ||||||||
|
|
||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| left: 400px; | ||||||||
| top: 350px; | ||||||||
| `); | ||||||||
|
|
||||||||
| overlayNaturalWidth = 300; | ||||||||
| fireEvent(window, new Event('resize')); | ||||||||
|
|
||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| left: 160px; | ||||||||
| top: 350px; | ||||||||
| `); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('should measure natural width in RTL, where end placements position via a right inset', function () { | ||||||||
| let res = render( | ||||||||
| <I18nProvider locale="ar-AE"> | ||||||||
| <AutoWidthExample placement="end top" triggerLeft={180} /> | ||||||||
| </I18nProvider> | ||||||||
| ); | ||||||||
| let overlay = res.getByTestId('overlay'); | ||||||||
|
|
||||||||
| // In RTL, 'end top' resolves to the physical 'left top' placement. | ||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| right: 320px; | ||||||||
| top: 250px; | ||||||||
| `); | ||||||||
| expect(overlay).toHaveTextContent('placement: left'); | ||||||||
|
|
||||||||
| overlayNaturalWidth = 100; | ||||||||
| fireEvent(window, new Event('resize')); | ||||||||
|
|
||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| right: 320px; | ||||||||
| top: 250px; | ||||||||
| `); | ||||||||
| expect(overlay).toHaveTextContent('placement: left'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('should leave only one horizontal inset applied after the placement changes', function () { | ||||||||
| let res = render(<AutoWidthExample placement="left top" triggerLeft={180} />); | ||||||||
| let overlay = res.getByTestId('overlay'); | ||||||||
|
|
||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| right: 320px; | ||||||||
| top: 250px; | ||||||||
| `); | ||||||||
|
|
||||||||
| res.rerender(<AutoWidthExample placement="bottom end" triggerLeft={180} />); | ||||||||
|
|
||||||||
| // 180 + 100 - 60 = 220. The stale right inset from the previous placement must | ||||||||
|
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.
Was that 320? or what is this referring to? |
||||||||
| // be gone so the overlay is positioned by exactly one horizontal inset. | ||||||||
| expect(overlay).toHaveStyle(` | ||||||||
| left: 220px; | ||||||||
| top: 350px; | ||||||||
| `); | ||||||||
| expect(overlay.style.right).toBe(''); | ||||||||
| }); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| describe('useOverlayPosition with positioned container', () => { | ||||||||
|
|
||||||||
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.
Are there any cases where we don't want to do the style changes? It looks like we only do it sometimes for top/bottom.
What if a width is already specified for the overlay, then we shouldn't need to do this?
Or what if the position is already left?
In general this seems more correct compared to the previous attempt.