Store: Reset change detection on post update (#6209)

* Store: Preserve change detection isDirty for ignored types

* Store: Reset change detection on post update

* Editor: Refactor dirtiness condition on pending transactions

* Testing: Add pressWithModifier E2E test utility

editor/store/reducer.js

@@ -224,8 +224,8 @@ export const editor = flow( [
 	// Track whether changes exist, resetting at each post save. Relies on
 	// editor initialization firing post reset as an effect.
 	withChangeDetection( {
-		resetTypes: [ 'SETUP_EDITOR_STATE', 'RESET_POST' ],
-		ignoreTypes: [ 'RECEIVE_BLOCKS' ],
+		resetTypes: [ 'SETUP_EDITOR_STATE', 'UPDATE_POST' ],
+		ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST' ],
 	} ),
 ] )( {
 	edits( state = {}, action ) {

editor/store/selectors.js

@@ -89,7 +89,7 @@ export function isEditedPostNew( state ) {
  * @return {boolean} Whether unsaved values exist.
  */
 export function isEditedPostDirty( state ) {
-	return state.editor.isDirty;
+	return state.editor.isDirty || inSomeHistory( state, isEditedPostDirty );
 }
 
 /**
@@ -1568,3 +1568,25 @@ export function getPermalinkParts( state ) {
 		suffix,
 	};
 }
+
+/**
+ * Returns true if an optimistic transaction is pending commit, for which the
+ * before state satisfies the given predicate function.
+ *
+ * @param {Object}   state     Editor state.
+ * @param {Function} predicate Function given state, returning true if match.
+ *
+ * @return {boolean} Whether predicate matches for some history.
+ */
+export function inSomeHistory( state, predicate ) {
+	const { optimist } = state;
+
+	// In recursion, optimist state won't exist. Assume exhausted options.
+	if ( ! optimist ) {
+		return false;
+	}
+
+	return optimist.some( ( { beforeState } ) => (
+		beforeState && predicate( beforeState )
+	) );
+}

editor/store/test/selectors.js

@@ -204,6 +204,28 @@ describe( 'selectors', () => {
 				editor: {
 					isDirty: true,
 				},
+				saving: {
+					requesting: false,
+				},
+			};
+
+			expect( isEditedPostDirty( state ) ).toBe( true );
+		} );
+
+		it( 'should return true if pending transaction with dirty state', () => {
+			const state = {
+				optimist: [
+					{
+						beforeState: {
+							editor: {
+								isDirty: true,
+							},
+						},
+					},
+				],
+				editor: {
+					isDirty: false,
+				},
 			};
 
 			expect( isEditedPostDirty( state ) ).toBe( true );
@@ -214,6 +236,9 @@ describe( 'selectors', () => {
 				editor: {
 					isDirty: false,
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostDirty( state ) ).toBe( false );
@@ -230,6 +255,9 @@ describe( 'selectors', () => {
 					id: 1,
 					status: 'auto-draft',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isCleanNewPost( state ) ).toBe( true );
@@ -244,6 +272,9 @@ describe( 'selectors', () => {
 					id: 1,
 					status: 'draft',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isCleanNewPost( state ) ).toBe( false );
@@ -258,6 +289,9 @@ describe( 'selectors', () => {
 					id: 1,
 					status: 'auto-draft',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isCleanNewPost( state ) ).toBe( false );
@@ -432,6 +466,9 @@ describe( 'selectors', () => {
 					},
 					isDirty: false,
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( getDocumentTitle( state ) ).toBe( 'The Title' );
@@ -450,6 +487,9 @@ describe( 'selectors', () => {
 						},
 					},
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( getDocumentTitle( state ) ).toBe( 'Modified Title' );
@@ -470,6 +510,9 @@ describe( 'selectors', () => {
 					},
 					isDirty: false,
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( getDocumentTitle( state ) ).toBe( __( 'New post' ) );
@@ -490,6 +533,9 @@ describe( 'selectors', () => {
 					},
 					isDirty: true,
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( getDocumentTitle( state ) ).toBe( __( '(Untitled)' ) );
@@ -719,6 +765,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'pending',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( true );
@@ -732,6 +781,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'draft',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( true );
@@ -745,6 +797,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'publish',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( false );
@@ -758,6 +813,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'publish',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( true );
@@ -771,6 +829,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'private',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( false );
@@ -784,6 +845,9 @@ describe( 'selectors', () => {
 				currentPost: {
 					status: 'future',
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( false );
@@ -797,6 +861,9 @@ describe( 'selectors', () => {
 				editor: {
 					isDirty: true,
 				},
+				saving: {
+					requesting: false,
+				},
 			};
 
 			expect( isEditedPostPublishable( state ) ).toBe( true );

editor/utils/with-change-detection/index.js

@@ -16,11 +16,7 @@ import { includes } from 'lodash';
  */
 const withChangeDetection = ( options = {} ) => ( reducer ) => {
 	return ( state, action ) => {
-		const nextState = reducer( state, action );
-
-		if ( includes( options.ignoreTypes, action.type ) ) {
-			return nextState;
-		}
+		let nextState = reducer( state, action );
 
 		// Reset at:
 		//  - Initial state
@@ -30,17 +26,26 @@ const withChangeDetection = ( options = {} ) => ( reducer ) => {
 			includes( options.resetTypes, action.type )
 		);
 
-		if ( isReset ) {
-			return {
-				...nextState,
-				isDirty: false,
-			};
+		const isChanging = state !== nextState;
+
+		// If not intending to update dirty flag, return early and avoid clone.
+		if ( ! isChanging && ! isReset ) {
+			return state;
 		}
 
-		const isChanging = state !== nextState;
+		// Avoid mutating state, unless it's already changing by original
+		// reducer and not initial.
+		if ( ! isChanging || state === undefined ) {
+			nextState = { ...nextState };
+		}
+
+		const isIgnored = includes( options.ignoreTypes, action.type );
 
-		if ( isChanging ) {
-			nextState.isDirty = true;
+		if ( isIgnored ) {
+			// Preserve the original value if ignored.
+			nextState.isDirty = state.isDirty;
+		} else {
+			nextState.isDirty = ! isReset && isChanging;
 		}
 
 		return nextState;

editor/utils/with-change-detection/test/index.js

@@ -14,10 +14,14 @@ describe( 'withChangeDetection()', () => {
 	function originalReducer( state = initialState, action ) {
 		switch ( action.type ) {
 			case 'INCREMENT':
-				return { ...state, count: state.count + 1 };
+				return {
+					count: state.count + 1,
+				};
 
 			case 'RESET_AND_CHANGE_REFERENCE':
-				return { ...state };
+				return {
+					count: state.count,
+				};
 		}
 
 		return state;
@@ -72,8 +76,9 @@ describe( 'withChangeDetection()', () => {
 		state = reducer( deepFreeze( state ), { type: 'INCREMENT' } );
 		expect( state ).toEqual( { count: 1, isDirty: true } );
 
-		state = reducer( deepFreeze( state ), {} );
-		expect( state ).toEqual( { count: 1, isDirty: true } );
+		const afterState = reducer( deepFreeze( state ), {} );
+		expect( afterState ).toEqual( { count: 1, isDirty: true } );
+		expect( afterState ).toBe( state );
 	} );
 
 	it( 'should maintain separate states', () => {

test/e2e/specs/a11y.test.js

@@ -2,7 +2,7 @@
  * Internal dependencies
  */
 import '../support/bootstrap';
-import { newPost, newDesktopBrowserPage } from '../support/utils';
+import { newPost, newDesktopBrowserPage, pressWithModifier } from '../support/utils';
 
 describe( 'a11y', () => {
 	beforeAll( async () => {
@@ -11,9 +11,7 @@ describe( 'a11y', () => {
 	} );
 
 	it( 'tabs header bar', async () => {
-		await page.keyboard.down( 'Control' );
-		await page.keyboard.press( '~' );
-		await page.keyboard.up( 'Control' );
+		await pressWithModifier( 'Control', '~' );
 
 		await page.keyboard.press( 'Tab' );