fix(portal): replay queued portal operations in order - #5048
Conversation
|
Thanks @ziarno. Same situation as #5046: Nothing needed from me that I can see. Branch is clean against |
| // The last portal in source order is painted on top, so `dialog` has to come | ||
| // after `modal` in the rendered tree. |
There was a problem hiding this comment.
| // The last portal in source order is painted on top, so `dialog` has to come | |
| // after `modal` in the rendered tree. |
|
|
||
| it('keeps queued mounts of other portals when one of them is updated', async () => { | ||
| // Mirrors `PortalConsumer`: mounts from `componentDidMount`, then updates its | ||
| // own key. Both calls land before `PortalHost` attaches its manager, so they | ||
| // go through the queue. | ||
| class QueuedConsumer extends React.Component<{ | ||
| manager: PortalMethods; | ||
| label: string; | ||
| update?: boolean; | ||
| }> { | ||
| componentDidMount() { | ||
| const key = this.props.manager.mount( | ||
| <Text testID="queued">{this.props.label}</Text> | ||
| ); | ||
|
|
||
| if (this.props.update) { | ||
| this.props.manager.update( | ||
| key, | ||
| <Text testID="queued">{`${this.props.label} (updated)`}</Text> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| await render( | ||
| <PortalHost> | ||
| <PortalContext.Consumer> | ||
| {(manager) => ( | ||
| <> | ||
| <QueuedConsumer manager={manager} label="first" /> | ||
| <QueuedConsumer manager={manager} label="second" update /> | ||
| </> | ||
| )} | ||
| </PortalContext.Consumer> | ||
| </PortalHost> | ||
| ); | ||
|
|
||
| // Deliberately order-independent: this asserts that no portal is lost, which | ||
| // is a separate concern from the order the queue is replayed in. | ||
| expect(screen.getAllByTestId('queued')).toHaveLength(2); | ||
| expect(screen.getByText('first')).toBeTruthy(); | ||
| expect(screen.getByText('second (updated)')).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
this is testing implementation details. please update it to use public behavior by checking what's rendered and performing operations with a normal protal without consuming the portal context directly
There was a problem hiding this comment.
Rewritten: the tests now render plain <Portal> elements inside a Portal.Host and assert on the rendered output with findAllByTestId + toHaveTextContent, so nothing touches the portal context. Order is observable from the outside because PortalManager renders its portals in array order, so a LIFO replay puts third, second, first into the tree and the assertion fails on visible text; the Modal/Dialog test is the same thing with real components.
I also dropped the second fix and the test you flagged. I instrumented the queued update() branch and ran the full suite: nothing but that test reaches it, since PortalConsumer only calls update() from componentDidUpdate and React flushes a layout-phase setState after the manager ref is already attached. I would rather not ship a change I cannot cover with public behaviour, so this PR is now just the one-line ordering fix.
`PortalHost` queues portal operations that arrive before its `PortalManager` ref is attached, which is every portal that mounts in the first commit. `componentDidMount` drained that queue with `pop()`, replaying the operations LIFO, so portals mounted in the same commit were stacked in reverse source order. Drain with `shift()` instead.
46879a8 to
c360ef7
Compare
Summary
Portal.Hostreplays its pending-operation queue in the wrong order, so portals that mount in the same commit are stacked in reverse source order.One line in
src/components/Portal/PortalHost.tsx, plus tests.Why the queue is used at all
PortalHostrenders<PortalManager ref={this.setManager} />as a sibling afterthis.props.children. React runs the children'scomponentDidMountbefore the parent's, so everyPortalthat mounts in the first commit callsmount()whilethis.manageris stillnulland gets pushed ontothis.queue.PortalHost.componentDidMountthen drains it.In other words the queue is not an edge case. It is the path every portal present on first render takes.
The bug
Portal z-order is document order, so replaying the queue backwards stacks the portals backwards. With three sibling portals under one host, the rendered order today is
third, second, first.The fix
Tests
Two tests in
src/components/__tests__/Portal.test.tsx, both public behaviour only: realPortalelements under aPortal.Host, assertions on what is rendered.renders portals in source order when mounted in the same commitasserts the three rendered portals readfirst, second, third. Onmainthey readthird, second, first.stacks components mounted in the same commit in source orderis the user-visible version, with a realModalandDialog. OnmaintheDialogrenders underneath theModal.Reverting the one-line change turns both red, with the reversed text visible in the failure output.
Full suite: 55 suites, 679 passed / 1 skipped, 168 snapshots, no snapshot churn.
yarn lint,yarn typecheckandprettier --checkare clean.Behavioural impact, please read
This changes stacking order for apps that mount more than one portal in the same commit (for example a
Modalplus aSnackbarplus aDialogrendered together on first paint). Apps that were silently compensating for the reversal, by reordering their JSX to get the layering they wanted, will see their layers flip.I think source order is the correct semantics and worth the change:
manager.mount()and is appended in the correct place. So today the same three portals stack one way on first render and the other way if they are mounted a tick later. That inconsistency is the part that is hardest to work around.Still, it is a real behavioural change rather than a pure internal fix, so it may deserve a note in the changelog or a minor rather than a patch release. Happy to adjust.
Changed since review
@satya164 flagged the third test as testing implementation details. That test drove the queue through
PortalContextdirectly, and it was the only cover for a second change that used to be in this PR: theupdate()lookup matched the first queued mount of any key instead of the given key.Before rewriting it I checked whether that second bug is reachable without touching the context. I instrumented the queued
update()branch and ran the whole suite: nothing except the removed test reaches it.PortalConsumeronly callsupdate()fromcomponentDidUpdate, and React flushes a layout-phasesetStateafter all of the commit's layout effects, by which point the manager ref is attached, so a plainPortalcan never callupdate()while the queue is still live.Rather than ship a change I cannot cover with a public-behaviour test, I dropped it. This PR is now the one-line ordering fix only, which matches its title. Happy to raise the other one separately if you want it as hardening.
Context
Related to #4647, which asks how to control the z-index of multiple portals. That issue is a question rather than a confirmed bug report and I do not want to overstate it. It is context for why the ordering matters, not a repro.