From 474993f1a3bcddad1b94d073e559ba52d4d2f357 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 25 Apr 2018 15:38:58 -0400 Subject: [PATCH 1/4] Testing: Add pressWithModifier E2E test utility --- test/e2e/specs/a11y.test.js | 6 +-- test/e2e/specs/multi-block-selection.test.js | 6 +-- test/e2e/support/utils.js | 41 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/test/e2e/specs/a11y.test.js b/test/e2e/specs/a11y.test.js index c90224fadd09d..fe0c948be2a5e 100644 --- a/test/e2e/specs/a11y.test.js +++ b/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' ); diff --git a/test/e2e/specs/multi-block-selection.test.js b/test/e2e/specs/multi-block-selection.test.js index 09142e94846b0..064540406b4f1 100644 --- a/test/e2e/specs/multi-block-selection.test.js +++ b/test/e2e/specs/multi-block-selection.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( 'Multi-block selection', () => { beforeAll( async () => { @@ -62,9 +62,7 @@ describe( 'Multi-block selection', () => { // Multiselect via keyboard await page.click( 'body' ); - await page.keyboard.down( 'Meta' ); - await page.keyboard.press( 'a' ); - await page.keyboard.up( 'Meta' ); + await pressWithModifier( 'Mod', 'a' ); // Verify selection expectMultiSelected( blocks, true ); diff --git a/test/e2e/support/utils.js b/test/e2e/support/utils.js index ae4050789f64e..1f6f0950323aa 100644 --- a/test/e2e/support/utils.js +++ b/test/e2e/support/utils.js @@ -10,6 +10,15 @@ const { WP_PASSWORD = 'password', } = process.env; +/** + * Platform-specific modifier key. + * + * @see pressWithModifier + * + * @type {string} + */ +const MOD_KEY = process.platform === 'darwin' ? 'Meta' : 'Control'; + function getUrl( WPPath, query = '' ) { const url = new URL( WP_BASE_URL ); @@ -58,3 +67,35 @@ export async function newDesktopBrowserPage() { global.page = await browser.newPage(); await page.setViewport( { width: 1000, height: 700 } ); } + +export async function switchToEditor( mode ) { + await page.click( '.edit-post-more-menu [aria-label="More"]' ); + const [ button ] = await page.$x( `//button[contains(text(), \'${ mode } Editor\')]` ); + await button.click( 'button' ); +} + +export async function getHTMLFromCodeEditor() { + await switchToEditor( 'Code' ); + const textEditorContent = await page.$eval( '.editor-post-text-editor', ( element ) => element.value ); + await switchToEditor( 'Visual' ); + return textEditorContent; +} + +/** + * Performs a key press with modifier (Shift, Control, Meta, Mod), where "Mod" + * is normalized to platform-specific modifier (Meta in MacOS, else Control). + * + * @param {string} modifier Modifier key. + * @param {string} key Key to press while modifier held. + * + * @return {Promise} Promise resolving when key combination pressed. + */ +export async function pressWithModifier( modifier, key ) { + if ( modifier.toLowerCase() === 'mod' ) { + modifier = MOD_KEY; + } + + await page.keyboard.down( modifier ); + await page.keyboard.press( key ); + return page.keyboard.up( modifier ); +} From dd9111f5a5fd00726e0b41de698dd365587390d0 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 16 Apr 2018 10:08:41 -0400 Subject: [PATCH 2/4] Store: Preserve change detection isDirty for ignored types --- editor/utils/with-change-detection/index.js | 31 +++++++++++-------- .../utils/with-change-detection/test/index.js | 13 +++++--- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/editor/utils/with-change-detection/index.js b/editor/utils/with-change-detection/index.js index 8b738e50015d6..e86db9a9905d4 100644 --- a/editor/utils/with-change-detection/index.js +++ b/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; diff --git a/editor/utils/with-change-detection/test/index.js b/editor/utils/with-change-detection/test/index.js index 728ef8b22a550..06abccb50fc5b 100644 --- a/editor/utils/with-change-detection/test/index.js +++ b/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', () => { From 59d60e6156faeb9dcdcfc3456fcc51a8be68e830 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 16 Apr 2018 10:09:12 -0400 Subject: [PATCH 3/4] Store: Reset change detection on post update --- editor/store/reducer.js | 4 +- editor/store/selectors.js | 2 +- editor/store/test/selectors.js | 61 ++++++++ test/e2e/specs/change-detection.test.js | 179 ++++++++++++++++++++++++ test/e2e/specs/hello.test.js | 7 +- 5 files changed, 244 insertions(+), 9 deletions(-) create mode 100644 test/e2e/specs/change-detection.test.js diff --git a/editor/store/reducer.js b/editor/store/reducer.js index 6d981f278de58..ca558159fa15e 100644 --- a/editor/store/reducer.js +++ b/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 ) { diff --git a/editor/store/selectors.js b/editor/store/selectors.js index 0d10c2106d396..e158d6d7f3886 100644 --- a/editor/store/selectors.js +++ b/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 || isSavingPost( state ); } /** diff --git a/editor/store/test/selectors.js b/editor/store/test/selectors.js index 53aa2a2bcfd59..cca6e2fa7033f 100644 --- a/editor/store/test/selectors.js +++ b/editor/store/test/selectors.js @@ -204,6 +204,22 @@ describe( 'selectors', () => { editor: { isDirty: true, }, + saving: { + requesting: false, + }, + }; + + expect( isEditedPostDirty( state ) ).toBe( true ); + } ); + + it( 'should return true if mid-save', () => { + const state = { + editor: { + isDirty: false, + }, + saving: { + requesting: true, + }, }; expect( isEditedPostDirty( state ) ).toBe( true ); @@ -214,6 +230,9 @@ describe( 'selectors', () => { editor: { isDirty: false, }, + saving: { + requesting: false, + }, }; expect( isEditedPostDirty( state ) ).toBe( false ); @@ -230,6 +249,9 @@ describe( 'selectors', () => { id: 1, status: 'auto-draft', }, + saving: { + requesting: false, + }, }; expect( isCleanNewPost( state ) ).toBe( true ); @@ -244,6 +266,9 @@ describe( 'selectors', () => { id: 1, status: 'draft', }, + saving: { + requesting: false, + }, }; expect( isCleanNewPost( state ) ).toBe( false ); @@ -258,6 +283,9 @@ describe( 'selectors', () => { id: 1, status: 'auto-draft', }, + saving: { + requesting: false, + }, }; expect( isCleanNewPost( state ) ).toBe( false ); @@ -432,6 +460,9 @@ describe( 'selectors', () => { }, isDirty: false, }, + saving: { + requesting: false, + }, }; expect( getDocumentTitle( state ) ).toBe( 'The Title' ); @@ -450,6 +481,9 @@ describe( 'selectors', () => { }, }, }, + saving: { + requesting: false, + }, }; expect( getDocumentTitle( state ) ).toBe( 'Modified Title' ); @@ -470,6 +504,9 @@ describe( 'selectors', () => { }, isDirty: false, }, + saving: { + requesting: false, + }, }; expect( getDocumentTitle( state ) ).toBe( __( 'New post' ) ); @@ -490,6 +527,9 @@ describe( 'selectors', () => { }, isDirty: true, }, + saving: { + requesting: false, + }, }; expect( getDocumentTitle( state ) ).toBe( __( '(Untitled)' ) ); @@ -719,6 +759,9 @@ describe( 'selectors', () => { currentPost: { status: 'pending', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( true ); @@ -732,6 +775,9 @@ describe( 'selectors', () => { currentPost: { status: 'draft', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( true ); @@ -745,6 +791,9 @@ describe( 'selectors', () => { currentPost: { status: 'publish', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( false ); @@ -758,6 +807,9 @@ describe( 'selectors', () => { currentPost: { status: 'publish', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( true ); @@ -771,6 +823,9 @@ describe( 'selectors', () => { currentPost: { status: 'private', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( false ); @@ -784,6 +839,9 @@ describe( 'selectors', () => { currentPost: { status: 'future', }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( false ); @@ -797,6 +855,9 @@ describe( 'selectors', () => { editor: { isDirty: true, }, + saving: { + requesting: false, + }, }; expect( isEditedPostPublishable( state ) ).toBe( true ); diff --git a/test/e2e/specs/change-detection.test.js b/test/e2e/specs/change-detection.test.js new file mode 100644 index 0000000000000..a3c08a51cb784 --- /dev/null +++ b/test/e2e/specs/change-detection.test.js @@ -0,0 +1,179 @@ +/** + * Internal dependencies + */ +import '../support/bootstrap'; +import { newPost, newDesktopBrowserPage, pressWithModifier } from '../support/utils'; + +describe( 'Change detection', () => { + let handleInterceptedRequest; + + beforeAll( async () => { + await newDesktopBrowserPage(); + } ); + + beforeEach( async () => { + await newPost(); + } ); + + async function assertIsDirty( isDirty ) { + let hadDialog = false; + + function handleOnDialog( dialog ) { + dialog.accept(); + hadDialog = true; + } + + try { + page.on( 'dialog', handleOnDialog ); + await page.reload(); + + // Ensure whether it was expected that dialog was encountered. + expect( hadDialog ).toBe( isDirty ); + } catch ( error ) { + throw error; + } finally { + page.removeListener( 'dialog', handleOnDialog ); + } + } + + async function interceptSave() { + await page.setRequestInterception( true ); + + handleInterceptedRequest = ( interceptedRequest ) => { + if ( ! interceptedRequest.url().includes( '/wp/v2/posts' ) ) { + interceptedRequest.continue(); + } + }; + page.on( 'request', handleInterceptedRequest ); + } + + async function releaseSaveIntercept() { + page.removeListener( 'request', handleInterceptedRequest ); + await page.setRequestInterception( false ); + } + + it( 'Should not prompt to confirm unsaved changes', async () => { + await assertIsDirty( false ); + } ); + + it( 'Should prompt if property changed without save', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + await assertIsDirty( true ); + } ); + + it( 'Should prompt if content added without save', async () => { + await page.click( '.editor-default-block-appender' ); + + await assertIsDirty( true ); + } ); + + it( 'Should not prompt if changes saved', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + await Promise.all( [ + // Wait for "Saved" to confirm save complete. + page.waitForSelector( '.editor-post-saved-state.is-saved' ), + + // Keyboard shortcut Ctrl+S save. + pressWithModifier( 'Mod', 'S' ), + ] ); + + await assertIsDirty( false ); + } ); + + it( 'Should prompt if save failed', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + await page.setOfflineMode( true ); + + // Keyboard shortcut Ctrl+S save. + await pressWithModifier( 'Mod', 'S' ); + + // Ensure save update fails and presents button. + await page.waitForXPath( '//p[contains(text(), \'Updating failed\')]' ); + await page.waitForSelector( '.editor-post-save-draft' ); + + // Need to disable offline to allow reload. + await page.setOfflineMode( false ); + + await assertIsDirty( true ); + } ); + + it( 'Should prompt if changes and save is in-flight', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + // Hold the posts request so we don't deal with race conditions of the + // save completing early. Other requests should be allowed to continue, + // for example the page reload test. + await interceptSave(); + + // Keyboard shortcut Ctrl+S save. + await pressWithModifier( 'Mod', 'S' ); + + await releaseSaveIntercept(); + + await assertIsDirty( true ); + } ); + + it( 'Should prompt if changes made while save is in-flight', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + // Hold the posts request so we don't deal with race conditions of the + // save completing early. Other requests should be allowed to continue, + // for example the page reload test. + await interceptSave(); + + // Keyboard shortcut Ctrl+S save. + await pressWithModifier( 'Mod', 'S' ); + + await page.type( '.editor-post-title__input', '!' ); + + await releaseSaveIntercept(); + + await assertIsDirty( true ); + } ); + + it( 'Should prompt if property changes made while save is in-flight, and save completes', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + // Hold the posts request so we don't deal with race conditions of the + // save completing early. + await interceptSave(); + + // Keyboard shortcut Ctrl+S save. + await pressWithModifier( 'Mod', 'S' ); + + // Dirty post while save is in-flight. + await page.type( '.editor-post-title__input', '!' ); + + // Allow save to complete. Disabling interception flushes pending. + await Promise.all( [ + page.waitForSelector( '.editor-post-saved-state.is-saved' ), + releaseSaveIntercept(), + ] ); + + await assertIsDirty( true ); + } ); + + it( 'Should prompt if block revision is made while save is in-flight, and save completes', async () => { + await page.type( '.editor-post-title__input', 'Hello World' ); + + // Hold the posts request so we don't deal with race conditions of the + // save completing early. + await interceptSave(); + + // Keyboard shortcut Ctrl+S save. + await pressWithModifier( 'Mod', 'S' ); + + await page.click( '.editor-default-block-appender' ); + + // Allow save to complete. Disabling interception flushes pending. + await Promise.all( [ + page.waitForSelector( '.editor-post-saved-state.is-saved' ), + releaseSaveIntercept(), + ] ); + + await assertIsDirty( true ); + } ); +} ); diff --git a/test/e2e/specs/hello.test.js b/test/e2e/specs/hello.test.js index 4a9fcfe8554c7..ff6653e7d3e0f 100644 --- a/test/e2e/specs/hello.test.js +++ b/test/e2e/specs/hello.test.js @@ -2,7 +2,7 @@ * Internal dependencies */ import '../support/bootstrap'; -import { newPost, visitAdmin, newDesktopBrowserPage } from '../support/utils'; +import { newPost, newDesktopBrowserPage } from '../support/utils'; describe( 'hello', () => { beforeAll( async () => { @@ -25,9 +25,4 @@ describe( 'hello', () => { expect( undoButton ).toBeNull(); expect( redoButton ).toBeNull(); } ); - - it( 'Should not prompt to confirm unsaved changes', async () => { - await visitAdmin( 'edit.php' ); - expect( page.url() ).not.toEqual( expect.stringContaining( 'post-new.php' ) ); - } ); } ); From 3a2e39fab9b5ce3942a65844379df57416fad07a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 25 Apr 2018 15:29:38 -0400 Subject: [PATCH 4/4] Editor: Refactor dirtiness condition on pending transactions --- editor/store/selectors.js | 24 +++++++++++++++++++++++- editor/store/test/selectors.js | 14 ++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/editor/store/selectors.js b/editor/store/selectors.js index e158d6d7f3886..4d620e08ca95f 100644 --- a/editor/store/selectors.js +++ b/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 || isSavingPost( state ); + 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 ) + ) ); +} diff --git a/editor/store/test/selectors.js b/editor/store/test/selectors.js index cca6e2fa7033f..df5d72efadb78 100644 --- a/editor/store/test/selectors.js +++ b/editor/store/test/selectors.js @@ -212,14 +212,20 @@ describe( 'selectors', () => { expect( isEditedPostDirty( state ) ).toBe( true ); } ); - it( 'should return true if mid-save', () => { + it( 'should return true if pending transaction with dirty state', () => { const state = { + optimist: [ + { + beforeState: { + editor: { + isDirty: true, + }, + }, + }, + ], editor: { isDirty: false, }, - saving: { - requesting: true, - }, }; expect( isEditedPostDirty( state ) ).toBe( true );