Background Image Panel: fix focus loss #69813
Conversation
…d focus handling - Added containerRef to BackgroundImageControls and BackgroundImagePanel for better focus management. - Updated closeAndFocus function to utilize requestAnimationFrame for ensuring DOM updates are complete before focusing the toggle button. - Removed unused replaceContainerRef as we couldn't focus due to unmounted state.
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
I think there are a couple of other tricks we might use, which, in theory, should reduce the complexity of this component.
- Refactor/re-arrange components so that the Media modal returns focus "naturally." It is usually my preferred method. Example: Site Logo: Prevent focus loss when updating media from the sidebar #68621.
- Use flag and ref callback. Example: Editor: Update focus return handler for the Featured Image #67236.
| // Use requestAnimationFrame to ensure DOM updates are complete | ||
| window.requestAnimationFrame( () => { |
There was a problem hiding this comment.
In my experience, rAFs or setTimeout tasks aren't entirely reliable for focus management.
There was a problem hiding this comment.
I will check out the other implementations in that case, should we also look into updating existing uses of rAFs in that case?
|
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 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? |
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? |
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. Also, This should be using dropdown component too to match other components where it opens on the side? |
|
@Mayank-Tripathi32 Thanks for investigating!
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 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 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. |
What?
Closes #69745
Fixes focus management in the Background Image Control component by improving ref handling and focus timing.
Why?
The focus was not being properly maintained after image operations in the Background Image Control because:
How?
containerRefattachment to the parent container div inBackgroundImagePanelcloseAndFocusfunction to userequestAnimationFramefor reliable timingcloseAndFocushandler call toonSelectMediaflow to ensure that focus returns correctly.The architectural cha 8000 nge of moving the ref to the parent container provides a more reliable way to find focusable elements within the component's DOM structure.
Testing Instructions (keyboard)
The focus should consistently return to the toggle button after any image operation (select/reset/remove) and the dropdown should close properly.
ScreenCast
https://q7utzrengv.ufs.sh/f/Wgl9eBAmTj29aD9Oh8fFRjF31d07GtqsWNpK4IXH6Z9OnbyQ