Enable React StrictMode again #47639
Conversation
|
Size Change: +61 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6a8a55f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4075152937
|
jsnajdr
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Suprisingly, e2e tests are passing with strict mode without any additional fixes. Some are flaky, but there are no permafails. |
| </ExperimentalBlockEditorProvider> | ||
| </SlotFillProvider> | ||
| </ShortcutProvider> | ||
| <StrictMode> |
There was a problem hiding this comment.
Does it make sense to enable it in the @wordpress/customize-widgets package too?
There was a problem hiding this comment.
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:
A completely harmless warning, but still a warning.
There was a problem hiding this comment.
And now the e2e tests for customize-widgets started failing 🤦
There was a problem hiding this comment.
😅 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
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
|
I think E2E tests aren’t run in script debug mode. At least based on this issue #17355. It would be good to see if we get different results. |
jsnajdr
force-pushed
the
enable/strict-mode
branch
from
e04809c to
532b1d9
Compare
|
Now I see that parts of the Playwright test suite never finished. Restarted them now. |
From what I understand, this doesn't affect how the app works, it's more about best practices and warnings issued by React in dev mode. At least that was my perception when I glanced through the errors in #17355. Having strict mode enabled will not affect production at all, but at the same time, it will help us locate and fix those bugs. Last, but not least, the strict mode was enabled for the post editor before we enabled concurrent mode, and since tests are passing and testing doesn't seem to indicate anything major broken, I don't see a good reason not to turn it on again. Worst case scenario, something can break for the development environment, and we can just revert this PR. |
Second run is completely green ✅ |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
jsnajdr
force-pushed
the
enable/strict-mode
branch
from
8a139bb to
6a8a55f
Compare
|
This PR actually broke the editors. In site editor for example the editor is not shown. And in block editor are other things broken.. |
|
@ntsekouras Is this local dev mode? |
|
Revert PR is #47701. |
|
Seems that |
|
@ntsekouras Rather than reverting this PR, can you try out this simple fix for |
|
Will test the new PR now @jsnajdr ! |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
| @@ -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' | |||
There was a problem hiding this comment.
Could we find another accessible selector for this? Using CSS selectors is generally not recommended.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Some ideas:
- 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? - 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 🙂 .
There was a problem hiding this comment.
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:
- The editor mode changes, and is announced by saying "you are currently in navigation mode".
- The
BlockSelectionButtonfor the selected block appears (it's the black thing on the screenshot below) and it announces itself by saying "Paragraph Block. Row 1."
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?
DaisyOlsen
added
[Type] Performance
Followup to #46467. Enable
StrictModeagain. It was disabled inedit-post, andedit-siteandedit-widgetsnever enabled it at all. Now it's everywhere. Let's fix the remaining e2e failures triggered by strict mode and then merge.