From cb10bcffde12b2b9196ae817285d3cbab684ddee Mon Sep 17 00:00:00 2001
From: Lena Morita <lena@jaguchi.com>
Date: Wed, 27 Jul 2022 00:11:03 +0900
Subject: [PATCH] Context System: Don't explicitly set `undefined` value to
 `children` (#42686)

* Add tests

* Simplify tests

* Compare with normal component behavior

* Components: Allow rendering `Icon` via `as` prop

* Add changelog

* Tweak changelog
---
 packages/components/CHANGELOG.md              |   1 +
 .../context/test/context-system-provider.js   | 102 +++++++++++++++++-
 .../src/ui/context/use-context-system.js      |   9 +-
 3 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
index 15db9f808ba3b..bac27e61ad0e5 100644
--- a/packages/components/CHANGELOG.md
+++ b/packages/components/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Bug Fix
 
+-   Context System: Stop explicitly setting `undefined` to the `children` prop. This fixes a bug where `Icon` could not be correctly rendered via the `as` prop of a context-connected component ([#42686](https://github.com/WordPress/gutenberg/pull/42686)).
 -   `Popover`, `Dropdown`: Fix width when `expandOnMobile` is enabled ([#42635](https://github.com/WordPress/gutenberg/pull/42635/)).
 -   `CustomSelectControl`: Fix font size and hover/focus style inconsistencies with `SelectControl` ([#42460](https://github.com/WordPress/gutenberg/pull/42460/)).
 -   `AnglePickerControl`: Fix gap between elements in RTL mode ([#42534](https://github.com/WordPress/gutenberg/pull/42534)).
diff --git a/packages/components/src/ui/context/test/context-system-provider.js b/packages/components/src/ui/context/test/context-system-provider.js
index 777a3bdf2dba6..e8c2b5c56286f 100644
--- a/packages/components/src/ui/context/test/context-system-provider.js
+++ b/packages/components/src/ui/context/test/context-system-provider.js
@@ -1,9 +1,14 @@
 /**
  * External dependencies
  */
-import { render } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
 import styled from '@emotion/styled';
 
+/**
+ * WordPress dependencies
+ */
+import { cloneElement } from '@wordpress/element';
+
 /**
  * Internal dependencies
  */
@@ -101,3 +106,98 @@ describe( 'props', () => {
 		expect( el.innerHTML ).not.toContain( 'WordPress.org' );
 	} );
 } );
+
+describe( 'children', () => {
+	test( 'should pass through children', () => {
+		const Component = ( props, ref ) => (
+			<View { ...useContextSystem( props, 'Component' ) } ref={ ref } />
+		);
+		const ConnectedComponent = contextConnect( Component, 'Component' );
+
+		render(
+			<ContextSystemProvider>
+				<ConnectedComponent>Pass through</ConnectedComponent>
+			</ContextSystemProvider>
+		);
+
+		expect( screen.getByText( 'Pass through' ) ).toBeInTheDocument();
+	} );
+
+	test( 'should not accept children via `context`', () => {
+		const Component = ( props, ref ) => (
+			<View { ...useContextSystem( props, 'Component' ) } ref={ ref } />
+		);
+		const ConnectedComponent = contextConnect( Component, 'Component' );
+
+		render(
+			<ContextSystemProvider
+				context={ { Component: { children: 'Override' } } }
+			>
+				<ConnectedComponent />
+			</ContextSystemProvider>
+		);
+
+		expect( screen.queryByText( 'Override' ) ).not.toBeInTheDocument();
+	} );
+
+	// This matches the behavior for normal, non-context-connected components.
+	test( 'should not override inherent children', () => {
+		const Component = ( props, ref ) => (
+			<View { ...useContextSystem( props, 'Component' ) } ref={ ref }>
+				Inherent
+			</View>
+		);
+		const ConnectedComponent = contextConnect( Component, 'Component' );
+		const NormalComponent = ( props ) => <div { ...props }>Inherent</div>;
+
+		render(
+			<ContextSystemProvider>
+				<ConnectedComponent />
+				<ConnectedComponent>Explicit children</ConnectedComponent>
+				<NormalComponent />
+				<NormalComponent>Explicit children</NormalComponent>
+			</ContextSystemProvider>
+		);
+
+		expect( screen.getAllByText( 'Inherent' ) ).toHaveLength( 4 );
+	} );
+
+	describe( 'when connected component does a `cloneElement()`', () => {
+		// eslint-disable-next-line no-unused-vars
+		const ComponentThatClones = ( { content, ...props }, _ref ) =>
+			cloneElement(
+				content,
+				useContextSystem( props, 'ComponentThatClones' )
+			);
+		const ConnectedComponentThatClones = contextConnect(
+			ComponentThatClones,
+			'ComponentThatClones'
+		);
+
+		test( 'should not override cloned inherent children with implicit `undefined` children', () => {
+			render(
+				<ContextSystemProvider>
+					<ConnectedComponentThatClones
+						content={ <span>Inherent</span> }
+					/>
+				</ContextSystemProvider>
+			);
+			expect( screen.getByText( 'Inherent' ) ).toBeInTheDocument();
+		} );
+
+		test( 'should override cloned inherent children with explicit children', () => {
+			render(
+				<ContextSystemProvider>
+					<ConnectedComponentThatClones
+						content={ <span>Inherent</span> }
+					>
+						Explicit children
+					</ConnectedComponentThatClones>
+				</ContextSystemProvider>
+			);
+			expect(
+				screen.getByText( 'Explicit children' )
+			).toBeInTheDocument();
+		} );
+	} );
+} );
diff --git a/packages/components/src/ui/context/use-context-system.js b/packages/components/src/ui/context/use-context-system.js
index 134ef9046b849..07297ddb30f7e 100644
--- a/packages/components/src/ui/context/use-context-system.js
+++ b/packages/components/src/ui/context/use-context-system.js
@@ -71,8 +71,13 @@ export function useContextSystem( props, namespace ) {
 		finalComponentProps[ key ] = overrideProps[ key ];
 	}
 
-	// @ts-ignore
-	finalComponentProps.children = rendered;
+	// Setting an `undefined` explicitly can cause unintended overwrites
+	// when a `cloneElement()` is involved.
+	if ( rendered !== undefined ) {
+		// @ts-ignore
+		finalComponentProps.children = rendered;
+	}
+
 	finalComponentProps.className = classes;
 
 	return finalComponentProps;