From 22d1a049c5c4d4ce97311207a514c7ad43a81268 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 14:16:17 +0100 Subject: [PATCH 01/12] Reflect the actual initial state of the ComplementaryArea --- .../interface/src/components/complementary-area/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index bac359b10326ad..8193530493b16e 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -102,8 +102,12 @@ function ComplementaryArea( { const { getActiveComplementaryArea, isItemPinned } = select( interfaceStore ); const _activeArea = getActiveComplementaryArea( scope ); + return { - isActive: _activeArea === identifier, + isActive: + _activeArea === undefined + ? undefined + : _activeArea === identifier, isPinned: isItemPinned( scope, identifier ), activeArea: _activeArea, isSmall: select( viewportStore ).isViewportMatch( '< medium' ), @@ -144,6 +148,7 @@ function ComplementaryArea( { isActive && ( ! showIconLabels || isLarge ) } aria-expanded={ isActive } + disabled={ isActive === undefined } label={ title } icon={ showIconLabels ? check : icon } showTooltip={ ! showIconLabels } From 4419a760b4dbfe4aea2451989baeaf95f1b76d50 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 14:18:00 +0100 Subject: [PATCH 02/12] Update openDocumentSettingsSidebar to wait until button is enabled --- .../editor/open-document-settings-sidebar.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts index 0cb27c33c6c882..74ec42738cef4a 100644 --- a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts +++ b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts @@ -5,14 +5,18 @@ import type { Editor } from './index'; import { expect } from '../test'; /** - * Clicks on the button in the header which opens Document Settings sidebar when it is closed. + * Clicks on the button in the header which opens Document Settings sidebar when + * it is closed. * * @param {Editor} this */ export async function openDocumentSettingsSidebar( this: Editor ) { - const editorSettingsButton = this.page.locator( - 'role=region[name="Editor top bar"i] >> role=button[name="Settings"i]' - ); + const editorSettingsButton = this.page + .getByRole( 'region', { name: 'Editor top bar' } ) + .getByRole( 'button', { + name: 'Settings', + disabled: false, + } ); const isEditorSettingsOpened = ( await editorSettingsButton.getAttribute( 'aria-expanded' ) ) === @@ -22,9 +26,9 @@ export async function openDocumentSettingsSidebar( this: Editor ) { await editorSettingsButton.click(); await expect( - this.page.locator( - 'role=region[name="Editor settings"i] >> role=button[name^="Close settings"i]' - ) + this.page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Close settings' } ) ).toBeVisible(); } } From 08f4d5b728a63c841aaf589f1f039f53a519660a Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 15:26:39 +0100 Subject: [PATCH 03/12] Use `aria-disabled` instead of `disabled` --- packages/interface/src/components/complementary-area/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index 8193530493b16e..9b3c6f166bd399 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -148,7 +148,7 @@ function ComplementaryArea( { isActive && ( ! showIconLabels || isLarge ) } aria-expanded={ isActive } - disabled={ isActive === undefined } + aria-disabled={ isActive === undefined } label={ title } icon={ showIconLabels ? check : icon } showTooltip={ ! showIconLabels } From bf733c33670f876b95e3371ccdde8af5cb7238a9 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 15:57:04 +0100 Subject: [PATCH 04/12] Update also on Puppeteer's side --- .../src/open-document-settings-sidebar.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/e2e-test-utils/src/open-document-settings-sidebar.js b/packages/e2e-test-utils/src/open-document-settings-sidebar.js index 2ab5f773c3715f..8d50c8c74b996a 100644 --- a/packages/e2e-test-utils/src/open-document-settings-sidebar.js +++ b/packages/e2e-test-utils/src/open-document-settings-sidebar.js @@ -2,12 +2,17 @@ * Clicks on the button in the header which opens Document Settings sidebar when it is closed. */ export async function openDocumentSettingsSidebar() { - const openButton = await page.$( - '.edit-post-header__settings button[aria-label="Settings"][aria-expanded="false"]' + const toggleButton = await page.waitForSelector( + '.edit-post-header__settings button[aria-label="Settings"][aria-disabled="false"]' ); - if ( openButton ) { - await openButton.click(); + const isClosed = await page.evaluate( + ( element ) => element.getAttribute( 'aria-expanded' ) === 'false', + toggleButton + ); + + if ( isClosed ) { + await toggleButton.click(); await page.waitForSelector( '.edit-post-sidebar' ); } } From af698f8c8408b7193d4bc7de999d073f0b035abe Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 17:46:34 +0100 Subject: [PATCH 05/12] Make naming consistent --- .../src/editor/open-document-settings-sidebar.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts index 74ec42738cef4a..a24075e3f3f3fa 100644 --- a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts +++ b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts @@ -11,19 +11,18 @@ import { expect } from '../test'; * @param {Editor} this */ export async function openDocumentSettingsSidebar( this: Editor ) { - const editorSettingsButton = this.page + const toggleButton = this.page .getByRole( 'region', { name: 'Editor top bar' } ) .getByRole( 'button', { name: 'Settings', disabled: false, } ); - const isEditorSettingsOpened = - ( await editorSettingsButton.getAttribute( 'aria-expanded' ) ) === - 'true'; + const isClosed = + ( await toggleButton.getAttribute( 'aria-expanded' ) ) === 'false'; - if ( ! isEditorSettingsOpened ) { - await editorSettingsButton.click(); + if ( isClosed ) { + await toggleButton.click(); await expect( this.page From 895c9c8aa27b39c799bed8965a562d4c0819089a Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 27 Jan 2023 17:48:43 +0100 Subject: [PATCH 06/12] Fix the initial `isActive` state in mobile viewport The panel is always initially closed in mobile viewport --- .../interface/src/components/complementary-area/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index 9b3c6f166bd399..e577ab8ede9382 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -102,15 +102,17 @@ function ComplementaryArea( { const { getActiveComplementaryArea, isItemPinned } = select( interfaceStore ); const _activeArea = getActiveComplementaryArea( scope ); + const _isSmall = + select( viewportStore ).isViewportMatch( '< medium' ); return { isActive: - _activeArea === undefined + _activeArea === undefined && ! _isSmall ? undefined : _activeArea === identifier, isPinned: isItemPinned( scope, identifier ), activeArea: _activeArea, - isSmall: select( viewportStore ).isViewportMatch( '< medium' ), + isSmall: _isSmall, isLarge: select( viewportStore ).isViewportMatch( 'large' ), }; }, From 1282d7d21f69bb5b3ebccae2b1c13417cf20a83e Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 2 Feb 2023 15:44:31 +0100 Subject: [PATCH 07/12] Use `waitFor` instead of `expect` This makes more sense contextually. `expect` should be reserved for the test subject. --- .../src/editor/open-document-settings-sidebar.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts index a24075e3f3f3fa..bb8c23b16348df 100644 --- a/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts +++ b/packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts @@ -2,7 +2,6 @@ * Internal dependencies */ import type { Editor } from './index'; -import { expect } from '../test'; /** * Clicks on the button in the header which opens Document Settings sidebar when @@ -23,11 +22,9 @@ export async function openDocumentSettingsSidebar( this: Editor ) { if ( isClosed ) { await toggleButton.click(); - - await expect( - this.page - .getByRole( 'region', { name: 'Editor settings' } ) - .getByRole( 'button', { name: 'Close settings' } ) - ).toBeVisible(); + await this.page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Close settings' } ) + .waitFor(); } } From cde5e00100ad5cacb78df6d808245ee03c5e9d5a Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 2 Feb 2023 15:46:06 +0100 Subject: [PATCH 08/12] Rename "Close settings sidebar" to "Close settings" This is consistent with the current settings sidebar name in the editor. --- packages/edit-site/src/components/sidebar-edit-mode/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/edit-site/src/components/sidebar-edit-mode/index.js b/packages/edit-site/src/components/sidebar-edit-mode/index.js index cba7bae649fe47..48574b783f5a2c 100644 --- a/packages/edit-site/src/components/sidebar-edit-mode/index.js +++ b/packages/edit-site/src/components/sidebar-edit-mode/index.js @@ -69,7 +69,7 @@ export function SidebarComplementaryAreaFills() { identifier={ sidebarName } title={ __( 'Settings' ) } icon={ isRTL() ? drawerLeft : drawerRight } - closeLabel={ __( 'Close settings sidebar' ) } + closeLabel={ __( 'Close settings' ) } header={ } headerClassName="edit-site-sidebar-edit-mode__panel-tabs" > From ed67d4d9b978f2ab9eab0a72ee4b1b9fefe25077 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 2 Feb 2023 15:50:04 +0100 Subject: [PATCH 09/12] Check if the area is being activated For a short time, if the `isComplementaryAreaVisible` preference is set to true, the `activeArea` value is `undefined`. Let's disable the area toggle button until the area is activated properly. This will help with E2E stability because the framework will wait until the button is enabled, and can also help with a11y as it will correctly indicate the initial state of the toggle. --- .../components/complementary-area/index.js | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index e577ab8ede9382..77b6268632cc85 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -12,6 +12,7 @@ import { __ } from '@wordpress/i18n'; import { check, starEmpty, starFilled } from '@wordpress/icons'; import { useEffect, useRef } from '@wordpress/element'; import { store as viewportStore } from '@wordpress/viewport'; +import { store as preferencesStore } from '@wordpress/preferences'; /** * Internal dependencies @@ -97,27 +98,34 @@ function ComplementaryArea( { isActiveByDefault, showIconLabels = false, } ) { - const { isActive, isPinned, activeArea, isSmall, isLarge } = useSelect( - ( select ) => { - const { getActiveComplementaryArea, isItemPinned } = - select( interfaceStore ); - const _activeArea = getActiveComplementaryArea( scope ); - const _isSmall = - select( viewportStore ).isViewportMatch( '< medium' ); + const { isLoading, isActive, isPinned, activeArea, isSmall, isLarge } = + useSelect( + ( select ) => { + const { getActiveComplementaryArea, isItemPinned } = + select( interfaceStore ); - return { - isActive: - _activeArea === undefined && ! _isSmall - ? undefined - : _activeArea === identifier, - isPinned: isItemPinned( scope, identifier ), - activeArea: _activeArea, - isSmall: _isSmall, - isLarge: select( viewportStore ).isViewportMatch( 'large' ), - }; - }, - [ identifier, scope ] - ); + const isComplementaryAreaVisible = select( + preferencesStore + ).get( scope, 'isComplementaryAreaVisible' ); + const _activeArea = getActiveComplementaryArea( scope ); + const _isSmall = + select( viewportStore ).isViewportMatch( '< medium' ); + + return { + isLoading: + isComplementaryAreaVisible && + _activeArea === undefined && + ! _isSmall, + isActive: _activeArea === identifier, + isPinned: isItemPinned( scope, identifier ), + activeArea: _activeArea, + isSmall: + select( viewportStore ).isViewportMatch( '< medium' ), + isLarge: select( viewportStore ).isViewportMatch( 'large' ), + }; + }, + [ identifier, scope ] + ); useAdjustComplementaryListener( scope, identifier, @@ -150,7 +158,7 @@ function ComplementaryArea( { isActive && ( ! showIconLabels || isLarge ) } aria-expanded={ isActive } - aria-disabled={ isActive === undefined } + aria-disabled={ isLoading } label={ title } icon={ showIconLabels ? check : icon } showTooltip={ ! showIconLabels } From ae4a680867d70ae040355ebbd2721f92099644b2 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 2 Feb 2023 18:08:24 +0100 Subject: [PATCH 10/12] Initially set the area disabled for small screens This makes the area state values to be consistent. --- .../components/complementary-area/index.js | 49 ++++++++++++------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index 77b6268632cc85..622fb117c77be6 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -48,26 +48,29 @@ function useAdjustComplementaryListener( const { enableComplementaryArea, disableComplementaryArea } = useDispatch( interfaceStore ); useEffect( () => { - // If the complementary area is active and the editor is switching from a big to a small window size. + // If the complementary area is active and the editor is switching from + // a big to a small window size. if ( isActive && isSmall && ! previousIsSmall.current ) { - // Disable the complementary area. disableComplementaryArea( scope ); - // Flag the complementary area to be reopened when the window size goes from small to big. + // Flag the complementary area to be reopened when the window size + // goes from small to big. shouldOpenWhenNotSmall.current = true; } else if ( - // If there is a flag indicating the complementary area should be enabled when we go from small to big window size - // and we are going from a small to big window size. + // If there is a flag indicating the complementary area should be + // enabled when we go from small to big window size and we are going + // from a small to big window size. shouldOpenWhenNotSmall.current && ! isSmall && previousIsSmall.current ) { - // Remove the flag indicating the complementary area should be enabled. + // Remove the flag indicating the complementary area should be + // enabled. shouldOpenWhenNotSmall.current = false; - // Enable the complementary area. enableComplementaryArea( scope, identifier ); } else if ( - // If the flag is indicating the current complementary should be reopened but another complementary area becomes active, - // remove the flag. + // If the flag is indicating the current complementary should be + // reopened but another complementary area becomes active, remove + // the flag. shouldOpenWhenNotSmall.current && activeArea && activeArea !== identifier @@ -104,18 +107,14 @@ function ComplementaryArea( { const { getActiveComplementaryArea, isItemPinned } = select( interfaceStore ); - const isComplementaryAreaVisible = select( - preferencesStore - ).get( scope, 'isComplementaryAreaVisible' ); + const isVisible = select( preferencesStore ).get( + scope, + 'isComplementaryAreaVisible' + ); const _activeArea = getActiveComplementaryArea( scope ); - const _isSmall = - select( viewportStore ).isViewportMatch( '< medium' ); return { - isLoading: - isComplementaryAreaVisible && - _activeArea === undefined && - ! _isSmall, + isLoading: isVisible && _activeArea === undefined, isActive: _activeArea === identifier, isPinned: isItemPinned( scope, identifier ), activeArea: _activeArea, @@ -143,8 +142,20 @@ function ComplementaryArea( { useEffect( () => { if ( isActiveByDefault && activeArea === undefined && ! isSmall ) { enableComplementaryArea( scope, identifier ); + } else if ( activeArea === undefined && isSmall ) { + // The isComplementaryAreaVisible state is preserved, so in case it + // was previously true, we need to adjust and initially disable for + // small screens. + disableComplementaryArea( scope, identifier ); } - }, [ activeArea, isActiveByDefault, scope, identifier, isSmall ] ); + }, [ + activeArea, + isActiveByDefault, + scope, + identifier, + isSmall, + isLoading, + ] ); return ( <> From 06574204f1119d5c2bab10fd5143550f68da1650 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 2 Feb 2023 18:15:42 +0100 Subject: [PATCH 11/12] Reword comment --- .../interface/src/components/complementary-area/index.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index 622fb117c77be6..2e7be0432c6adb 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -140,12 +140,11 @@ function ComplementaryArea( { } = useDispatch( interfaceStore ); useEffect( () => { + // Set initial visibility: For large screens, enable if it's active by + // default. For small screens, always initially disable. if ( isActiveByDefault && activeArea === undefined && ! isSmall ) { enableComplementaryArea( scope, identifier ); } else if ( activeArea === undefined && isSmall ) { - // The isComplementaryAreaVisible state is preserved, so in case it - // was previously true, we need to adjust and initially disable for - // small screens. disableComplementaryArea( scope, identifier ); } }, [ From 573ddf9a4913c8457a106f63b2a3c3be0911c9ca Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Fri, 3 Feb 2023 14:16:02 +0100 Subject: [PATCH 12/12] Create isComplementaryAreaLoading selector --- .../components/complementary-area/index.js | 23 ++++++------------- packages/interface/src/store/selectors.js | 14 ++++++++++- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/packages/interface/src/components/complementary-area/index.js b/packages/interface/src/components/complementary-area/index.js index 2e7be0432c6adb..5617c39da301bd 100644 --- a/packages/interface/src/components/complementary-area/index.js +++ b/packages/interface/src/components/complementary-area/index.js @@ -12,7 +12,6 @@ import { __ } from '@wordpress/i18n'; import { check, starEmpty, starFilled } from '@wordpress/icons'; import { useEffect, useRef } from '@wordpress/element'; import { store as viewportStore } from '@wordpress/viewport'; -import { store as preferencesStore } from '@wordpress/preferences'; /** * Internal dependencies @@ -104,17 +103,16 @@ function ComplementaryArea( { const { isLoading, isActive, isPinned, activeArea, isSmall, isLarge } = useSelect( ( select ) => { - const { getActiveComplementaryArea, isItemPinned } = - select( interfaceStore ); + const { + getActiveComplementaryArea, + isComplementaryAreaLoading, + isItemPinned, + } = select( interfaceStore ); - const isVisible = select( preferencesStore ).get( - scope, - 'isComplementaryAreaVisible' - ); const _activeArea = getActiveComplementaryArea( scope ); return { - isLoading: isVisible && _activeArea === undefined, + isLoading: isComplementaryAreaLoading( scope ), isActive: _activeArea === identifier, isPinned: isItemPinned( scope, identifier ), activeArea: _activeArea, @@ -147,14 +145,7 @@ function ComplementaryArea( { } else if ( activeArea === undefined && isSmall ) { disableComplementaryArea( scope, identifier ); } - }, [ - activeArea, - isActiveByDefault, - scope, - identifier, - isSmall, - isLoading, - ] ); + }, [ activeArea, isActiveByDefault, scope, identifier, isSmall ] ); return ( <> diff --git a/packages/interface/src/store/selectors.js b/packages/interface/src/store/selectors.js index 13b5915e17aac5..c92e45bbd3c590 100644 --- a/packages/interface/src/store/selectors.js +++ b/packages/interface/src/store/selectors.js @@ -28,7 +28,7 @@ export const getActiveComplementaryArea = createRegistrySelector( } // Return `null` to indicate the user hid the complementary area. - if ( ! isComplementaryAreaVisible ) { + if ( isComplementaryAreaVisible === false ) { return null; } @@ -36,6 +36,18 @@ export const getActiveComplementaryArea = createRegistrySelector( } ); +export const isComplementaryAreaLoading = createRegistrySelector( + ( select ) => ( state, scope ) => { + const isVisible = select( preferencesStore ).get( + scope, + 'isComplementaryAreaVisible' + ); + const identifier = state?.complementaryAreas?.[ scope ]; + + return isVisible && identifier === undefined; + } +); + /** * Returns a boolean indicating if an item is pinned or not. *