Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make save panel a dialog with proper labels, fix site editor focus loss after save #59622

Merged
merged 15 commits into from
Mar 11, 2024
Merged
41 changes: 24 additions & 17 deletions packages/edit-site/src/components/save-panel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,13 @@ const EntitiesSavedStatesForPreview = ( { onClose } ) => {
);
};

const _EntitiesSavedStates = ( { onClose } ) => {
const _EntitiesSavedStates = ( { onClose, renderDialog = undefined } ) => {
if ( isPreviewingTheme() ) {
return <EntitiesSavedStatesForPreview onClose={ onClose } />;
}
return <EntitiesSavedStates close={ onClose } />;
return (
<EntitiesSavedStates close={ onClose } renderDialog={ renderDialog } />
);
};

export default function SavePanel() {
Expand Down Expand Up @@ -142,21 +144,26 @@ export default function SavePanel() {
} ) }
ariaLabel={ __( 'Save panel' ) }
>
{ isSaveViewOpen ? (
<_EntitiesSavedStates onClose={ onClose } />
) : (
<div className="edit-site-editor__toggle-save-panel">
<Button
variant="secondary"
className="edit-site-editor__toggle-save-panel-button"
onClick={ () => setIsSaveViewOpened( true ) }
aria-expanded={ false }
disabled={ disabled }
__experimentalIsFocusable
>
{ __( 'Open save panel' ) }
</Button>
</div>
<div
className={ classnames( 'edit-site-editor__toggle-save-panel', {
'screen-reader-text': isSaveViewOpen,
} ) }
>
<Button
variant="secondary"
className={ classnames(
'edit-site-editor__toggle-save-panel-button'
) }
onClick={ () => setIsSaveViewOpened( true ) }
aria-haspopup={ 'dialog' }
disabled={ disabled }
__experimentalIsFocusable
>
{ __( 'Open save panel' ) }
</Button>
</div>
{ isSaveViewOpen && (
<_EntitiesSavedStates onClose={ onClose } renderDialog />
) }
</NavigableRegion>
);
Expand Down
41 changes: 33 additions & 8 deletions packages/editor/src/components/entities-saved-states/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import {
} from '@wordpress/element';
import { store as coreStore } from '@wordpress/core-data';
import { store as blockEditorStore } from '@wordpress/block-editor';
import { __experimentalUseDialog as useDialog } from '@wordpress/compose';
import {
__experimentalUseDialog as useDialog,
useInstanceId,
} from '@wordpress/compose';
import { store as noticesStore } from '@wordpress/notices';

/**
Expand All @@ -31,10 +34,17 @@ function identity( values ) {
return values;
}

export default function EntitiesSavedStates( { close } ) {
export default function EntitiesSavedStates( {
close,
renderDialog = undefined,
} ) {
const isDirtyProps = useIsDirty();
return (
<EntitiesSavedStatesExtensible close={ close } { ...isDirtyProps } />
<EntitiesSavedStatesExtensible
close={ close }
renderDialog={ renderDialog }
{ ...isDirtyProps }
/>
);
}

Expand All @@ -44,6 +54,7 @@ export function EntitiesSavedStatesExtensible( {
onSave = identity,
saveEnabled: saveEnabledProp = undefined,
saveLabel = __( 'Save' ),
renderDialog = undefined,

dirtyEntityRecords,
isDirty,
Expand Down Expand Up @@ -183,12 +194,20 @@ export function EntitiesSavedStatesExtensible( {
const [ saveDialogRef, saveDialogProps ] = useDialog( {
onClose: () => dismissPanel(),
} );
const dialogLabel = useInstanceId( EntitiesSavedStatesExtensible, 'label' );
const dialogDescription = useInstanceId(
EntitiesSavedStatesExtensible,
'description'
);

return (
<div
ref={ saveDialogRef }
{ ...saveDialogProps }
className="entities-saved-states__panel"
role={ renderDialog ? 'dialog' : undefined }
aria-labelledby={ renderDialog ? dialogLabel : undefined }
aria-describedby={ renderDialog ? dialogDescription : undefined }
>
<Flex className="entities-saved-states__panel-header" gap={ 2 }>
<FlexItem
Expand All @@ -197,6 +216,7 @@ export function EntitiesSavedStatesExtensible( {
ref={ saveButtonRef }
variant="primary"
disabled={ ! saveEnabled }
__experimentalIsFocusable
onClick={ saveCheckedEntities }
className="editor-entities-saved-states__save-button"
>
Expand All @@ -213,11 +233,16 @@ export function EntitiesSavedStatesExtensible( {
</Flex>

<div className="entities-saved-states__text-prompt">
<strong className="entities-saved-states__text-prompt--header">
{ __( 'Are you ready to save?' ) }
</strong>
{ additionalPrompt }
<p>
<div
className="entities-saved-states__text-prompt--header-wrapper"
id={ renderDialog ? dialogLabel : undefined }
>
<strong className="entities-saved-states__text-prompt--header">
{ __( 'Are you ready to save?' ) }
</strong>
{ additionalPrompt }
</div>
<p id={ renderDialog ? dialogDescription : undefined }>
{ isDirty
? createInterpolateElement(
sprintf(
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/editor/various/pattern-overrides.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ test.describe( 'Pattern Overrides', () => {
.click();
await page
.getByRole( 'region', { name: 'Save panel' } )
.getByRole( 'button', { name: 'Save' } )
.getByRole( 'button', { name: 'Save', exact: true } )
.click();

await expect(
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/specs/site-editor/multi-entity-saving.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ test.describe( 'Site Editor - Multi-entity save flow', () => {
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'button', { name: 'Saved' } )
).toBeDisabled();

// Check focus returns to Saved button.
await expect(
page
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'button', { name: 'Saved' } )
).toBeFocused();
} );

test( 'save flow should allow re-saving after changing the same block attribute', async ( {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/site-editor/patterns.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ test.describe( 'Patterns', () => {
.click();
await page
.getByRole( 'region', { name: 'Save panel' } )
.getByRole( 'button', { name: 'Save' } )
.getByRole( 'button', { name: 'Save', exact: true } )
.click();
await expect(
page.getByRole( 'button', { name: 'Dismiss this notice' } )
Expand Down