diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 93591222faad9..ad2e5be8e7121 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -5,6 +5,10 @@ - `withFilters` has been optimized to avoid binding hook handlers for each mounted instance of the component, instead using a single centralized hook delegator. - `withFilters` has been optimized to reuse a single shared component definition for all filtered instances of the component. +### Bug Fixes + +- Resolves a conflict where two instance of Slot would produce an inconsistent or duplicated rendering output. + ## 7.0.5 (2019-01-03) ## 7.0.4 (2018-12-12) diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js index df4476fea5b9c..a39ac94013e0a 100644 --- a/packages/components/src/slot-fill/context.js +++ b/packages/components/src/slot-fill/context.js @@ -41,12 +41,21 @@ class SlotFillProvider extends Component { } registerSlot( name, slot ) { + const previousSlot = this.slots[ name ]; this.slots[ name ] = slot; this.forceUpdateFills( name ); // Sometimes the fills are registered after the initial render of slot // But before the registerSlot call, we need to rerender the slot this.forceUpdateSlot( name ); + + // If a new instance of a slot is being mounted while another with the + // same name exists, force its update _after_ the new slot has been + // assigned into the instance, such that its own rendering of children + // will be empty (the new Slot will subsume all fills for this name). + if ( previousSlot ) { + previousSlot.forceUpdate(); + } } registerFill( name, instance ) { @@ -57,7 +66,14 @@ class SlotFillProvider extends Component { this.forceUpdateSlot( name ); } - unregisterSlot( name ) { + unregisterSlot( name, instance ) { + // If a previous instance of a Slot by this name unmounts, do nothing, + // as the slot and its fills should only be removed for the current + // known instance. + if ( this.slots[ name ] !== instance ) { + return; + } + delete this.slots[ name ]; this.forceUpdateFills( name ); } @@ -75,7 +91,13 @@ class SlotFillProvider extends Component { return this.slots[ name ]; } - getFills( name ) { + getFills( name, slotInstance ) { + // Fills should only be returned for the current instance of the slot + // in which they occupy. + if ( this.slots[ name ] !== slotInstance ) { + return []; + } + return sortBy( this.fills[ name ], 'occurrence' ); } diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index 66b025ca00178..7c2b3fb0ad5a2 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -62,7 +62,7 @@ class FillComponent extends Component { let { children } = this.props; const slot = getSlot( name ); - if ( ! slot || ! slot.props.bubblesVirtually ) { + if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) { return null; } diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js index 4f53cd6d10300..13a226eab5de5 100644 --- a/packages/components/src/slot-fill/slot.js +++ b/packages/components/src/slot-fill/slot.js @@ -63,7 +63,7 @@ class SlotComponent extends Component { return
; } - const fills = map( getFills( name ), ( fill ) => { + const fills = map( getFills( name, this ), ( fill ) => { const fillKey = fill.occurrence; const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children; diff --git a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap index 709d1522fcbf4..79b43ad4c65c0 100644 --- a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap +++ b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap @@ -1,5 +1,59 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Slot bubblesVirtually false should subsume another slot by the same name 1`] = ` +Array [ +
, +
+ Content +
, +] +`; + +exports[`Slot bubblesVirtually false should subsume another slot by the same name 2`] = ` +Array [ +
, +
+ Content +
, +] +`; + +exports[`Slot bubblesVirtually true should subsume another slot by the same name 1`] = ` +Array [ +
+
+
, +
+
+
, +] +`; + +exports[`Slot bubblesVirtually true should subsume another slot by the same name 2`] = ` +Array [ +
, +
+
+
, +] +`; + exports[`Slot should re-render Slot when not bubbling virtually 1`] = ` Array [
diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js index a12c9cd99c6ca..47ba2e16de524 100644 --- a/packages/components/src/slot-fill/test/slot.js +++ b/packages/components/src/slot-fill/test/slot.js @@ -209,4 +209,48 @@ describe( 'Slot', () => { expect( testRenderer.toJSON() ).toMatchSnapshot(); } ); + + [ false, true ].forEach( ( bubblesVirtually ) => { + describe( 'bubblesVirtually ' + bubblesVirtually, () => { + it( 'should subsume another slot by the same name', () => { + const testRenderer = ReactTestRenderer.create( + +
+ +
+
+ Content + + ); + + testRenderer.update( + +
+ +
+
+ +
+ Content + + ); + + expect( testRenderer.toJSON() ).toMatchSnapshot(); + + testRenderer.update( + +
+
+ +
+ Content + + ); + + expect( testRenderer.toJSON() ).toMatchSnapshot(); + + expect( testRenderer.getInstance().slots ).toHaveProperty( 'egg' ); + } ); + } ); + } ); } );