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
38 changes: 21 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,23 @@ 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="edit-site-editor__toggle-save-panel">
<Button
variant="secondary"
className={ classnames(
'edit-site-editor__toggle-save-panel-button',
{ 'screen-reader-text': isSaveViewOpen }
) }
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
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 @@
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'button', { name: 'Saved' } )
).toBeDisabled();

// Check focus returns to Open save panel button.
await expect(
page
.getByRole( 'region', { name: 'Save panel' } )
.getByRole( 'button', { name: 'Open save panel' } )
).toBeFocused();

Check failure on line 60 in test/e2e/specs/site-editor/multi-entity-saving.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 5

[chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected

1) [chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected Error: Timed out 5000ms waiting for expect(locator).toBeFocused() Locator: getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) Expected: focused Received: inactive Call log: - expect.toBeFocused with timeout 5000ms - waiting for getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" 58 | .getByRole( 'region', { name: 'Save panel' } ) 59 | .getByRole( 'button', { name: 'Open save panel' } ) > 60 | ).toBeFocused(); | ^ 61 | } ); 62 | 63 | test( 'save flow should allow re-saving after changing the same block attribute', async ( { at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/site-editor/multi-entity-saving.spec.js:60:5

Check failure on line 60 in test/e2e/specs/site-editor/multi-entity-saving.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 5

[chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected

1) [chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Timed out 5000ms waiting for expect(locator).toBeFocused() Locator: getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) Expected: focused Received: inactive Call log: - expect.toBeFocused with timeout 5000ms - waiting for getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" 58 | .getByRole( 'region', { name: 'Save panel' } ) 59 | .getByRole( 'button', { name: 'Open save panel' } ) > 60 | ).toBeFocused(); | ^ 61 | } ); 62 | 63 | test( 'save flow should allow re-saving after changing the same block attribute', async ( { at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/site-editor/multi-entity-saving.spec.js:60:5

Check failure on line 60 in test/e2e/specs/site-editor/multi-entity-saving.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 5

[chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected

1) [chromium] › site-editor/multi-entity-saving.spec.js:29:2 › Site Editor - Multi-entity save flow › save flow should work as expected Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: Timed out 5000ms waiting for expect(locator).toBeFocused() Locator: getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) Expected: focused Received: inactive Call log: - expect.toBeFocused with timeout 5000ms - waiting for getByRole('region', { name: 'Save panel' }).getByRole('button', { name: 'Open save panel' }) - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" - locator resolved to <button type="button" aria-disabled="true" aria-haspopup…>Open save panel</button> - unexpected value "not focused" 58 | .getByRole( 'region', { name: 'Save panel' } ) 59 | .getByRole( 'button', { name: 'Open save panel' } ) > 60 | ).toBeFocused(); | ^ 61 | } ); 62 | 63 | test( 'save flow should allow re-saving after changing the same block attribute', async ( { at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/site-editor/multi-entity-saving.spec.js:60:5
} );

test( 'save flow should allow re-saving after changing the same block attribute', async ( {
Expand Down