Fix ComplementaryArea's initial isActive state #47498
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +220 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Jan 27, 2023
|
Flaky tests detected in 573ddf9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4084704360
|
Mamaduka
reviewed
Jan 27, 2023
packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts
Show resolved
Hide resolved
|
@WunderBart, the changes look good to me 👍 Do you have manual testing instructions, or should we rely on CI tests? |
Thanks for the review, @Mamaduka! 🙇 I think CI should tell us (based on the tests using the Settings sidebar) if the change is OK. 🤞 |
jsnajdr
reviewed
Jan 27, 2023
There was a problem hiding this comment.
This looks like a good fix 👍
The main issue is that on editor boot, enabling the sidebar is a two-step process:
- There is a server-supplied preference,
isComplementaryAreaVisible. Server sends that in the initial HTML and when the sidebar is enabled, this value istruefrom the very beginning. - The sidebar has two tabs, with ids
edit-post/documentandedit-post/block. Initially none is selected, and theactiveAreaisundefined, although the sidebar is opened. - Only a bit later, in an effect, the
activeAreais set to a valid value. Either in an effect that sets theisActiveByDefaultvalue, or in an effect that observes block selection and selects the document ("Post", "Template", ...) tab when no block is selected, and switches to "Block" once a block gets selected.
This PR catches that ambiguous "sidebar opened but no tab selected" situation and disables the toggle button while it lasts.
packages/e2e-test-utils-playwright/src/editor/open-document-settings-sidebar.ts
Show resolved
Hide resolved
WunderBart
force-pushed
the
fix/complementary-area-initial-state
branch
from
725ad50 to
fd4c55d
Compare
kevin940726
approved these changes
Jan 30, 2023
mirka
added
[Package] Interface
WunderBart
force-pushed
the
fix/complementary-area-initial-state
branch
from
fd4c55d to
1369279
Compare
The panel is always initially closed in mobile viewport
This makes more sense contextually. `expect` should be reserved for the test subject.
This is consistent with the current settings sidebar name in the editor.
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.
WunderBart
force-pushed
the
fix/complementary-area-initial-state
branch
from
1369279 to
ed67d4d
Compare
This makes the area state values to be consistent.
WunderBart
force-pushed
the
fix/complementary-area-initial-state
branch
from
39ae045 to
573ddf9
Compare
WunderBart
commented
Feb 3, 2023
| @@ -69,7 +69,7 @@ export function SidebarComplementaryAreaFills() { | |||
| identifier={ sidebarName } | |||
| title={ __( 'Settings' ) } | |||
| icon={ isRTL() ? drawerLeft : drawerRight } | |||
| closeLabel={ __( 'Close settings sidebar' ) } | |||
There was a problem hiding this comment.
It's called Close settings everywhere else, so it made sense to me to make it consistent.
|
👋 @kevin940726 @jsnajdr @talldan, I'd appreciate it if you could take another look - I needed to make some updates to address the persisting area visibility preference between viewports. |
|
👋 @kevin940726 friendly ping in case you missed the last one! Could you take another look at this one? 🙏 |
kevin940726
approved these changes
Feb 13, 2023
DaisyOlsen
added
[Feature] Site Editor
fullofcaffeine
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Feb 28, 2023
fullofcaffeine
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Feb 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
ComplementaryAreacomponent,Why?
The active complementary area identifier is initially unavailable as it needs to be loaded into the store. This leads to a situation where the initial
isActivestate of theComplementaryAreacomponent isfalseand, after one or more cycles, becomestrue.While this change doesn't really manifest itself visually (it lasts only a few milliseconds), it is a good practice in the E2E testing context because it reliably signals when the component is ready to interact.
For example, if an E2E test wants to open the
Editor settingssidebar (which uses theComplementaryAreacomponent), it needs to check whether that sidebar is already opened before it clicks the toggle button. If that sidebar's initialisActivestate isfalse, but the actual state istrue(which will kick in with a delay), we might end up closing the sidebar instead if the click is late by a few milliseconds.Also, as @kevin940726 mentioned, this change "might also have a positive impact for assistive technologies, which they usually evaluate the context of the page as soon as possible for performance reasons."
How to test
The E2E tests should cover this change, so it shouldn't need manual testing.
Related
openDocumentSettingsSidebarutil not opening the sidebar #43506