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.

Sign up for GitHub

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

Enable React StrictMode again #47639

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Enable React StrictMode again #47639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
532b1d9
Enable React StricMode again
jsnajdr Feb 1, 2023
f0fb47f
Enable concurrent and strict mode in customize-widgets, too
jsnajdr Feb 2, 2023
4cd93c7
FocusControl: unbind preview event only if it was bound
jsnajdr Feb 2, 2023
207e16d
Customizing widgets test: change how navigation mode is detected. Res…
jsnajdr Feb 2, 2023
6a8a55f
Customizing widgets test: click on widget test to unfocus editor and …
jsnajdr Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions packages/customize-widgets/src/components/focus-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,26 @@ export default function FocusControl( { api, sidebarControls, children } ) {
focusWidget( widgetId );
}

let previewBound = false;

function handleReady() {
api.previewer.preview.bind(
'focus-control-for-setting',
handleFocus
);
previewBound = true;
}

api.previewer.bind( 'ready', handleReady );

return () => {
api.previewer.unbind( 'ready', handleReady );
api.previewer.preview.unbind(
'focus-control-for-setting',
handleFocus
);
if ( previewBound ) {
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
api.previewer.preview.unbind(
'focus-control-for-setting',
handleFocus
);
}
};
}, [ api, focusWidget ] );

Expand Down
17 changes: 9 additions & 8 deletions packages/customize-widgets/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { render } from '@wordpress/element';
import { StrictMode, createRoot } from '@wordpress/element';
import {
registerCoreBlocks,
__experimentalGetCoreBlocks,
Expand Down Expand Up @@ -91,13 +91,14 @@ export function initialize( editorName, blockEditorSettings ) {
}
} );

render(
<CustomizeWidgets
api={ wp.customize }
sidebarControls={ sidebarControls }
blockEditorSettings={ blockEditorSettings }
/>,
container
createRoot( container ).render(
<StrictMode>
<CustomizeWidgets
api={ wp.customize }
sidebarControls={ sidebarControls }
blockEditorSettings={ blockEditorSettings }
/>
</StrictMode>
);
} );
}
42 changes: 23 additions & 19 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
store as editorStore,
experiments as editorExperiments,
} from '@wordpress/editor';
import { useMemo } from '@wordpress/element';
import { StrictMode, useMemo } from '@wordpress/element';
import { SlotFillProvider } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';
Expand Down Expand Up @@ -173,24 +173,28 @@ function Editor( { postId, postType, settings, initialEdits, ...props } ) {
}

return (
<ShortcutProvider>
<SlotFillProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={ isTemplateMode ? template : undefined }
{ ...props }
>
<ErrorBoundary>
<EditorInitialization postId={ postId } />
<Layout styles={ styles } />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
<StrictMode>
<ShortcutProvider>
<SlotFillProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={
isTemplateMode ? template : undefined
}
{ ...props }
>
<ErrorBoundary>
<EditorInitialization postId={ postId } />
<Layout styles={ styles } />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
</StrictMode>
);
}

Expand Down
24 changes: 13 additions & 11 deletions packages/edit-site/src/components/app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import { SlotFillProvider, Popover } from '@wordpress/components';
import { UnsavedChangesWarning } from '@wordpress/editor';
import { StrictMode } from '@wordpress/element';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';
import { store as noticesStore } from '@wordpress/notices';
import { useDispatch } from '@wordpress/data';
Expand Down Expand Up @@ -31,16 +32,17 @@ export default function App() {
}

return (
<ShortcutProvider style={ { height: '100%' } }>
<SlotFillProvider>
<Popover.Slot />
<UnsavedChangesWarning />

<Routes>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</Routes>
</SlotFillProvider>
</ShortcutProvider>
<StrictMode>
<ShortcutProvider style={ { height: '100%' } }>
<SlotFillProvider>
<Popover.Slot />
<UnsavedChangesWarning />
<Routes>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</Routes>
</SlotFillProvider>
</ShortcutProvider>
</StrictMode>
);
}
40 changes: 22 additions & 18 deletions packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
store as coreStore,
useResourcePermissions,
} from '@wordpress/core-data';
import { useMemo } from '@wordpress/element';
import { StrictMode, useMemo } from '@wordpress/element';
import {
BlockEditorKeyboardShortcuts,
CopyHandler,
Expand Down Expand Up @@ -99,22 +99,26 @@ export default function WidgetAreasBlockEditorProvider( {
);

return (
<ShortcutProvider>
<BlockEditorKeyboardShortcuts.Register />
<KeyboardShortcuts.Register />
<SlotFillProvider>
<ExperimentalBlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ settings }
useSubRegistry={ false }
{ ...props }
>
<CopyHandler>{ children }</CopyHandler>
<ReusableBlocksMenuItems rootClientId={ widgetAreaId } />
</ExperimentalBlockEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
<StrictMode>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to enable it in the @wordpress/customize-widgets package too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good catch, a forgotten legacy use of render :) I fixed it in f0fb47f.

And I fixed it in a way that's IMO slightly better that how we do it in other apps. Instead of rendering <StrictMode> inside the top-level component, I use <StrictMode> as the very top-level component, in the render call.

Because otherwise, React devtools show a warning that the top level component is not in strict mode. Because only its children are:

Screenshot 2023-02-02 at 10 03 48

A completely harmless warning, but still a warning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now the e2e tests for customize-widgets started failing 🤦

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 I've rerun them just in case it was a flaky run. Feel free to revert the change and leave it for another PR if you feel it introduces extra flakiness, though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to figure out the failures and fix them.

One was a buggy adding and removing event listeners in FocusControl. The useEffect cleanup function removes listener that was never added from an object that doesn't exist, leading to crash. Caused by React unmounting the effect and then mounting it again.

Then there was a check if the editor is in navigation mode, which was searching for dynamic "you are currently in navigation mode" text element, added by speak(). But there are two speak() calls racing with each other, editor mode switch and block selection, and concurrent mode reversed their order. I fixed that by searching for .is-navigate-mode CSS selector instead.

The last one is a pre-existing failure where the element to click on is obcured by block toolbar:

Screenshot 2023-02-02 at 13 39 09

This has been auto-reported in #38832 for almost a year. I fixed this by first clicking on the page text, unfocusing the editor and hiding the toolbar, and only then clicking on the blue round "edit widget" icon that's no longer obscured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been auto-reported in #38832 for almost a year.

Seems like it's a different failure though. 🤔
I think the reported issue might be a legit flaky behavior of the customizer but I didn't verify it 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this block toolbar overlap when running this locally. Also, the failures-artifact zip file will have screenshots when this fails in CI, can be inspected.

<ShortcutProvider>
<BlockEditorKeyboardShortcuts.Register />
<KeyboardShortcuts.Register />
<SlotFillProvider>
<ExperimentalBlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ settings }
useSubRegistry={ false }
{ ...props }
>
<CopyHandler>{ children }</CopyHandler>
<ReusableBlocksMenuItems
rootClientId={ widgetAreaId }
/>
</ExperimentalBlockEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
</StrictMode>
);
}
7 changes: 5 additions & 2 deletions test/e2e/specs/widgets/customizing-widgets.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ test.describe( 'Widgets Customizer', () => {
await expect( firstParagraphBlock ).toBeFocused();

// Expect to focus on a already focused widget.
await paragraphWidget.click(); // noop click on the widget text to unfocus the editor and hide toolbar
await editParagraphWidget.click();
await expect( firstParagraphBlock ).toBeFocused();

Expand All @@ -272,6 +273,8 @@ test.describe( 'Widgets Customizer', () => {
const editHeadingWidget = headingWidget.locator(
'role=button[name="Click to edit this widget."i]'
);

await headingWidget.click(); // noop click on the widget text to unfocus the editor and hide toolbar
await editHeadingWidget.click();

const headingBlock = page.locator(
Expand Down Expand Up @@ -463,9 +466,9 @@ test.describe( 'Widgets Customizer', () => {
await page.keyboard.press( 'Escape' );
await expect(
page.locator(
'*[aria-live="polite"][aria-relevant="additions text"] >> text=/^You are currently in navigation mode./'
'css=.block-editor-block-list__layout.is-navigate-mode'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we find another accessible selector for this? Using CSS selectors is generally not recommended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but this was the only indication in the HTML markup that the editor is in navigation mode. Previously this test used the "you are currently in navigation mode" text in the aria-live region used by speak(), but that gets quickly overwritten by the next speak() call.

Copy link
Member

@kevin940726 kevin940726 Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some ideas:

  1. If the speak() call gets overridden, does it also mean that the message might not be accessible to screen readers or other assistive technologies? Will that be a concern in this case?
  2. A common alternative to CSS selectors is using data-testid (or other public API if there's any), is it suitable here?

Anyway, this isn't that important and more like a nitpick 😅. We might want to restrict CSS selectors in e2e tests in the long term though, it'd be good to start these discussions early 🙂 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the speak() call gets overridden, does it also mean that the message might not be accessible to screen readers or other assistive technologies?

The problem here is that when you press Escape to enter the navigation mode, two things happen at once:

  1. The editor mode changes, and is announced by saying "you are currently in navigation mode".
  2. The BlockSelectionButton for the selected block appears (it's the black thing on the screenshot below) and it announces itself by saying "Paragraph Block. Row 1."

Screenshot 2023-02-03 at 14 52 42

Both happen at the same time, while handling the Escape key event. The order is determined by random things like which store listeners runs first or which update is scheduled to which microtask. Enabling concurrent mode reversed this order.

What does the screen reader do in this case? I don't know. Does it say the first message and then the second? Or does it ignore the first one, because it disappeared very quickly?

)
).toHaveCount( 1 );
).toBeVisible();
await expect( paragraphBlock ).toBeVisible();
} );

Expand Down