Shadow/Font size preset panel crashes the editor

[t-hamano]

Description

Shadow panel

• Open the site editor > global styles > Shadows > Choose one of the presets > click the Back button
• The editor crashes

Uncaught TypeError: Cannot read properties of undefined (reading 'shadow') at shadows-edit-panel.js:165:31

Font size preset panel

• Open the site editor > global styles > Typography > Font size presets > Choose one of the presets > click the Back button
• The editor crashes

Uncaught TypeError: Cannot read properties of undefined (reading 'fluid') at FontSize (font-size.js:57:12)

After investigating the problem using git bisect, I found that the problem was caused by #64777.

Screenshots, screen recording, code snippet

Shadow panel

a296d467e9b8a647388068a2c049bb0f.mp4

Font size preset panel

font-size.mp4

Environment info

Gutenberg version: 19.3.0 (faa6968)

[ciampo]

After investigating the problem using git bisect, I found that the problem was caused by #64777.

Looking at the symptoms, this feels like a bug in the panel itself that was somehow exposed by #64777. It could possibly be caused by the fact that prior to that PR, navigating from A to B would immediately remove A's contents from the react tree.

After #64777, A is kept around for a bit more while its exit animation plays out. So, probably there is some logic in that screen that crashes while the screen animates out

[t-hamano]

@ciampo Thanks for the feedback!

The cause of this problem is likely that in the component where the crash occurs, some logic relies on URL parameters, namely useNavigator.

For example, in the Shadows panel, category and slug are taken from URL parameters here.

However, when we return from this panel, the URL parameters have already been changed to new ones, so all parameters are undefined. The panel crashes because parameters are not expected to be undefined.

One solution is to perform an early return when a parameter required by the logic is undefined, as shown below:

diff --git a/packages/edit-site/src/components/global-styles/shadows-edit-panel.js b/packages/edit-site/src/components/global-styles/shadows-edit-panel.js
index ec1dd1a900..8bd4810a80 100644
--- a/packages/edit-site/src/components/global-styles/shadows-edit-panel.js
+++ b/packages/edit-site/src/components/global-styles/shadows-edit-panel.js
@@ -96,6 +96,10 @@ export default function ShadowsEditPanel() {
        const [ isRenameModalVisible, setIsRenameModalVisible ] = useState( false );
        const [ shadowName, setShadowName ] = useState( selectedShadow.name );
 
+       if ( ! category || ! slug ) {
+               return null;
+       }
+
        const onShadowChange = ( shadow ) => {
                setSelectedShadow( { ...selectedShadow, shadow } );
                const updatedShadows = shadows.map( ( s ) =>

However, in this case, a small flash will occur:

4241c5765d81466721a22fe5c4f88648.mp4

I think the ideal solution would be to not include parameters in the path prop, what do you think?

For example, in the Blocks screen, we dynamically create navigation screens here for each path.

[ciampo]

One solution is to perform an early return when a parameter required by the logic is undefined, as shown below:
...
However, in this case, a small flash will occur:

I think we should apply this fix immediately to prevent the app from crashing, and then iterated on a more refined fix.

I think the ideal solution would be to not include parameters in the path prop, what do you think?

In my mind, the ideal solution would be for Navigator to "freeze" those parameters, but it would require some non-trivial changes to the component.

Maybe we can add a similar logic specifically for the affected screens, where we basically ignore any updates to the params when the location retrieved through useNavigator doesn't match the screen's path — not super elegant, but it would probably avoid the bug without causing any flashes.

Alternatively, as you suggest, refactoring to harcoded paths (ie. without parameters) would also work by avoiding the issue altogether — although I feel like we should find a way to solve this issue, since it will benefit all consumers of Navigator