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

Jump to bottom

Block editor: create automatic change higher order reducer #48312

Merged
merged 3 commits into from
Feb 23, 2023
Merged

Block editor: create automatic change higher order reducer #48312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89aa3ad
Block editor: create automatic change higher order reducer
ellatrix Feb 22, 2023
b81339b
Move reducer into higher order reducer
ellatrix Feb 22, 2023
43b0f74
Make test more stable
ellatrix Feb 22, 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
111 changes: 68 additions & 43 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1416,9 +1416,19 @@ export function selection( state = {}, action ) {
}
}

const selectionStart = selectionHelper( state.selectionStart, action );
const selectionEnd = selectionHelper( state.selectionEnd, action );

if (
selectionStart === state.selectionStart &&
selectionEnd === state.selectionEnd
) {
return state;
Copy link
Member Author

@ellatrix ellatrix Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also noticed that we were continuously creating a new object here. I'm curious if this improves performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would if there are selectors using the parent key as memoization key.

}

return {
selectionStart: selectionHelper( state.selectionStart, action ),
selectionEnd: selectionHelper( state.selectionEnd, action ),
selectionStart,
selectionEnd,
};
}

Expand Down Expand Up @@ -1750,45 +1760,6 @@ export function lastBlockAttributesChange( state = null, action ) {
return state;
}

/**
* Reducer returning automatic change state.
*
* @param {?string} state Current state.
* @param {Object} action Dispatched action.
*
* @return {string | undefined} Updated state.
*/
export function automaticChangeStatus( state, action ) {
switch ( action.type ) {
case 'MARK_AUTOMATIC_CHANGE':
return 'pending';
case 'MARK_AUTOMATIC_CHANGE_FINAL':
if ( state === 'pending' ) {
return 'final';
}

return;
case 'SELECTION_CHANGE':
// As long as the state is not final, ignore any selection changes.
if ( state !== 'final' ) {
return state;
}

return;
// Undoing an automatic change should still be possible after mouse
// move or after visibility change.
case 'SET_BLOCK_VISIBILITY':
case 'START_TYPING':
case 'STOP_TYPING':
case 'UPDATE_BLOCK_LIST_SETTINGS':
return state;
}

// TODO: This is a source of bug, as each time there's a change in timing,
// or a new action is added, this could break.
// Reset the state by default (for any action not handled).
}

/**
* Reducer returning current highlighted block.
*
Expand Down Expand Up @@ -1863,7 +1834,7 @@ export function temporarilyEditingAsBlocks( state = '', action ) {
return state;
}

export default combineReducers( {
const combinedReducers = combineReducers( {
blocks,
isTyping,
isBlockInterfaceHidden,
Expand All @@ -1881,9 +1852,63 @@ export default combineReducers( {
lastBlockAttributesChange,
editorMode,
hasBlockMovingClientId,
automaticChangeStatus,
highlightedBlock,
lastBlockInserted,
temporarilyEditingAsBlocks,
blockVisibility,
} );

function withAutomaticChangeReset( reducer ) {
return ( state, action ) => {
const nextState = reducer( state, action );

if ( ! state ) {
return nextState;
}

// Take over the last value without creating a new reference.
nextState.automaticChangeStatus = state.automaticChangeStatus;

if ( action.type === 'MARK_AUTOMATIC_CHANGE' ) {
return {
...nextState,
automaticChangeStatus: 'pending',
};
}

if (
action.type === 'MARK_AUTOMATIC_CHANGE_FINAL' &&
state.automaticChangeStatus === 'pending'
) {
return {
...nextState,
automaticChangeStatus: 'final',
};
}

// If there's a change that doesn't affect blocks or selection, maintain
// the current status.
if (
nextState.blocks === state.blocks &&
nextState.selection === state.selection
Copy link
Contributor

Choose a reason for hiding this comment

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

So the question is: Is there potentially an action that updates either selection or blocks but shouldn't update "automaticChangeStatus"?

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 don't think so

Copy link
Member Author

Choose a reason for hiding this comment

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

So far every time we had this issue it's because of some other non content state

) {
return nextState;
}

// As long as the state is not final, ignore any selection changes.
if (
nextState.automaticChangeStatus !== 'final' &&
nextState.selection !== state.selection
) {
return nextState;
}

// Reset the status if blocks change or selection changes (when status is final).
return {
...nextState,
automaticChangeStatus: undefined,
};
};
}

export default withAutomaticChangeReset( combinedReducers );
3 changes: 3 additions & 0 deletions test/e2e/specs/editor/blocks/list.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ test.describe( 'List (@firefox)', () => {
await page.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '* ' );
await expect( page.locator( '[data-type="core/list"]' ) ).toBeVisible();
// Wait until the automatic change is marked as "final", which is done
// with an idle callback, see __unstableMarkAutomaticChange.
await page.evaluate( () => new Promise( window.requestIdleCallback ) );
Comment on lines +185 to +187
Copy link
Member

Choose a reason for hiding this comment

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

This is not something we want to put in e2e tests, since it's implementation details and not what end users could interact with. It can potentially also be an issue for end users if they press the ArrowUp key fast enough, right? Ideally, we should fix it in the code so that it can be deterministic. If that's not possible at the time then we should think along the line with retrying and waiting for the results, something like this:

await expect( async () => {
	await page.keyboard.press( 'ArrowUp' );
	expect(
		await page.evaluate( () =>
			window.wp.data.select( 'core/block-editor' ).getSelectedBlock()
		)
	).toMatchObject( { name: 'core/list' } );
} ).toPass();

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really an issue for a user: if selection changes too quickly, it's just that it will still be possible to undo the automatic change. We have a few of these lines throughout the e2e test where we have to wait for selection. We also have undo levels for rich text after 1 second of inactivity. How would we write an e2e test for something like that? Let's look at these all together separately.

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'd rather improve test stability for trunk right now.)

Copy link
Member

Choose a reason for hiding this comment

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

Please follow up with a PR to fix this then.

We have a few of these lines throughout the e2e test where we have to wait for selection.

And this is the anti-pattern that I'm recommending against.

We also have undo levels for rich text after 1 second of inactivity. How would we write an e2e test for something like that?

Some of these can be mocked to reduce the time we have to wait. If that's hard-coded then we can always fallback to the pattern I mentioned above: wait for feedback that's accessible to the end users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid mocking entirely in e2e tests if possible otherwise it's not e2e testing anymore. If the user has to wait, let's make the test wait as well.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. But sometimes it's inevitable, like caching and nonce tests, or just isolating for more stable tests. Waiting is okay, we just need to wait for user-visible behaviors instead.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @ellatrix, friendly reminder that this exists ❤️ . Feel free to request my reviews in the PR and we can discuss alternatives there more!

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'm not really sure what you're asking here. I added the best solution to my knowledge in this PR. What does the code above do? It just keeps pressing up until the test passes? To me that doesn't feel like an e2e test?

Copy link
Member

Choose a reason for hiding this comment

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

What to do is up to you, just don't rely on implementation details like requestIdleCallback. This could potentially be very flaky when React changes how it schedule async work for instance. Why doesn't it feel like e2e to you? Retrying and waiting on user-visible interactions sounds exactly like how end users would use the app to me. If retrying is not suitable here, try waiting on user-visible actions (like caret position) before pressing the button.

await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.press( 'Backspace' );
Expand Down