Skip to content

Commit

Permalink
Saving: Open preview window before setting URL (#8330)
Browse files Browse the repository at this point in the history 
* Saving: Remove MouseEvent handling from openPreviewWindow

No longer operating as a link needing default prevented

* Saving: Open preview window before setting URL

* Testing: Attempt to avoid preview navigation race condition

See https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pageclickselector-options
  • Loading branch information
@aduth @gziolo
aduth authored and gziolo committed Aug 1, 2018
1 parent 1ace1c7 commit 69de8bf
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 116 deletions.
42 changes: 17 additions & 25 deletions packages/editor/src/components/post-preview-button/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,22 @@ export class PostPreviewButton extends Component {
// unintentional forceful redirects.
if ( previewLink && ! prevProps.previewLink ) {
this.setPreviewWindowLink( previewLink );

// Once popup redirect is evaluated, even if already closed, delete
// reference to avoid later assignment of location in post update.
delete this.previewWindow;
}
}

/**
* Sets the preview window's location to the given URL, if a preview window
* exists and is not already at that location.
* exists and is not closed.
*
* @param {string} url URL to assign as preview window location.
*/
setPreviewWindowLink( url ) {
const { previewWindow } = this;

// Once popup redirect is evaluated, even if already closed, delete
// reference to avoid later assignment of location in a post update.
delete this.previewWindow;

if ( previewWindow && ! previewWindow.closed ) {
previewWindow.location = url;
}
Expand All @@ -55,37 +55,29 @@ export class PostPreviewButton extends Component {
}

/**
* Handles a click event to open a popup window and prevent default click
* behavior if the post is either autosaveable or has a previously assigned
* preview link to be shown in the popup window target. Triggers autosave
* if post is autosaveable.
*
* @param {MouseEvent} event Click event from preview button click.
* Opens a popup window, navigating user to a preview of the current post.
* Triggers autosave if post is autosaveable.
*/
openPreviewWindow( event ) {
openPreviewWindow() {
const { isAutosaveable, previewLink, currentPostLink } = this.props;

// Open a popup, BUT: Set it to a blank page until save completes. This
// is necessary because popups can only be opened in response to user
// interaction (click), but we must still wait for the post to save.
if ( ! this.previewWindow ) {
this.previewWindow = window.open( '', this.getWindowTarget() );
}

// If there are no changes to autosave, we cannot perform the save, but
// if there is an existing preview link (e.g. previous published post
// autosave), it should be reused as the popup destination.
if ( ! isAutosaveable && ! previewLink && currentPostLink ) {
this.previewWindow = window.open(
currentPostLink,
this.getWindowTarget()
);
this.setPreviewWindowLink( currentPostLink );
return;
}

// Open a popup, BUT: Set it to a blank page until save completes. This
// is necessary because popups can only be opened in response to user
// interaction (click), but we must still wait for the post to save.
event.preventDefault();
this.previewWindow = window.open(
isAutosaveable ? 'about:blank' : previewLink,
this.getWindowTarget()
);

if ( ! isAutosaveable ) {
this.setPreviewWindowLink( previewLink );
return;
}

Expand Down
22 changes: 11 additions & 11 deletions packages/editor/src/components/post-preview-button/test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,13 @@ describe( 'PostPreviewButton', () => {
describe( 'openPreviewWindow()', () => {
function assertForPreview( props, expectedPreviewURL, isExpectingSave ) {
const autosave = jest.fn();
const preventDefault = jest.fn();
const setLocation = jest.fn();
const windowOpen = window.open;
window.open = jest.fn( () => {
return {
set location( url ) {
setLocation( url );
},
document: {
write: jest.fn(),
close: jest.fn(),
Expand All @@ -95,17 +98,14 @@ describe( 'PostPreviewButton', () => {
/>
);

wrapper.simulate( 'click', { preventDefault } );
wrapper.simulate( 'click' );

if ( expectedPreviewURL ) {
if ( expectedPreviewURL !== props.currentPostLink ) {
expect( preventDefault ).toHaveBeenCalled();
}
expect( window.open ).toHaveBeenCalledWith( '', 'wp-preview-1' );

expect( window.open ).toHaveBeenCalledWith( expectedPreviewURL, 'wp-preview-1' );
if ( expectedPreviewURL ) {
expect( setLocation ).toHaveBeenCalledWith( expectedPreviewURL );
} else {
expect( preventDefault ).not.toHaveBeenCalled();
expect( window.open ).not.toHaveBeenCalled();
expect( setLocation ).not.toHaveBeenCalled();
}

window.open = windowOpen;
Expand All @@ -131,14 +131,14 @@ describe( 'PostPreviewButton', () => {
assertForPreview( {
isAutosaveable: true,
previewLink: 'https://wordpress.org/?p=1&preview=true',
}, 'about:blank', true );
}, null, true );
} );

it( 'should save for autosaveable post without preview link', () => {
assertForPreview( {
isAutosaveable: true,
previewLink: undefined,
}, 'about:blank', true );
}, null, true );
} );

it( 'should not save but open a popup window if not autosaveable but preview link available', () => {
Expand Down
104 changes: 24 additions & 80 deletions test/e2e/specs/preview.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,85 +23,23 @@ describe( 'Preview', () => {
await newPost();
} );

let lastPreviewPage;

/**
* Clicks the preview button and returns the generated preview window page,
* either the newly created tab or the redirected existing target. This is
* required because Chromium infuriatingly disregards same target name in
* specific undetermined circumstances, else our efforts to reuse the same
* popup have been fruitless and exhausting. It is worth exploring further,
* perhaps considering factors such as origin of the interstitial page (the
* about:blank placeholder screen), or whether the preview link default
* behavior is used / prevented by the display of the popup window of the
* same target name. Resolves only once the preview page has finished
* loading.
* Given a Puppeteer Page instance for a preview window, clicks Preview and
* awaits the window navigation.
*
* @param {puppeteer.Page} previewPage Page on which to await navigation.
*
* @return {Promise} Promise resolving with focused, loaded preview page.
* @return {Promise} Promise resolving once navigation completes.
*/
async function getOpenedPreviewPage() {
const eventHandlers = [];

page.click( '.editor-post-preview' );

const race = [
new Promise( ( resolve ) => {
async function onBrowserTabOpened( target ) {
const targetPage = await target.page();
resolve( targetPage );
}
browser.once( 'targetcreated', onBrowserTabOpened );
eventHandlers.push( [ browser, 'targetcreated', onBrowserTabOpened ] );
} ),
];

if ( lastPreviewPage ) {
race.push( new Promise( async ( resolve ) => {
function onLastPreviewPageLoaded() {
resolve( lastPreviewPage );
}

lastPreviewPage.once( 'load', onLastPreviewPageLoaded );
eventHandlers.push( [ lastPreviewPage, 'load', onLastPreviewPageLoaded ] );
} ) );
}

// The preview page is whichever of the two resolves first:
// - A new tab has opened.
// - An existing tab is reused and navigates.
const previewPage = await Promise.race( race );

// Since there may be lingering event handlers from whichever of the
// race candidates had lost, remove all handlers.
eventHandlers.forEach( ( [ target, event, handler ] ) => {
target.removeListener( event, handler );
} );

// If a new preview tab is opened and there was a previous one, close
// the previous tab.
if ( lastPreviewPage && lastPreviewPage !== previewPage ) {
await lastPreviewPage.close();
}

lastPreviewPage = previewPage;

// Allow preview to generate if interstitial is visible.
const isGeneratingPreview = await previewPage.evaluate( () => (
!! document.querySelector( '.editor-post-preview-button__interstitial-message' )
) );

if ( isGeneratingPreview ) {
await previewPage.waitForNavigation();
}

await previewPage.bringToFront();

return previewPage;
function waitForPreviewNavigation( previewPage ) {
return Promise.all( [
previewPage.waitForNavigation(),
page.click( '.editor-post-preview' ),
] );
}

it( 'Should open a preview window for a new post', async () => {
const editorPage = page;
let previewPage;

// Disabled until content present.
const isPreviewDisabled = await page.$$eval(
Expand All @@ -112,7 +50,15 @@ describe( 'Preview', () => {

await editorPage.type( '.editor-post-title__input', 'Hello World' );

previewPage = await getOpenedPreviewPage();
await page.click( '.editor-post-preview' );

const previewPage = await new Promise( ( resolve ) => {
async function onBrowserTabOpened( target ) {
const targetPage = await target.page();
resolve( targetPage );
}
browser.once( 'targetcreated', onBrowserTabOpened );
} );

// When autosave completes for a new post, the URL of the editor should
// update to include the ID. Use this to assert on preview URL.
Expand All @@ -130,7 +76,7 @@ describe( 'Preview', () => {
// Return to editor to change title.
await editorPage.bringToFront();
await editorPage.type( '.editor-post-title__input', '!' );
previewPage = await getOpenedPreviewPage();
await waitForPreviewNavigation( previewPage );

// Title in preview should match updated input.
previewTitle = await previewPage.$eval( '.entry-title', ( node ) => node.textContent );
Expand All @@ -139,7 +85,7 @@ describe( 'Preview', () => {
// Pressing preview without changes should bring same preview window to
// front and reload, but should not show interstitial.
await editorPage.bringToFront();
previewPage = await getOpenedPreviewPage();
await waitForPreviewNavigation( previewPage );
previewTitle = await previewPage.$eval( '.entry-title', ( node ) => node.textContent );
expect( previewTitle ).toBe( 'Hello World!' );

Expand All @@ -152,15 +98,13 @@ describe( 'Preview', () => {
page.click( '.editor-post-publish-panel__header button' ),
] );
expectedPreviewURL = await editorPage.$eval( '.notice-success a', ( node ) => node.href );
previewPage = await getOpenedPreviewPage();
await waitForPreviewNavigation( previewPage );
expect( previewPage.url() ).toBe( expectedPreviewURL );

// Return to editor to change title.
await editorPage.bringToFront();
await editorPage.type( '.editor-post-title__input', ' And more.' );

// Published preview should reuse same popup frame.
previewPage = await getOpenedPreviewPage();
await waitForPreviewNavigation( previewPage );

// Title in preview should match updated input.
previewTitle = await previewPage.$eval( '.entry-title', ( node ) => node.textContent );
Expand All @@ -178,7 +122,7 @@ describe( 'Preview', () => {
//
// See: https://github.com/WordPress/gutenberg/issues/7561
await editorPage.bringToFront();
previewPage = await getOpenedPreviewPage();
await waitForPreviewNavigation( previewPage );

// Title in preview should match updated input.
previewTitle = await previewPage.$eval( '.entry-title', ( node ) => node.textContent );
Expand Down

0 comments on commit 69de8bf

Please sign in to comment.