Tools Panel: Fix race conditions caused by conditionally displayed ToolsPanelItems #36588
Conversation
|
Size Change: +66 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
andrewserong
added
[Type] Bug
There was a problem hiding this comment.
Nice work diving into this one @stacimc, looks like a particularly tricky issue, and I like the look of all the added callbacks passed to the set functions (and thanks for adding the storybook examples — they're working well for me 👍)
Unfortunately, I think this might introduce another regression, which I could replicate on this branch, but not on trunk where when switching between two similar blocks that both use the same ToolsPanel (in this case Typography), the default control winds up not rendering at all.
To replicate:
- Add a couple of paragraph blocks to a post, but don't change any of the Typography settings available
- Add another paragraph block, and add in a few more Typography controls and set a value
- Select the first paragraph block
- The Size control in the Typography panel is not visible (and there appear to be no panels registered)
Here's a gif:
I'm not quite sure what would be causing this, but I've added a note in the registerPanelItem function where we're mutating items, just in case that might be related. CC: @aaronrobertshaw and @ciampo in case either of them have a better idea of what might cause it.
I was also wondering if it'd be worth trying to write a unit test for this change, in case that'd help isolate the issue above? (Though, it could be tricky trying to cover the race condition 🤔)
|
Thanks @andrewserong for the testing -- it looks like I may need to make sure I'm keeping track of the
Definitely agreed! |
|
I think I have the issue @andrewserong brought up fixed; I'm definitely a bit shaky on some of the ToolsPanel implementation, so hoping @aaronrobertshaw can take a look here :) I'll give some more unit tests a go now |
There was a problem hiding this comment.
Thanks for taking this on @stacimc, there are a lot of moving parts to consider!
@andrewserong was too quick for me 🚀
I also ran into the same regression he outlined and got distracted looking into that and tests. We'll definitely want to improve the test coverage here. An entry into the components changelog will also be needed.
A couple of rough tests to start
diff --git a/packages/components/src/tools-panel/test/index.js b/packages/components/src/tools-panel/test/index.js
index 2d3fcf022f..6dab5ba148 100644
--- a/packages/components/src/tools-panel/test/index.js
+++ b/packages/components/src/tools-panel/test/index.js
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, screen, fireEvent, within } from '@testing-library/react';
/**
* Internal dependencies
@@ -168,6 +168,7 @@ const selectMenuItem = async ( label ) => {
describe( 'ToolsPanel', () => {
afterEach( () => {
controlProps.attributes.value = true;
+ altControlProps.attributes.value = false;
} );
describe( 'basic rendering', () => {
@@ -348,6 +349,162 @@ describe( 'ToolsPanel', () => {
// there.
expect( optionalItem ).not.toBeInTheDocument();
} );
+
+ it( 'should render default controls with conditional isShownByDefault', async () => {
+ const linkedControlProps = {
+ attributes: { value: false },
+ hasValue: jest.fn().mockImplementation( () => {
+ return !! linkedControlProps.attributes.value;
+ } ),
+ label: 'Linked',
+ onDeselect: jest.fn(),
+ onSelect: jest.fn(),
+ };
+
+ const { rerender } = render(
+ <ToolsPanel { ...defaultProps }>
+ <ToolsPanelItem
+ { ...altControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Default control</div>
+ </ToolsPanelItem>
+ <ToolsPanelItem
+ { ...linkedControlProps }
+ isShownByDefault={ !! altControlProps.attributes.value }
+ >
+ <div>Linked control</div>
+ </ToolsPanelItem>
+ </ToolsPanel>
+ );
+
+ let linkedItem = screen.queryByText( 'Linked control' );
+ expect( linkedItem ).not.toBeInTheDocument();
+
+ // Simulate the main control having a value set which should
+ // trigger the linked control becoming a default control via the
+ // conditional `isShownByDefault` prop.
+ altControlProps.attributes.value = true;
+
+ rerender(
+ <ToolsPanel { ...defaultProps }>
+ <ToolsPanelItem
+ { ...altControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Default control</div>
+ </ToolsPanelItem>
+ <ToolsPanelItem
+ { ...linkedControlProps }
+ isShownByDefault={ !! altControlProps.attributes.value }
+ >
+ <div>Linked control</div>
+ </ToolsPanelItem>
+ </ToolsPanel>
+ );
+
+ // The linked control should now be a default control and rendered
+ // despite not having a value.
+ linkedItem = screen.getByText( 'Linked control' );
+ expect( linkedItem ).toBeInTheDocument();
+
+ // The linked control should now appear in the default controls
+ // menu group and have been removed from the optional group.
+ openDropdownMenu();
+ const menuGroups = screen.getAllByRole( 'group' );
+
+ // There should now only be two groups. The default controls and
+ // and the group for the reset all option.
+ expect( menuGroups.length ).toEqual( 2 );
+
+ // The new default control item for the Linked control should be
+ // within the first menu group.
+ const defaultItem = within( menuGroups[ 0 ] ).getByText( 'Linked' );
+ expect( defaultItem ).toBeInTheDocument();
+
+ // Optional controls have an additional aria-label. This can be used
+ // to confirm the conditional default control has been removed from
+ // the optional menu item group.
+ const optionalItem = screen.queryByRole( 'menuitemcheckbox', {
+ name: 'Show Linked',
+ } );
+ expect( optionalItem ).not.toBeInTheDocument();
+ } );
+
+ it( 'should handle conditionally rendered default control', async () => {
+ const conditionalControlProps = {
+ attributes: { value: false },
+ hasValue: jest.fn().mockImplementation( () => {
+ return !! conditionalControlProps.attributes.value;
+ } ),
+ label: 'Conditional',
+ onDeselect: jest.fn(),
+ onSelect: jest.fn(),
+ };
+
+ const { rerender } = render(
+ <ToolsPanel { ...defaultProps }>
+ <ToolsPanelItem
+ { ...altControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Default control</div>
+ </ToolsPanelItem>
+ { !! altControlProps.attributes.value && (
+ <ToolsPanelItem
+ { ...conditionalControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Conditional control</div>
+ </ToolsPanelItem>
+ ) }
+ </ToolsPanel>
+ );
+
+ // The conditional control should not yet be rendered.
+ let conditionalItem = screen.queryByText( 'Conditional control' );
+ expect( conditionalItem ).not.toBeInTheDocument();
+
+ // Simulate the main control having a value set which will now
+ // render the new default control into the ToolsPanel.
+ altControlProps.attributes.value = true;
+
+ rerender(
+ <ToolsPanel { ...defaultProps }>
+ <ToolsPanelItem
+ { ...altControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Default control</div>
+ </ToolsPanelItem>
+ { !! altControlProps.attributes.value && (
+ <ToolsPanelItem
+ { ...conditionalControlProps }
+ isShownByDefault={ true }
+ >
+ <div>Conditional control</div>
+ </ToolsPanelItem>
+ ) }
+ </ToolsPanel>
+ );
+
+ // The conditional control should now be rendered and included in
+ // the panel's menu.
+ conditionalItem = screen.getByText( 'Conditional control' );
+ expect( conditionalItem ).toBeInTheDocument();
+
+ // The conditional control should now appear in the default controls
+ // menu group.
+ openDropdownMenu();
+ const menuGroups = screen.getAllByRole( 'group' );
+
+ // The new default control item for the Conditional control should
+ // be within the first menu group.
+ const defaultItem = within( menuGroups[ 0 ] ).getByText(
+ 'Conditional'
+ );
+ expect( defaultItem ).toBeInTheDocument();
+ } );
} );
describe( 'callbacks on menu item selection', () => {P.S. Just prior to hitting submit on this I noticed fresh commits. I'll retest shortly.
|
Checking the Nice work @stacimc ✨ |
Me too, that's testing well for me now. Nice find, Staci! |
|
Added a couple of tests, working on top of the suggestions by @aaronrobertshaw, plus a unit test for the issue @andrewserong found. Confirmed that the test fails without that latest fix and passes now 😄 |
There was a problem hiding this comment.
Thanks for following up with the tests @stacimc! Unfortunately, I think I've found another issue, similar to the previous one, but where switching between the two blocks that share the same Typography control results in losing the value for the font size:
I'm wondering if it's possible that the onDeselect is somehow being triggered when switching between blocks?
Once we introduced the use of SlotFills to inject items into the My guess would be that the changes around |
|
I hope you don't mind @stacimc, I pushed a commit ( 5d16f7a ) that prevents the last regression we found. Please feel free to revert this if you see a better option. My thinking was pushing it now would help others to test this and the Post Featured Image Dimensions PR. The check made before flagging a default control as customized in the menu was altered to not only update once a control had been customized by a user but also when they "undid" that customization. This also meant that when that second case occurred, the item's 5d16f7a brings the UX back to what it is on trunk for default controls. During its development, it was requested that as soon as a user modifies a default control it should show as customized in the panel menu until reset via that menu. If we wish to change that behaviour I suggest we address it in another PR. With the behaviour around flagging default controls as customized restored, this tested well for me. All the unit tests pass and selections are honoured when switching between blocks in the editor. Rebasing #36540 onto this has it testing fine as well. Screen.Recording.2021-11-18.at.5.38.38.pm.mp4Let me know what you think. |
There was a problem hiding this comment.
@andrewserong was too quick for me 🚀
You have all been extremely quick! Props to everyone for the excellent collaboration, and for not only fixing the issue, but also improving the unit tests and the storybook examples! 🙇
I had a look at the code and at the Storybook examples and everything worked well for me, although I'll let @andrewserong and @aaronrobertshaw give the final ✅
|
Thanks all for the quick responses and very thorough testing, much appreciated ✨
I don't mind at all, thank you very much for the quick fix @aaronrobertshaw!
Thanks for this context -- very happy to leave the behavior as requested, and I agree that should be addressed elsewhere if we decide to revisit. The persistence of the dirty state in the menu when emptying the input stood out to me as I was testing the conditionally displayed panels, but I missed that this behavior is consistent with other controls and on trunk. Seems very reasonable to me. The fix works great for me and I think it's the right way to go 👍 |
There was a problem hiding this comment.
Great work @stacimc 👍
This is testing as advertised for me.
✅ Existing block support panels continue to function
✅ Storybook examples look good
✅ Rebasing changes from #36540 onto these tests well
I left a couple of minor comments which I'll leave up to you to decide if you want to action.
That aside, this LGTM 
There was a problem hiding this comment.
This is looking good to me now, too:
✅ Regressions when switching between blocks with common panel items appear to be fixed
✅ Storybook examples still work nicely in manual testing
Thanks for the detailed work here @stacimc and for fixing up that last regression and providing the extra context @aaronrobertshaw!
LGTM 🎉
stacimc
force-pushed
the
fix/tools-panel-conditionally-displayed-items
branch
from
ace59ca to
0cac6cc
Compare
…ritten It's possible to encounter race conditions when updating the panelItems and menuItems if you have a ToolsPanelItem that's displayed conditionally, or which is shown by default only conditionally. This can cause multiple calls to update the state in quick sequence, which can result in those calls overwriting one another. To prevent this, the calls to setState have been updated to depend on the previous state.
stacimc
force-pushed
the
fix/tools-panel-conditionally-displayed-items
branch
from
0cac6cc to
8e8db97
Compare
|
Just rebased for merge conflicts in the changelog. When builds pass I think this should be ready for merge. |
…olsPanelItems (#36588) * Ensure state changes happen in order and flagged values are not overwritten It's possible to encounter race conditions when updating the panelItems and menuItems if you have a ToolsPanelItem that's displayed conditionally, or which is shown by default only conditionally. This can cause multiple calls to update the state in quick sequence, which can result in those calls overwriting one another. To prevent this, the calls to setState have been updated to depend on the previous state. * Add storybook examples * Update storybook example name * Do not mutate existing state when registering PanelItems * Prevent deregistration of panelItems when switching between panels * Linting Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> * Add unit tests for conditionally displayed panel items * Add unit test for regression with deregistration of panelItems * Prevent triggering onDeselect * Clean up tests * Changelog * Clarify comments in tests Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Description
Problem
In #36540 it was discovered that attempting to conditionally display a ToolsPanelItem causes unreliable panel behavior such as controls suddenly disappearing. This happens both when:
defaultandoptionalcontrols in the Menu:This behavior is caused by race conditions in setting the state of the panel and menu items in quick sequence.
Fixes in this PR
default controls will now indicate that they do not have a value when the control is clearedHow has this been tested?
Unit tests:
Run
npm run test-unit "./packages/components/src/tools-panel/test/index.js"npm run storybook:devScreenshots
conditional-default.mov
conditionally-rendered-control.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).