From 81573c2b689bf23101e69fa834fd7ca5ba29ff87 Mon Sep 17 00:00:00 2001
From: Ilyas Foo <foo.ilyas@gmail.com>
Date: Thu, 6 May 2021 15:07:45 +0800
Subject: [PATCH] Fix SelectControl focus and de-focus bug
 (https://github.com/woocommerce/woocommerce-admin/pull/6906)

* Fix select-control component to preserve currently selected item on focus and de-focus

* No longer auto select option on initial render, pressing tab would only select if selection exists

* Fix to reflect the proper suggested changes

* Attempt to fix unnecessary search reset on pressing tab

* Fix for keyboard interactions and added accompanying tests
---
 .../packages/components/CHANGELOG.md          |   2 +
 .../components/src/select-control/index.js    |  33 ++-
 .../components/src/select-control/list.js     |  13 +-
 .../src/select-control/stories/index.js       |  15 ++
 .../src/select-control/test/index.js          | 232 ++++++++++++++++++
 plugins/woocommerce-admin/readme.txt          |   1 +
 6 files changed, 288 insertions(+), 8 deletions(-)

diff --git a/plugins/woocommerce-admin/packages/components/CHANGELOG.md b/plugins/woocommerce-admin/packages/components/CHANGELOG.md
index 1d77e484c2f..02c091ffd0b 100644
--- a/plugins/woocommerce-admin/packages/components/CHANGELOG.md
+++ b/plugins/woocommerce-admin/packages/components/CHANGELOG.md
@@ -1,5 +1,7 @@
 # Unreleased
 
+-   SelectControl: automatically scroll to selected options when list is displayed. #6906
+-   SelectControl: no longer auto selects on rendering list. #6906
 -   Make `Search` accept synchronous `autocompleter.options`. #6884
 -   Add new (experimental) collapsible list item to collapse list items. #6869
 -   SelectControl: fix display of multiple selections without inline tags. #6862
diff --git a/plugins/woocommerce-admin/packages/components/src/select-control/index.js b/plugins/woocommerce-admin/packages/components/src/select-control/index.js
index e2359fd3ba2..1f39da76220 100644
--- a/plugins/woocommerce-admin/packages/components/src/select-control/index.js
+++ b/plugins/woocommerce-admin/packages/components/src/select-control/index.js
@@ -25,10 +25,15 @@ const initialState = { isExpanded: false, isFocused: false, query: '' };
 export class SelectControl extends Component {
 	constructor( props ) {
 		super( props );
+
+		const { selected, options, excludeSelectedOptions } = props;
 		this.state = {
 			...initialState,
 			searchOptions: [],
-			selectedIndex: 0,
+			selectedIndex:
+				selected && options?.length && ! excludeSelectedOptions
+					? options.findIndex( ( option ) => option.key === selected )
+					: null,
 		};
 
 		this.bindNode = this.bindNode.bind( this );
@@ -50,11 +55,16 @@ export class SelectControl extends Component {
 	}
 
 	reset( selected = this.getSelected() ) {
-		const { multiple } = this.props;
+		const { multiple, excludeSelectedOptions } = this.props;
 		const newState = { ...initialState };
-		// Reset to the option label if single selection.
+		// Reset to the option label and selectedIndex if single selection.
 		if ( ! multiple && selected.length && selected[ 0 ].label ) {
 			newState.query = selected[ 0 ].label;
+			newState.selectedIndex = ! excludeSelectedOptions
+				? this.props.options.findIndex(
+						( i ) => i.key === selected[ 0 ].key
+				  )
+				: null;
 		}
 
 		this.setState( newState );
@@ -107,6 +117,17 @@ export class SelectControl extends Component {
 		if ( isSelected === -1 ) {
 			this.setNewValue( newSelected );
 		}
+
+		// After selecting option, the list will reset and we'd need to correct selectedIndex.
+		const newSelectedIndex = this.props.excludeSelectedOptions
+			? // Since we're excluding the selected option, invalidate selection
+			  // so re-focusing wont immediately set it to the neigbouring option.
+			  null
+			: this.getOptions().findIndex( ( i ) => i.key === option.key );
+
+		this.setState( {
+			selectedIndex: newSelectedIndex,
+		} );
 	}
 
 	setNewValue( newValue ) {
@@ -233,8 +254,9 @@ export class SelectControl extends Component {
 			{
 				query,
 				isFocused: true,
-				selectedIndex: 0,
 				searchOptions,
+				selectedIndex:
+					query?.length > 0 ? null : this.state.selectedIndex, // Only reset selectedIndex if we're actually searching.
 			},
 			() => {
 				this.setState( {
@@ -268,8 +290,9 @@ export class SelectControl extends Component {
 
 			this.setState(
 				{
-					selectedIndex: 0,
 					searchOptions,
+					selectedIndex:
+						query?.length > 0 ? null : this.state.selectedIndex, // Only reset selectedIndex if we're actually searching.
 				},
 				() => {
 					this.setState( {
diff --git a/plugins/woocommerce-admin/packages/components/src/select-control/list.js b/plugins/woocommerce-admin/packages/components/src/select-control/list.js
index 26a1a73a015..6d6bf3d5155 100644
--- a/plugins/woocommerce-admin/packages/components/src/select-control/list.js
+++ b/plugins/woocommerce-admin/packages/components/src/select-control/list.js
@@ -100,7 +100,9 @@ class List extends Component {
 				break;
 
 			case ENTER:
-				this.select( options[ selectedIndex ] );
+				if ( options[ selectedIndex ] ) {
+					this.select( options[ selectedIndex ] );
+				}
 				event.preventDefault();
 				event.stopPropagation();
 				break;
@@ -116,9 +118,10 @@ class List extends Component {
 				return;
 
 			case TAB:
-				this.select( options[ selectedIndex ] );
+				if ( options[ selectedIndex ] ) {
+					this.select( options[ selectedIndex ] );
+				}
 				setExpanded( false );
-				onSearch( null );
 				break;
 
 			default:
@@ -139,6 +142,10 @@ class List extends Component {
 	}
 
 	componentDidMount() {
+		const { selectedIndex } = this.props;
+		if ( selectedIndex > -1 ) {
+			this.scrollToOption( selectedIndex );
+		}
 		this.toggleKeyEvents( true );
 	}
 
diff --git a/plugins/woocommerce-admin/packages/components/src/select-control/stories/index.js b/plugins/woocommerce-admin/packages/components/src/select-control/stories/index.js
index 1c6ecec4dce..a7ba061a06b 100644
--- a/plugins/woocommerce-admin/packages/components/src/select-control/stories/index.js
+++ b/plugins/woocommerce-admin/packages/components/src/select-control/stories/index.js
@@ -59,6 +59,7 @@ const SelectControlExample = withState( {
 	singleSelectedShowAll: [],
 	multipleSelected: [],
 	inlineSelected: [],
+	allOptionsIncludingSelected: options[ options.length - 1 ].key,
 } )(
 	( {
 		simpleSelected,
@@ -67,6 +68,7 @@ const SelectControlExample = withState( {
 		singleSelectedShowAll,
 		multipleSelected,
 		inlineSelected,
+		allOptionsIncludingSelected,
 		setState,
 	} ) => (
 		<div>
@@ -91,6 +93,19 @@ const SelectControlExample = withState( {
 				selected={ simpleMultipleSelected }
 			/>
 			<br />
+			<SelectControl
+				label="Show all options with default selected"
+				onChange={ ( selected ) =>
+					setState( { allOptionsIncludingSelected: selected } )
+				}
+				options={ options }
+				placeholder="Start typing to filter options..."
+				selected={ allOptionsIncludingSelected }
+				showAllOnFocus
+				isSearchable
+				excludeSelectedOptions={ false }
+			/>
+			<br />
 			<SelectControl
 				label="Single value searchable"
 				isSearchable
diff --git a/plugins/woocommerce-admin/packages/components/src/select-control/test/index.js b/plugins/woocommerce-admin/packages/components/src/select-control/test/index.js
index 9ade19730a5..990ee7a9f10 100644
--- a/plugins/woocommerce-admin/packages/components/src/select-control/test/index.js
+++ b/plugins/woocommerce-admin/packages/components/src/select-control/test/index.js
@@ -201,6 +201,25 @@ describe( 'SelectControl', () => {
 		expect( getAllByRole( 'option' ) ).toHaveLength( 1 );
 	} );
 
+	it( 'should not automatically select first option on focus', async () => {
+		const onChangeMock = jest.fn();
+		const { getByRole, queryByRole } = render(
+			<SelectControl
+				isSearchable
+				showAllOnFocus
+				options={ options }
+				onSearch={ () => options }
+				onFilter={ () => options }
+				onChange={ onChangeMock }
+			/>
+		);
+		getByRole( 'combobox' ).focus();
+		await waitFor( () =>
+			expect( getByRole( 'option', { name: 'bar' } ) ).toBeInTheDocument()
+		);
+		expect( queryByRole( 'option', { selected: true } ) ).toBeNull();
+	} );
+
 	describe( 'selected value', () => {
 		it( 'should return an array if array', async () => {
 			const onChangeMock = jest.fn();
@@ -260,6 +279,72 @@ describe( 'SelectControl', () => {
 			);
 		} );
 
+		describe( 'prop excludeSelectedOptions', () => {
+			it( 'should preserve selected option when focused', async () => {
+				const onChangeMock = jest.fn();
+				const { getByRole } = render(
+					<SelectControl
+						isSearchable
+						showAllOnFocus
+						selected={ options[ 2 ].key }
+						options={ options }
+						onSearch={ () => options }
+						onFilter={ () => options }
+						onChange={ onChangeMock }
+						excludeSelectedOptions={ false }
+					/>
+				);
+				getByRole( 'combobox' ).focus();
+				await waitFor( () =>
+					expect(
+						getByRole( 'option', { name: 'bar' } )
+					).toBeInTheDocument()
+				);
+				// In browser, the <Button> in <List> component is automatically "selected" when <Control> lost focus.
+				// I was not able to produce the same behaviour with unit test, but a click on the currently
+				// selected option should be sufficient to simulate the logic in this test.
+				userEvent.click( getByRole( 'option', { selected: true } ) );
+				expect( onChangeMock ).toHaveBeenCalledTimes( 0 );
+			} );
+
+			it( 'should reset selected option if searching', async () => {
+				const onChangeMock = jest.fn();
+				const { getByRole, queryByRole } = render(
+					<SelectControl
+						isSearchable
+						showAllOnFocus
+						selected={ options[ 2 ].key }
+						options={ options }
+						onSearch={ () => options }
+						onFilter={ () => options }
+						onChange={ onChangeMock }
+						excludeSelectedOptions={ false }
+					/>
+				);
+
+				getByRole( 'combobox' ).focus();
+				await waitFor( () =>
+					expect(
+						getByRole( 'option', { name: 'bar' } )
+					).toBeInTheDocument()
+				);
+
+				userEvent.click( getByRole( 'option', { name: 'bar' } ) );
+				userEvent.clear( getByRole( 'combobox' ) );
+				userEvent.type( getByRole( 'combobox' ), 'bar' );
+
+				await waitFor( () =>
+					expect(
+						getByRole( 'option', { name: 'bar' } )
+					).toBeInTheDocument()
+				);
+
+				expect(
+					queryByRole( 'option', { selected: true } )
+				).toBeNull();
+			} );
+		} );
+
 		it( 'disables the component', async () => {
 			const { getByRole } = render(
 				<SelectControl disabled options={ options } />
@@ -324,4 +409,151 @@ describe( 'SelectControl', () => {
 
 		expect( getByText( options[ 1 ].label ) ).toBeInTheDocument();
 	} );
+
+	describe( 'keyboard interaction', () => {
+		it( 'pressing keydown on combobox should highlight first option', async () => {
+			const { getByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ null }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{arrowdown}' );
+
+			expect(
+				getByRole( 'option', { selected: true } ).textContent
+			).toEqual( options[ 0 ].label );
+		} );
+
+		it( 'pressing keydown on combobox with selected should highlight the next option', async () => {
+			const { getByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ options[ 1 ].key }
+					excludeSelectedOptions={ false }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{arrowdown}' );
+
+			expect(
+				getByRole( 'option', { selected: true } ).textContent
+			).toEqual( options[ 2 ].label );
+		} );
+
+		it( 'pressing keydown on combobox with selected last option should rotate to first option', async () => {
+			const { getByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ options[ options.length - 1 ].key }
+					excludeSelectedOptions={ false }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{arrowdown}' );
+
+			expect(
+				getByRole( 'option', { selected: true } ).textContent
+			).toEqual( options[ 0 ].label );
+		} );
+
+		it( 'pressing keyup on combobox should highlight last option', async () => {
+			const { getByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ null }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{arrowup}' );
+
+			expect(
+				getByRole( 'option', { selected: true } ).textContent
+			).toEqual( options[ options.length - 1 ].label );
+		} );
+
+		it( 'pressing tab on combobox should hide option list', async () => {
+			const { getByRole, queryByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ null }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{tab}' );
+
+			expect( queryByRole( 'option', { selected: true } ) ).toBeNull();
+		} );
+
+		it( 'pressing enter should select highlighted option', async () => {
+			const onChangeMock = jest.fn();
+			const { getByRole } = render(
+				<SelectControl
+					options={ options }
+					onSearch={ () => options }
+					onFilter={ () => options }
+					selected={ null }
+					excludeSelectedOptions={ false }
+					onChange={ onChangeMock }
+				/>
+			);
+
+			getByRole( 'combobox' ).focus();
+			await waitFor( () =>
+				expect(
+					getByRole( 'option', { name: 'bar' } )
+				).toBeInTheDocument()
+			);
+
+			userEvent.type( getByRole( 'combobox' ), '{arrowdown}{enter}' );
+
+			expect( onChangeMock ).toHaveBeenCalled();
+		} );
+	} );
 } );
diff --git a/plugins/woocommerce-admin/readme.txt b/plugins/woocommerce-admin/readme.txt
index bc8d5bb6959..05279d00d12 100644
--- a/plugins/woocommerce-admin/readme.txt
+++ b/plugins/woocommerce-admin/readme.txt
@@ -119,6 +119,7 @@ Release and roadmap notes are available on the [WooCommerce Developers Blog](htt
 - Fix: Get currency from CurrencyContext #6723
 - Fix: Correct the left position of transient notices when the new nav is used. #6914
 - Fix: Exclude WC Shipping for store that are only offering downloadable products #6917
+- Fix: SelectControl focus and de-focus bug #6906
 - Performance: Avoid updating customer info synchronously from the front end. #6765
 - Tweak: Add settings_section event prop for CES #6762
 - Tweak: Refactor payments to allow management of methods #6786