Open t-hamano opened 1 week ago
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
@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:
https://github.com/user-attachments/assets/dbe29b8c-f0ca-4d2b-8c5c-be231d878fee
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.
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
Description
Shadow panel
Font size preset panel
After investigating the problem using
git bisect
, I found that the problem was caused by #64777.Screenshots, screen recording, code snippet
Shadow panel
https://github.com/user-attachments/assets/8d2bc1db-00db-4750-b968-dd132d3c136e
Font size preset panel
https://github.com/user-attachments/assets/5d3a708a-6133-4ea8-bc53-ced9020cac0b
Environment info
Gutenberg version: 19.3.0 (faa6968936)