The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Thanks for the PR!

This problem might be more difficult than I thought.

I was wondering why this block needs special focus handling because the MediaRplaceFlow component should automatically return focus to the proper element.

From what I've researched, different components are rendered depending on whether media is set or not. If media is not set, the MediaReplaceFlow component is rendered. If media is set, a Dropdown component is rendered and the MediaReplaceFlow component is rendered in a popover.

Although they look the same button at first glance, different components are rendered, so I think special focus handling is required.

Considering that, we may not be able to use the same approach as the site logo.

Is my understanding correct?

Although they look the same button at first glance, different components are rendered, so I think special focus handling is required.

Considering that, we may not be able to use the same approach as the site logo.

Yes, its using different buttons, So while the component should return focus but it cannot as original button might be unmounted.

We can probably refactor the component.

		{ shouldShowBackgroundImageControls ? (
				<BackgroundControlsPanel
					label={ title }
					filename={ title }
					url={ url }
					onToggle={ setIsDropDownOpen }
					hasImageValue={ hasImageValue }
				>
					<VStack spacing={ 3 } className="single-column">
						<BackgroundImageControls
							onChange={ onChange }
							style={ value }
							inheritedValue={ resolvedInheritedValue }
							displayInPanel
							onResetImage={ () => {
								setIsDropDownOpen( false );
								resetBackground();
							} }
							onRemoveImage={ () => setIsDropDownOpen( false ) }
							defaultValues={ defaultValues }
							containerRef={ containerRef }
						/>
						<BackgroundSizeControls
							onChange={ onChange }
							style={ value }
							defaultValues={ defaultValues }
							inheritedValue={ resolvedInheritedValue }
						/>
					</VStack>
				</BackgroundControlsPanel>
			) : (
				<BackgroundImageControls
					onChange={ onChange }
					style={ value }
					inheritedValue={ resolvedInheritedValue }
					defaultValues={ defaultValues }
					onResetImage={ () => {
						setIsDropDownOpen( false );
						resetBackground();
					} }
					onRemoveImage={ () => setIsDropDownOpen( false ) }
					containerRef={ containerRef }
				/>
			) }

I think following code might be causing it.

@t-hamano Should I go ahead with changing the structure a little bit so that focus returns automatically?

Should I go ahead with changing the structure a little bit so that focus returns automatically?

Yes, as long as there are no visual changes, I think refactoring to achieve our purpose is acceptable.

Should I go ahead with changing the structure a little bit so that focus returns automatically?

Yes, as long as there are no visual changes, I think refactoring to achieve our purpose is acceptable.

I tried to refactor the component to remove then "unmounting" of toggle button but it wont look the same as internally its using two different popovers.

or we can refactor "MediaReplaceFlow" to use Dropdown component as well.

Also,

This should be using dropdown component too to match other components where it opens on the side?

@Mayank-Tripathi32 Thanks for investigating!

Yes, as long as there are no visual changes, I think refactoring to achieve our purpose is acceptable.

I have tried to find an ideal approach, but I haven't found it yet 😅 For now, I can think of three approaches:

The first approach: Don't unmount the components, but visually hide the components that shouldn't be displayed with visibility: hidden. That way, we won't lose the ref and we should be able to focus on the button when the image is uploaded or reset.

The second approach: create a separate toggle button for the popover, which may not be acceptable since it introduces a visual change. Two examples:

The third approach: As already tried, use window.requestAnimationFrame. This seems like it might be unreliable for focus management, but as far as I've tested, this approach seems to work stably.

@Mamaduka @Mayank-Tripathi32

I think it might be a good idea to try the third approach, what do you think?

Agreed. Let's try the third approach for now.

Let's go ahead with this PR for now, and if the rAF approach is unstable, consider a different approach in follow-up.

Sorry, somehow I missed the latest pings. No further feedback on rAF, besides the initial comment.

I think this is a good hotfix. When I have time, I might look into refactoring the whole component, but it's not a priority at the moment.