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.

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

Fix race condition in block moving animation. #16750

Merged
merged 3 commits into from Jul 26, 2019

Conversation

@youknowriad
Copy link
Contributor

commented Jul 25, 2019

The block animations introduced in the previous release can suffer from some issues:

  • Sometimes on very quick rerenders, the animation stops in the middle causing the blocks to overflow each other.
  • The style prop that can be provided by third-party plugins is not rendered anymore.

closes #16540

@youknowriad youknowriad force-pushed the fix/race-condition-moving-animation branch from b31090f to 931f7c2 Jul 25, 2019

@jorgefilipecosta
Copy link
Member

left a comment

I used the code sample provided in the issue with JSX removed:

const { createHigherOrderComponent } = wp.compose;
const el = wp.element.createElement;

const withStyles = createHigherOrderComponent( ( BlockListBlock ) => {
	return ( props ) => {
		if ( props.block.name !== 'core/image' ) {
			return el( BlockListBlock, props );
		}

		let wrapperProps = props.wrapperProps;
		wrapperProps = {
			...wrapperProps,
			style: {
				...( wrapperProps && { ...wrapperProps.style } ),
				marginBottom: '30px',
			},
		};
		return el( BlockListBlock, { ...props, wrapperProps } );
	};
}, 'withStyles' );

wp.hooks.addFilter( 'editor.BlockListBlock', `my-plugin/core-image-blb-with-styles`, withStyles );

This PR fixed the problem, but I feel the reducers logic is complex to follow. Can it be coded in a way that is easy to understand for people seeing it for the first time? If not, I guess we should add a small comment explaining how the reducers work.

@@ -29,10 +29,17 @@ import { useReducedMotion } from '@wordpress/compose';
*/
function useMovingAnimation( ref, isSelected, enableAnimation, triggerAnimationOnChange ) {
const prefersReducedMotion = useReducedMotion() || ! enableAnimation;
const [ resetAnimation, setResetAnimation ] = useState( false );
const [ triggeredAnimation, triggerAnimation ] = useReducer( ( state = 0 ) => state + 1 );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 25, 2019

Member

Is useReducer needed here? The logic using a reducer to do a sum is complex to follow, can we use two-state flags (triggeredAnimation, finishedAnimation) and change the flags accordingly?
If using useReducer can we use a single reducer with multiple actions if that makes things more simple?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Author Contributor

I find it simpler this way personally. Each hook keeps track of the "id" of the animation. Basically, it's just a counter that increments each time you call the dispatch function.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Author Contributor

Could this be a personal preference. Or maybe it's just because we're not used to using useReducer that much?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Author Contributor

I don't want to discard that feedback thought, what do you think? maybe others could share opinions here. @aduth @epiqueras

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 25, 2019

Contributor

useReducer is useful when you need handlers to read props and current state, but don't want to create a new function on every render or whenever they change if you are memoizing. In this case you could just use setState:

const [ state, setState ] = useState( 0 );
setState( ( state ) => ++state );

I don't think that makes a big difference though. I do agree with @jorgefilipecosta that it would be nicer if we could just use 2 booleans instead of ever increasing numbers. Something like:

const [ { started, finished }, setAnimationState ] = useState( {
	started: false,
	finished: false,
} );

Also note that you should pass initialState as the second parameter to useReducer instead of using a default parameter in the reducer function, so that it is defined before the first update.

https://github.com/facebook/react/blob/42b75ab007a5e7c159933cfdbf2b6845d89fc7f2/packages/react-reconciler/src/ReactFiberHooks.js#L635

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 26, 2019

Author Contributor

Ohhh, I wasn't clear enough. We can't use booleans (it was the previous approach) to fix the race conditions completely. We need to be sure all the animations (multiple rerenders can trigger two in a row) have been triggered before resetting.

const [ state, setState ] = useState( 0 );
setState( ( state ) => ++state );

This is actually a bad pattern, it creates unexpected bugs especially if you trigger setState from effects because state can be outdated at that point. React team talked about that a little bit. The idea is that each time the updated state value depends on the previous state value, it's better to use useReducer.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 26, 2019

Author Contributor

I extracted the reducer to avoid recreating a function on each render, I also used the default value argument.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 26, 2019

Contributor

Ohhh, I wasn't clear enough. We can't use booleans (it was the previous approach) to fix the race conditions completely. We need to be sure all the animations (multiple rerenders can trigger two in a row) have been triggered before resetting.

You could achieve that with 2 booleans instead of 1 like the previous approach. But, I can see why numbers make this easier.

This is actually a bad pattern, it creates unexpected bugs especially if you trigger setState from effects because state can be outdated at that point. React team talked about that a little bit. The idea is that each time the updated state value depends on the previous state value, it's better to use useReducer.

That's why my example uses the functional variant of setState. I am not closing over the state, React passes the latest value as a param and I operate on that.

I extracted the reducer to avoid recreating a function on each render, I also used the default value argument.

🚀

@jorgefilipecosta
Copy link
Member

left a comment

Approving this PR as the reason the code was not easy to understand is probably related to the fact that useReducer is still not common in the codebase.

@youknowriad youknowriad merged commit d0ea5bf into master Jul 26, 2019

1 of 2 checks passed

Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the fix/race-condition-moving-animation branch Jul 26, 2019

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

{ ...blockWrapperProps }
style={

This comment has been minimized.

Copy link
@aduth

aduth Jul 30, 2019

Member

This reordering clobbers any styles assigned by blockWrapperProps.

Notably: Column widths.

getEditWrapperProps( attributes ) {
const { width } = attributes;
if ( Number.isFinite( width ) ) {
return {
style: {
flexBasis: width + '%',
},
};
}
},

This comment has been minimized.

Copy link
@aduth

aduth Jul 30, 2019

Member

What's the difference between wrapperProps and blockWrapperProps anyways? It's not very clear.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 30, 2019

Author Contributor

I don't understand. I'm merging the styles here

{
						...wrapperProps.style,
						...animationStyle,
					}

so it should continue to work?

This comment has been minimized.

Copy link
@aduth

aduth Jul 30, 2019

Member

Oh, I guess blockWrapperProps just merges into wrapperProps anyways:

// Determine whether the block has props to apply to the wrapper.
let blockWrapperProps = wrapperProps;
if ( blockType.getEditWrapperProps ) {
blockWrapperProps = {
...blockWrapperProps,
...blockType.getEditWrapperProps( attributes ),
};
}

So the solution should probably be that we don't ever reference wrapperProps directly after this merge, and instead always use blockWrapperProps (either that or we just remove the separate variable and merge into itself).

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 30, 2019

Author Contributor

Oh I missed that. yeah, that should work.

This comment has been minimized.

Copy link
@aduth
{ ...blockWrapperProps }
style={
wrapperProps && wrapperProps.style ?

This comment has been minimized.

Copy link
@miina

miina Aug 2, 2019

Contributor

@youknowriad Question: Is it necessary to add animationStyle even if enableAnimation is set to false? This way it's overriding the potential transform style added by wrapperProps.style.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 2, 2019

Author Contributor

Probably not necessary in that case.

This comment has been minimized.

Copy link
@miina

miina Aug 2, 2019

Contributor

OK, thanks, will create a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.