Skip to content

Commit

Permalink
useToolsPanel: calculate menuItems in layout effect to avoid painting…
Browse files Browse the repository at this point in the history 
… intermediate state (#65494)

* useToolsPanel: calculate menuItems in layout effect to avoid painting intermediate state

* useToolsPanel: remove setState deps, calculate derived values in layout effects

* ToolsPanelItem: also use layout effect to prevent loops

* Changelog entry

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
  • Loading branch information
@jsnajdr @ciampo @gutenbergplugin
3 people authored and gutenbergplugin committed Sep 20, 2024
1 parent 8488c64 commit 2cce429
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 80 deletions.
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- `ToolsPanel`: avoid paining intermediate unfinished states ([#65494](https://github.com/WordPress/gutenberg/pull/65494)).

## 28.8.0 (2024-09-19)

### Bug Fixes
Expand Down
13 changes: 4 additions & 9 deletions packages/components/src/tools-panel/tools-panel-item/hook.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,7 @@
* WordPress dependencies
*/
import { usePrevious } from '@wordpress/compose';
import {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
} from '@wordpress/element';
import { useCallback, useLayoutEffect, useMemo } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -101,7 +96,7 @@ export function useToolsPanelItem(
deregisterPanelItem,
] );

useEffect( () => {
useLayoutEffect( () => {
if ( hasMatchingPanel ) {
registerResetAllFilter( resetAllFilterCallback );
}
Expand All @@ -127,7 +122,7 @@ export function useToolsPanelItem(
const isValueSet = hasValue();
// Notify the panel when an item's value has changed except for optional
// items without value because the item should not cause itself to hide.
useEffect( () => {
useLayoutEffect( () => {
if ( ! isShownByDefault && ! isValueSet ) {
return;
}
Expand All @@ -143,7 +138,7 @@ export function useToolsPanelItem(

// Determine if the panel item's corresponding menu is being toggled and
// trigger appropriate callback if it is.
useEffect( () => {
useLayoutEffect( () => {
// We check whether this item is currently registered as items rendered
// via fills can persist through the parent panel being remounted.
// See: https://github.com/WordPress/gutenberg/pull/45673
Expand Down
131 changes: 60 additions & 71 deletions packages/components/src/tools-panel/tools-panel/hook.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
Expand Down Expand Up @@ -101,7 +101,7 @@ export function useToolsPanel(
// the resetAll task. Without this, the flag is cleared after the first
// control updates and forces a rerender with subsequent controls then
// believing they need to reset, unfortunately using stale data.
useEffect( () => {
useLayoutEffect( () => {
if ( wasResetting ) {
isResettingRef.current = false;
}
Expand All @@ -114,76 +114,66 @@ export function useToolsPanel(
ResetAllFilter[]
>( [] );

const registerPanelItem = useCallback(
( item: ToolsPanelItem ) => {
// Add item to panel items.
setPanelItems( ( items ) => {
const newItems = [ ...items ];
// If an item with this label has already been registered, remove it
// first. This can happen when an item is moved between the default
// and optional groups.
const existingIndex = newItems.findIndex(
( oldItem ) => oldItem.label === item.label
);
if ( existingIndex !== -1 ) {
newItems.splice( existingIndex, 1 );
}
return [ ...newItems, item ];
} );
const registerPanelItem = useCallback( ( item: ToolsPanelItem ) => {
// Add item to panel items.
setPanelItems( ( items ) => {
const newItems = [ ...items ];
// If an item with this label has already been registered, remove it
// first. This can happen when an item is moved between the default
// and optional groups.
const existingIndex = newItems.findIndex(
( oldItem ) => oldItem.label === item.label
);
if ( existingIndex !== -1 ) {
newItems.splice( existingIndex, 1 );
}
return [ ...newItems, item ];
} );

// Track the initial order of item registration. This is used for
// maintaining menu item order later.
setMenuItemOrder( ( items ) => {
if ( items.includes( item.label ) ) {
return items;
}
// Track the initial order of item registration. This is used for
// maintaining menu item order later.
setMenuItemOrder( ( items ) => {
if ( items.includes( item.label ) ) {
return items;
}

return [ ...items, item.label ];
} );
},
[ setPanelItems, setMenuItemOrder ]
);
return [ ...items, item.label ];
} );
}, [] );

// Panels need to deregister on unmount to avoid orphans in menu state.
// This is an issue when panel items are being injected via SlotFills.
const deregisterPanelItem = useCallback(
( label: string ) => {
// When switching selections between components injecting matching
// controls, e.g. both panels have a "padding" control, the
// deregistration of the first panel doesn't occur until after the
// registration of the next.
setPanelItems( ( items ) => {
const newItems = [ ...items ];
const index = newItems.findIndex(
( item ) => item.label === label
);
if ( index !== -1 ) {
newItems.splice( index, 1 );
}
return newItems;
} );
},
[ setPanelItems ]
);
const deregisterPanelItem = useCallback( ( label: string ) => {
// When switching selections between components injecting matching
// controls, e.g. both panels have a "padding" control, the
// deregistration of the first panel doesn't occur until after the
// registration of the next.
setPanelItems( ( items ) => {
const newItems = [ ...items ];
const index = newItems.findIndex(
( item ) => item.label === label
);
if ( index !== -1 ) {
newItems.splice( index, 1 );
}
return newItems;
} );
}, [] );

const registerResetAllFilter = useCallback(
( newFilter: ResetAllFilter ) => {
setResetAllFilters( ( filters ) => {
return [ ...filters, newFilter ];
} );
setResetAllFilters( ( filters ) => [ ...filters, newFilter ] );
},
[ setResetAllFilters ]
[]
);

const deregisterResetAllFilter = useCallback(
( filterToRemove: ResetAllFilter ) => {
setResetAllFilters( ( filters ) => {
return filters.filter(
( filter ) => filter !== filterToRemove
);
} );
setResetAllFilters( ( filters ) =>
filters.filter( ( filter ) => filter !== filterToRemove )
);
},
[ setResetAllFilters ]
[]
);

// Manage and share display state of menu items representing child controls.
Expand All @@ -193,17 +183,16 @@ export function useToolsPanel(
} );

// Setup menuItems state as panel items register themselves.
useEffect( () => {
setMenuItems( ( prevState ) => {
const items = generateMenuItems( {
useLayoutEffect( () => {
setMenuItems( ( currentMenuItems ) =>
generateMenuItems( {
panelItems,
shouldReset: false,
currentMenuItems: prevState,
currentMenuItems,
menuItemOrder,
} );
return items;
} );
}, [ panelItems, setMenuItems, menuItemOrder ] );
} )
);
}, [ panelItems, menuItemOrder ] );

// Updates the status of the panel’s menu items. For default items the
// value represents whether it differs from the default and for optional
Expand All @@ -225,7 +214,7 @@ export function useToolsPanel(
return newState;
} );
},
[ setMenuItems ]
[]
);

// Whether all optional menu items are hidden or not must be tracked
Expand All @@ -235,7 +224,7 @@ export function useToolsPanel(
const [ areAllOptionalControlsHidden, setAreAllOptionalControlsHidden ] =
useState( false );

useEffect( () => {
useLayoutEffect( () => {
if (
isMenuItemTypeEmpty( menuItems?.default ) &&
! isMenuItemTypeEmpty( menuItems?.optional )
Expand All @@ -245,7 +234,7 @@ export function useToolsPanel(
).some( ( [ , isSelected ] ) => isSelected );
setAreAllOptionalControlsHidden( allControlsHidden );
}
}, [ menuItems, setAreAllOptionalControlsHidden ] );
}, [ menuItems ] );

const cx = useCx();
const classes = useMemo( () => {
Expand Down Expand Up @@ -297,7 +286,7 @@ export function useToolsPanel(

setMenuItems( newMenuItems );
},
[ menuItems, panelItems, setMenuItems ]
[ menuItems, panelItems ]
);

// Resets display of children and executes resetAll callback if available.
Expand All @@ -314,7 +303,7 @@ export function useToolsPanel(
shouldReset: true,
} );
setMenuItems( resetMenuItems );
}, [ panelItems, resetAllFilters, resetAll, setMenuItems, menuItemOrder ] );
}, [ panelItems, resetAllFilters, resetAll, menuItemOrder ] );

// Assist ItemGroup styling when there are potentially hidden placeholder
// items by identifying first & last items that are toggled on for display.
Expand Down

1 comment on commit 2cce429

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky tests detected in 2cce429.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10959666641
📝 Reported issues:

Please sign in to comment.