Editor: Reshape editor state for optimized and accurate dirtiness det…

…ection (#10844)

* Editor: Move dirty flag to blocks substate

* Editor: Optimize autosaveable condition

* Editor: Remove redundant artificial dirtying

docs/data/data-core-editor.md

@@ -36,6 +36,18 @@ the post has been saved.
 
 Whether the post is new.
 
+### hasChangedContent
+
+Returns true if content includes unsaved changes, or false otherwise.
+
+*Parameters*
+
+ * state: Editor state.
+
+*Returns*
+
+Whether content includes unsaved changes.
+
 ### isEditedPostDirty
 
 Returns true if there are unsaved values for the current edit session, or

packages/editor/src/store/effects/posts.js

@@ -171,18 +171,8 @@ export const requestPostUpdate = async ( action, store ) => {
  * @param {Object} action  action object.
  * @param {Object} store   Redux Store.
  */
-export const requestPostUpdateSuccess = ( action, store ) => {
+export const requestPostUpdateSuccess = ( action ) => {
 	const { previousPost, post, isAutosave, postType } = action;
-	const { dispatch, getState } = store;
-
-	// TEMPORARY: If edits remain after a save completes, the user must be
-	// prompted about unsaved changes. This should be refactored as part of
-	// the `isEditedPostDirty` selector instead.
-	//
-	// See: https://github.com/WordPress/gutenberg/issues/7409
-	if ( Object.keys( getPostEdits( getState() ) ).length ) {
-		dispatch( { type: 'DIRTY_ARTIFICIALLY' } );
-	}
 
 	// Autosaves are neither shown a notice nor redirected.
 	if ( isAutosave ) {

packages/editor/src/store/reducer.js

@@ -236,13 +236,6 @@ export const editor = flow( [
 		ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
 		shouldOverwriteState,
 	} ),
-
-	// Track whether changes exist, resetting at each post save. Relies on
-	// editor initialization firing post reset as an effect.
-	withChangeDetection( {
-		resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
-		ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
-	} ),
 ] )( {
 	edits( state = {}, action ) {
 		switch ( action.type ) {
@@ -264,9 +257,6 @@ export const editor = flow( [
 
 				return state;
 
-			case 'DIRTY_ARTIFICIALLY':
-				return { ...state };
-
 			case 'UPDATE_POST':
 			case 'RESET_POST':
 				const getCanonicalValue = action.type === 'UPDATE_POST' ?
@@ -287,7 +277,16 @@ export const editor = flow( [
 		return state;
 	},
 
-	blocks: combineReducers( {
+	blocks: flow( [
+		combineReducers,
+
+		// Track whether changes exist, resetting at each post save. Relies on
+		// editor initialization firing post reset as an effect.
+		withChangeDetection( {
+			resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
+			ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
+		} ),
+	] )( {
 		byClientId( state = {}, action ) {
 			switch ( action.type ) {
 				case 'RESET_BLOCKS':

packages/editor/src/store/selectors.js

@@ -102,6 +102,26 @@ export function isEditedPostNew( state ) {
 	return getCurrentPost( state ).status === 'auto-draft';
 }
 
+/**
+ * Returns true if content includes unsaved changes, or false otherwise.
+ *
+ * @param {Object} state Editor state.
+ *
+ * @return {boolean} Whether content includes unsaved changes.
+ */
+export function hasChangedContent( state ) {
+	return (
+		state.editor.present.blocks.isDirty ||
+
+		// `edits` is intended to contain only values which are different from
+		// the saved post, so the mere presence of a property is an indicator
+		// that the value is different than what is known to be saved. While
+		// content in Visual mode is represented by the blocks state, in Text
+		// mode it is tracked by `edits.content`.
+		'content' in state.editor.present.edits
+	);
+}
+
 /**
  * Returns true if there are unsaved values for the current edit session, or
  * false if the editing state matches the saved or new post.
@@ -111,7 +131,23 @@ export function isEditedPostNew( state ) {
  * @return {boolean} Whether unsaved values exist.
  */
 export function isEditedPostDirty( state ) {
-	return state.editor.isDirty || inSomeHistory( state, isEditedPostDirty );
+	if ( hasChangedContent( state ) ) {
+		return true;
+	}
+
+	// Edits should contain only fields which differ from the saved post (reset
+	// at initial load and save complete). Thus, a non-empty edits state can be
+	// inferred to contain unsaved values.
+	if ( Object.keys( state.editor.present.edits ).length > 0 ) {
+		return true;
+	}
+
+	// Edits and change detectiona are reset at the start of a save, but a post
+	// is still considered dirty until the point at which the save completes.
+	// Because the save is performed optimistically, the prior states are held
+	// until committed. These can be referenced to determine whether there's a
+	// chance that state may be reverted into one considered dirty.
+	return inSomeHistory( state, isEditedPostDirty );
 }
 
 /**
@@ -451,9 +487,17 @@ export function isEditedPostAutosaveable( state ) {
 		return true;
 	}
 
+	// To avoid an expensive content serialization, use the content dirtiness
+	// flag in place of content field comparison against the known autosave.
+	// This is not strictly accurate, and relies on a tolerance toward autosave
+	// request failures for unnecessary saves.
+	if ( hasChangedContent( state ) ) {
+		return true;
+	}
+
 	// If the title, excerpt or content has changed, the post is autosaveable.
 	const autosave = getAutosave( state );
-	return [ 'title', 'excerpt', 'content' ].some( ( field ) => (
+	return [ 'title', 'excerpt' ].some( ( field ) => (
 		autosave[ field ] !== getEditedPostAttribute( state, field )
 	) );
 }

packages/editor/src/store/test/effects.js

@@ -25,7 +25,6 @@ import actions, {
 	resetBlocks,
 	selectBlock,
 	setTemplateValidity,
-	editPost,
 } from '../actions';
 import effects, { validateBlocksToTemplate } from '../effects';
 import { SAVE_POST_NOTICE_ID } from '../effects/posts';
@@ -222,16 +221,6 @@ describe( 'effects', () => {
 	describe( '.REQUEST_POST_UPDATE_SUCCESS', () => {
 		const handler = effects.REQUEST_POST_UPDATE_SUCCESS;
 
-		function createGetState( hasLingeringEdits = false ) {
-			let state = reducer( undefined, {} );
-			if ( hasLingeringEdits ) {
-				state = reducer( state, editPost( { edited: true } ) );
-			}
-
-			const getState = () => state;
-			return getState;
-		}
-
 		const defaultPost = {
 			id: 1,
 			title: {
@@ -259,14 +248,11 @@ describe( 'effects', () => {
 		} );
 
 		it( 'should dispatch notices when publishing or scheduling a post', () => {
-			const dispatch = jest.fn();
-			const store = { dispatch, getState: createGetState() };
-
 			const previousPost = getDraftPost();
 			const post = getPublishedPost();
 			const postType = getPostType();
 
-			handler( { post, previousPost, postType }, store );
+			handler( { post, previousPost, postType } );
 
 			expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
 				'Post published.',
@@ -280,14 +266,11 @@ describe( 'effects', () => {
 		} );
 
 		it( 'should dispatch notices when reverting a published post to a draft', () => {
-			const dispatch = jest.fn();
-			const store = { dispatch, getState: createGetState() };
-
 			const previousPost = getPublishedPost();
 			const post = getDraftPost();
 			const postType = getPostType();
 
-			handler( { post, previousPost, postType }, store );
+			handler( { post, previousPost, postType } );
 
 			expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
 				'Post reverted to draft.',
@@ -299,14 +282,11 @@ describe( 'effects', () => {
 		} );
 
 		it( 'should dispatch notices when just updating a published post again', () => {
-			const dispatch = jest.fn();
-			const store = { dispatch, getState: createGetState() };
-
 			const previousPost = getPublishedPost();
 			const post = getPublishedPost();
 			const postType = getPostType();
 
-			handler( { post, previousPost, postType }, store );
+			handler( { post, previousPost, postType } );
 
 			expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
 				'Post updated.',
@@ -320,29 +300,13 @@ describe( 'effects', () => {
 		} );
 
 		it( 'should do nothing if the updated post was autosaved', () => {
-			const dispatch = jest.fn();
-			const store = { dispatch, getState: createGetState() };
-
 			const previousPost = getPublishedPost();
 			const post = { ...getPublishedPost(), id: defaultPost.id + 1 };
 
-			handler( { post, previousPost, isAutosave: true }, store );
+			handler( { post, previousPost, isAutosave: true } );
 
 			expect( dataDispatch( 'core/notices' ).createSuccessNotice ).not.toHaveBeenCalled();
 		} );
-
-		it( 'should dispatch dirtying action if edits linger after autosave', () => {
-			const dispatch = jest.fn();
-			const store = { dispatch, getState: createGetState( true ) };
-
-			const previousPost = getPublishedPost();
-			const post = { ...getPublishedPost(), id: defaultPost.id + 1 };
-
-			handler( { post, previousPost, isAutosave: true }, store );
-
-			expect( dispatch ).toHaveBeenCalledTimes( 1 );
-			expect( dispatch ).toHaveBeenCalledWith( { type: 'DIRTY_ARTIFICIALLY' } );
-		} );
 	} );
 
 	describe( '.REQUEST_POST_UPDATE_FAILURE', () => {

packages/editor/src/store/test/reducer.js

@@ -279,15 +279,15 @@ describe( 'state', () => {
 			unregisterBlockType( 'core/test-block' );
 		} );
 
-		it( 'should return history (empty edits, blocks), dirty flag by default', () => {
+		it( 'should return history (empty edits, blocks) by default', () => {
 			const state = editor( undefined, {} );
 
 			expect( state.past ).toEqual( [] );
 			expect( state.future ).toEqual( [] );
 			expect( state.present.edits ).toEqual( {} );
 			expect( state.present.blocks.byClientId ).toEqual( {} );
 			expect( state.present.blocks.order ).toEqual( {} );
-			expect( state.isDirty ).toBe( false );
+			expect( state.present.blocks.isDirty ).toBe( false );
 		} );
 
 		it( 'should key by reset blocks clientId', () => {