Global styles: incorrect usage of Navigator.BackButton

[ciampo]

While working on #65790, I noticed that the ScreenHeader has a onBack prop that gets called when Navigator.BackButton gets clicked:

gutenberg/packages/edit-site/src/components/global-styles/header.js

Lines 21 to 26 in 8e4aac8

	<Navigator.BackButton
	icon={ isRTL() ? chevronRight : chevronLeft }
	size="small"
	label={ __( 'Back' ) }
	onClick={ onBack }
	/>

Navigator.BackButton already performs a backward navigation (to the "parent" screen) — but I noticed usages where consumers of ScreenHeader use the onBack callback to trigger another navigation. This can cause unintended consequences, since it may trigger Navigator to make two consecutive navigations.

I could spot two usages:

Font sizes screen

gutenberg/packages/edit-site/src/components/global-styles/font-sizes/font-size.js

Line 154 in 8e4aac8

onBack={ () => goTo( '/typography/font-sizes/' ) }

Calling goTo seems totally unnecessary here — if we just let Navigator.BackButton do its job, I believe it would take us to the same screen. Also, using goTo without the isBack option causes the transition to move in the wrong direction.

Revisions screen

gutenberg/packages/edit-site/src/components/global-styles/screen-revisions/index.js

Line 143 in 8e4aac8

onBack={ onCloseRevisions }

Here I may need @ramonjd and @andrewserong 's help to understand why we need to navigate to / when clicking back.

Could we change the code so that Navigator.BackButton's original behaviour is preserved?

Alternatively, we'll need to see if we can preventDefault, or consider rendering a Button (instead of Navigator.BackButton) in ScreenHeader, and make the onBack callback an alternative to the "default" going back behaviour, instead of an addition.

[ramonjd]

understand why we need to navigate to / when clicking back.

Thanks for the ping. I think it was because:

1. There's no intermediate path, so root seemed like the best option
2. I didn't know about Navigator.BackButton!! TIL

Could we change the code so that Navigator.BackButton's original behaviour is preserved?

I'll test

[ramonjd]

This one is for the global styles: #65810

I looked briefly into the font sizes panel, but it appears there's an existing bug that's preventing testing - the origin and slug are missing from navigation route.

[ramonjd]

I looked briefly into the font sizes panel, but it appears there's an existing bug that's preventing testing - the origin and slug are missing from navigation route.

Fix incoming

[ramonjd]

onBack={ () => goTo( '/typography/font-sizes/' ) }

I think the font sizes component needs this. Otherwise, it'll skip the intermediary font sizes list:

2024-10-02.11.01.17.mp4

So it might need a button refactor as you suggest. I'll just add the fix for the bug I saw for now.

[ciampo]

I looked briefly into the font sizes panel, but it appears there's an existing bug that's preventing testing - the origin and slug are missing from navigation route.

Fix incoming

There are already two PRs with potential fixes:

• Fix: Shadow/Font size preset panel crashes the editor #65765
• Global Styles: fix screens crashing on exit animation #65790

I admittedly forgot to give y'all context about this bug, my bad

There is also another PR that is fixing a separate issue #65791

Probably better to wait for the dust to settle on these urgent bug fixes, and then look at the code again

[ciampo]

So it might need a button refactor as you suggest. I'll just add the fix for the bug I saw for now.

Could we instead tweak the intermediate screen's path, so that it follows a "hierarchic" URL scheme? That should cause the default behavior of Navigator.BackButton to work as expected

[ciampo]

With #65942 and #65946 merged, I think we can close this issue too.

Thank you all for the help!