Hold focus inside the library's two panels

packages/app/src/hooks/useStartLetter.js

@@ -2,7 +2,7 @@
 // narrows the grid to it and hands focus to the results, so the next press moves through
 // them rather than along the letters. Picking the same letter again clears it.
 
-import {useCallback, useEffect, useMemo, useState} from 'react';
+import {useCallback, useEffect, useMemo, useRef, useState} from 'react';
 import Spotlight from '@enact/spotlight';
 
 import {filterByStartLetter} from '../utils/gridChrome';
@@ -19,9 +19,23 @@ const useStartLetter = ({allItems, isLoading, gridSpotlightId}) => {
 		}
 	}, [startLetter]);
 
+	// Which letter the grid was last handed focus for. The effect below has to
+	// watch the rebuilt list to know when to move, and that list also rebuilds
+	// for a filter or a search the viewer ran from somewhere else entirely.
+	// Without this it would answer those too and pull focus out of whatever
+	// panel they were working in.
+	const focusedForLetterRef = useRef(null);
+
 	// The grid rebuilds around the narrower list, so the focus waits for it to settle.
 	useEffect(() => {
-		if (!startLetter || items.length === 0 || isLoading) return undefined;
+		if (!startLetter) {
+			// Cleared, so the same letter picked again is a fresh pick.
+			focusedForLetterRef.current = null;
+			return undefined;
+		}
+		if (items.length === 0 || isLoading) return undefined;
+		if (focusedForLetterRef.current === startLetter) return undefined;
+		focusedForLetterRef.current = startLetter;
 		const id = setTimeout(() => Spotlight.focus(gridSpotlightId), 100);
 		return () => clearTimeout(id);
 	}, [startLetter, items.length, isLoading, gridSpotlightId]);

packages/app/src/hooks/useStartLetter.test.js

@@ -0,0 +1,105 @@
+import {renderHook, act} from '@testing-library/react';
+import Spotlight from '@enact/spotlight';
+
+import useStartLetter from './useStartLetter';
+
+jest.mock('@enact/spotlight', () => ({__esModule: true, default: {focus: jest.fn()}}));
+
+const item = (name) => ({SortName: name});
+const ALIENS = [item('Alien'), item('Aliens'), item('Blade Runner')];
+
+const setup = (props = {}) => renderHook(
+	({allItems, isLoading}) => useStartLetter({
+		allItems,
+		isLoading,
+		gridSpotlightId: 'library-grid'
+	}),
+	{initialProps: {allItems: ALIENS, isLoading: false, ...props}}
+);
+
+const pick = (result, letter) => act(() => {
+	result.current.handleLetterSelect({currentTarget: {dataset: {letter}}});
+});
+
+beforeEach(() => {
+	jest.useFakeTimers();
+	Spotlight.focus.mockClear();
+});
+
+afterEach(() => {
+	jest.runOnlyPendingTimers();
+	jest.useRealTimers();
+});
+
+describe('useStartLetter', () => {
+	test('narrows the list to the letter picked', () => {
+		const {result} = setup();
+
+		pick(result, 'A');
+
+		expect(result.current.items.map(i => i.SortName)).toEqual(['Alien', 'Aliens']);
+	});
+
+	test('picking the same letter again clears it', () => {
+		const {result} = setup();
+
+		pick(result, 'A');
+		pick(result, 'A');
+
+		expect(result.current.startLetter).toBeNull();
+		expect(result.current.items).toHaveLength(3);
+	});
+
+	test('hands focus to the grid once the narrowed list has settled', () => {
+		const {result} = setup();
+
+		pick(result, 'A');
+		act(() => jest.advanceTimersByTime(100));
+
+		expect(Spotlight.focus).toHaveBeenCalledWith('library-grid');
+	});
+
+	// The list also rebuilds for a filter or a search the viewer ran from a panel
+	// that is still open, and taking focus for those drags them out of it.
+	test('leaves focus alone when the list rebuilds under the same letter', () => {
+		const {result, rerender} = setup();
+
+		pick(result, 'A');
+		act(() => jest.advanceTimersByTime(100));
+		Spotlight.focus.mockClear();
+
+		// A filter applied from the panel: reload, then a different set back.
+		rerender({allItems: ALIENS, isLoading: true});
+		rerender({allItems: [item('Alien')], isLoading: false});
+		act(() => jest.advanceTimersByTime(200));
+
+		expect(Spotlight.focus).not.toHaveBeenCalled();
+	});
+
+	test('still moves focus when a different letter is picked', () => {
+		const {result} = setup();
+
+		pick(result, 'A');
+		act(() => jest.advanceTimersByTime(100));
+		Spotlight.focus.mockClear();
+
+		pick(result, 'B');
+		act(() => jest.advanceTimersByTime(100));
+
+		expect(Spotlight.focus).toHaveBeenCalledWith('library-grid');
+	});
+
+	test('a letter cleared and picked again counts as a fresh pick', () => {
+		const {result} = setup();
+
+		pick(result, 'A');
+		act(() => jest.advanceTimersByTime(100));
+		pick(result, 'A');
+		Spotlight.focus.mockClear();
+
+		pick(result, 'A');
+		act(() => jest.advanceTimersByTime(100));
+
+		expect(Spotlight.focus).toHaveBeenCalledWith('library-grid');
+	});
+});

packages/app/src/views/Library/Library.js

@@ -33,8 +33,19 @@ const SpottableDiv = Spottable('div');
 const SpottableButton = Spottable('button');
 const ToolbarContainer = SpotlightContainerDecorator({enterTo: 'last-focused', restrict: 'self-first'}, 'div');
 const GridContainer = SpotlightContainerDecorator({enterTo: 'last-focused', restrict: 'self-only'}, 'div');
-const SortPanelContainer = SpotlightContainerDecorator({enterTo: 'last-focused', restrict: 'self-only'}, 'div');
-const SettingsPanelContainer = SpotlightContainerDecorator({enterTo: 'last-focused', restrict: 'self-only'}, 'div');
+// A panel sits on top of the grid it was opened from, so a 5-way press at its
+// edge has to stay put rather than land on whatever is behind it. leaveFor is
+// what holds it: restrict never reaches the container, because
+// SpotlightContainerDecorator reads that from its spotlightRestrict prop and
+// not from this config, and the prop defaults to self-first.
+const PANEL_HOLDS_FOCUS = {
+	enterTo: 'last-focused',
+	restrict: 'self-only',
+	leaveFor: {left: '', right: '', up: '', down: ''}
+};
+
+const SortPanelContainer = SpotlightContainerDecorator(PANEL_HOLDS_FOCUS, 'div');
+const SettingsPanelContainer = SpotlightContainerDecorator(PANEL_HOLDS_FOCUS, 'div');
 
 // Every sort ends on SortName so items the server ranks equally keep a stable
 // order between pages, which a bare key leaves to whatever the database returns.
@@ -619,15 +630,6 @@ const Library = ({library, genreFilter, studioFilter, onSelectItem, onViewPhoto,
 		initialFocusDoneRef.current = false;
 	}, []);
 
-	useEffect(() => {
-		if (items.length > 0 && !isLoading && !initialFocusDoneRef.current) {
-			setTimeout(() => {
-				Spotlight.focus(groupedActive ? 'library-group-row-0' : 'library-grid');
-				initialFocusDoneRef.current = true;
-			}, 100);
-		}
-	}, [items.length, isLoading, groupedActive]);
-
 	const handleItemClick = useCallback((ev) => {
 		const itemIndex = ev.currentTarget?.dataset?.index;
 		if (itemIndex === undefined) return;
@@ -719,6 +721,21 @@ const Library = ({library, genreFilter, studioFilter, onSelectItem, onViewPhoto,
 		onBack: handleBackBeyondPanels,
 		enabled: !isMusicBrowseHome
 	});
+	// The grid takes focus once the library has something to show. The reload
+	// this waits on also runs for every filter picked, and that reload clears
+	// the flag below, so a panel left open would be handed the grid out from
+	// under the viewer the moment their pick came back. A panel on screen is
+	// the viewer still choosing, so the grid waits for them to finish with it.
+	useEffect(() => {
+		if (showSortPanel || showSettingsPanel) return;
+		if (items.length > 0 && !isLoading && !initialFocusDoneRef.current) {
+			setTimeout(() => {
+				Spotlight.focus(groupedActive ? 'library-group-row-0' : 'library-grid');
+				initialFocusDoneRef.current = true;
+			}, 100);
+		}
+	}, [items.length, isLoading, groupedActive, showSortPanel, showSettingsPanel]);
+
 
 	// Choosing the sort already in use turns it around rather than doing nothing.
 	const handleSortSelect = useCallback((ev) => {