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.

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

Preview: Open preview to previewLink if not autosaveable #7703

Merged
merged 4 commits into from Jul 6, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Jul 4, 2018

Closes #7561

This pull request seeks to address set of issues related to the hanging preview interstitial "Please wait..." screen. The specific issue is caused by the fact that an autosave is attempted and then aborted by the corresponding effect because it is not autosaveable. Particularly, this can occur with the artificial dirtying of a published post autosave; the post is technically dirty, but cannot be autosaved since the fields are identical to the last autosave.

It also incidentally resolves an issue where the preview window can occasionally prompt the user about unsaved changes when being closed. This was a very subtle bug in the following line of code:

this.previewWindow.onbeforeunload = () => delete this.previewWindow;

...a combination of:

  • Implicit return of the concise arrow function body
  • delete operator returning true on a successful deletion
  • An onbeforeunload handler returning true having the impact of a prompt being shown
    • When this event returns (or sets the returnValue property to) a value other than null or undefined, the user is prompted to confirm the page unload.

There is one remaining buggy case here:

When reloading a post which has an autosave, if the user immediately presses Preview, the preview window will hang. This is due to the fact that our state is unaware of the presence of an existing autosave, and therefore assumes that autosave is valid, even if no changes are made.

// If we don't already have an autosave, the post is autosaveable.
if ( ! hasAutosave( state ) ) {
return true;
}

I am classifying this as falling under the scope of #7416.

Testing instructions:

Verify that pressing "Preview" loads the accurate preview of the post, with no hanging "Please wait..." screen. Try a variety of conditions:

  • Pressing while already having saved
  • Pressing with and without a preview tab already being opened
  • Published posts and draft posts
  • Before and after the default autosave behavior

Ensure end-to-end tests pass:

npm run test-e2e
@tofumatt

The code here makes sense but it looks like a few tests are failing. For me, locally, three PostPreviewButton tests are failing:

Summary of all failing tests
 FAIL  editor/components/post-preview-button/test/index.js
  ● PostPreviewButton › setPreviewWindowLink() › should do nothing if the preview window is already at url location

    expect(jest.fn()).not.toHaveBeenCalled()

    Expected mock function not to be called but it was called with:
      ["https://wordpress.org"]

      38 | 			wrapper.instance().setPreviewWindowLink( url );
      39 |
    > 40 | 			expect( setter ).not.toHaveBeenCalled();
      41 | 		} );
      42 |
      43 | 		it( 'set preview window location to url', () => {

      at Object.<anonymous> (editor/components/post-preview-button/test/index.js:40:25)

  ● PostPreviewButton › saveForPreview() › should save if not dirty for new post

    expect(jest.fn()).toHaveBeenCalled()

    Expected mock function to have been called.

      112 |
      113 | 			if ( isExpectingSave ) {
    > 114 | 				expect( autosave ).toHaveBeenCalled();
      115 | 				expect( preventDefault ).toHaveBeenCalled();
      116 | 				expect( window.open ).toHaveBeenCalled();
      117 | 				expect( wrapper.instance().previewWindow.document.write ).toHaveBeenCalled();

      at assertForSave (editor/components/post-preview-button/test/index.js:114:24)
      at Object.<anonymous> (editor/components/post-preview-button/test/index.js:137:4)

  ● PostPreviewButton › saveForPreview() › should open a popup window

    expect(jest.fn()).toHaveBeenCalled()

    Expected mock function to have been called.

      112 |
      113 | 			if ( isExpectingSave ) {
    > 114 | 				expect( autosave ).toHaveBeenCalled();
      115 | 				expect( preventDefault ).toHaveBeenCalled();
      116 | 				expect( window.open ).toHaveBeenCalled();
      117 | 				expect( wrapper.instance().previewWindow.document.write ).toHaveBeenCalled();

      at assertForSave (editor/components/post-preview-button/test/index.js:114:24)
      at Object.<anonymous> (editor/components/post-preview-button/test/index.js:146:4)


Test Suites: 1 failed, 2 skipped, 196 passed, 197 of 199 total
Tests:       3 failed, 7 skipped, 1773 passed, 1783 total
Snapshots:   64 passed, 64 total
Time:        36.748s
Ran all test suites.

and one e2e test:

 FAIL  test/e2e/specs/preview.test.js
  Preview
    ✕ Should open a preview window for a new post (2612ms)

  ● Preview › Should open a preview window for a new post

    Error: failed to find element matching selector ".entry-title"

      at Frame.$eval (node_modules/puppeteer/lib/FrameManager.js:344:13)
/**
* Clicks the preview button and returns the generated preview window page,
* either the newly created tab or the redirected existing target. This is

This comment has been minimized.

@tofumatt

tofumatt Jul 4, 2018

Member

Haha, this is definitely one of those "I am frustrated" comment blocks 😄

) ) );
}
const [ , previewPage ] = await Promise.all( [

This comment has been minimized.

@tofumatt

tofumatt Jul 4, 2018

Member

Huh, I've never seen that before! TIL

// 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(
'about:blank',
previewLink ? previewLink : 'about:blank',

This comment has been minimized.

@tofumatt

tofumatt Jul 4, 2018

Member

In this case, previewLink || 'about:blank' might be a bit nicer than the ternary, but it's just a style thing really.

This comment has been minimized.

@aduth

aduth Jul 5, 2018

Member

In this case, previewLink || 'about:blank' might be a bit nicer than the ternary, but it's just a style thing really.

I'd agree, though to the general point this sort of clever use of || and && can sometimes bite when people think only of the fallback case, but not the fall...-forward (?) case, i.e. not anticipating the evaluation to the falsey value as actually being the assignment.

// false || null
null

// 0 && null
0

The latter one is a common pain-point in React where people have the tendency to do things like:

array.length && (
    <ul>{ /* ... */ }</ul>
)

...not anticipating that when the array is empty, there will be the 0 as output.

Demo: https://codepen.io/aduth/pen/dWwzrL

This comment has been minimized.

@tofumatt

tofumatt Jul 5, 2018

Member

Heh, true I guess. I wouldn't ever think to use array.length && ( […] ) but if others do: fair enough!

@gziolo gziolo added this to the 3.2 milestone Jul 5, 2018

@@ -38,11 +38,9 @@ export class PostPreviewButton extends Component {
*/
setPreviewWindowLink( url ) {
const { previewWindow } = this;
if ( ! previewWindow || previewWindow.location.href === url ) {

This comment has been minimized.

@aduth

aduth Jul 5, 2018

Member

Note: I found that previewWindow.location.href was not returning the value I expected: For the new window, it would return the URL of the editor tab, not the window. (Chrome 67.0.3396.99)

aduth added some commits Jul 5, 2018

Saving: Update preview button test for interstitial with preview link
Previously we would allow the previous link to be reused; now we force to about:blank to write the empty interstitial message

@tofumatt tofumatt self-requested a review Jul 6, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 6, 2018

In Firefox, if I:

  1. open an existing post with an autosave present
  2. make a quick change
  3. click "Preview" before autosave happens

I get this.previewWindow is undefined in the console and:

2018-07-06 15 53 07

forever.

Is that outside the scope of this bug?

@aduth

This comment has been minimized.

Member

aduth commented Jul 6, 2018

@tofumatt The general "autosave exists at the beginning of post" is still problematic after this, yes, mentioned also in the original comment:

There is one remaining buggy case here:

When reloading a post which has an autosave, if the user immediately presses Preview, the preview window will hang. This is due to the fact that our state is unaware of the presence of an existing autosave, and therefore assumes that autosave is valid, even if no changes are made.

Seems like it may be the same for you. There's more to be done here with accounting for the initial autosave, which is why I was more inclined to save it for #7416.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 6, 2018

Oh, actually, seems like it always happens for the first click on Preview (when first opening the edit page screen and also happens again after making changes), but if I click a second time the preview loads:

2018-07-06 15 55 44

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 6, 2018

Okay, sounds good. The tests are passing for me now, just running a bit more locally but looking good. Should be another few minutes.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 6, 2018

Loading a published post with no autosaves hangs for me:

2018-07-06 16 04 43

TypeError: this.previewWindow is undefined[Learn More] index.js:114:2
	openPreviewWindow index.js:114:2
	openPreviewWindow self-hosted:986:17
	Sh http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:143:77
	invokeGuardedCallback http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:143:276
	invokeGuardedCallbackAndCatchFirstError http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:143:367
	Wd http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:14:337
	Bg http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:145:478
	Ug http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:146:164
	Va http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:15:123
	yc http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:16:208
	Yd http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:16:373
	rh http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:40:250
	ug http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:136:357
	ue http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:25:80
	Nb http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:41:283
	wg http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:137:152
	Ze http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:41:24
	Ze self-hosted:1030:17
@tofumatt

My opinion is… if you think this is better (I like the tests) then I’m fine with it.

It's still pretty buggy, but I don't think it's worse. 🤷‍♂️

If you think we should punt to 3.3, that's fine. If you think merging is better, do so.

@aduth

This comment has been minimized.

Member

aduth commented Jul 6, 2018

Debugging a bit, I think the behavior in Firefox may be slightly different than anticipated, where document.write is triggering the unload event which incurs the delete this.previewWindow, thus not allowing the redirect to take place. This also existed in master, so I expect similar problems may exist there.

Saving: Avoid unsetting previewWindow on unload event
Unload can be triggered in Firefox merely by the writing to the interstitial window. Only delete once a redirect has been evaluated (and not closed).
@aduth

This comment has been minimized.

Member

aduth commented Jul 6, 2018

@tofumatt Based on the Firefox behavior, I don't know that we can rely on the beforeunload event as much as we were. I pushed some changes in ccd822a which should make the experience a bit better in Firefox. Could you give it another try?

@tofumatt

Tested new version in Firefox and it's much better. No more errors in the console and previews for new posts with no autosaves work.

@aduth

This comment has been minimized.

Member

aduth commented Jul 6, 2018

Gotta fix some unit tests first for the new changes, will merge after updates pass Travis.

Saving: Compare preview location on reference window object
previewWindow is deleted from the instance once props are set, but reference object location is mutated directly

@aduth aduth merged commit e408fa7 into master Jul 6, 2018

2 checks passed

codecov/project 48.46% (+2.04%) compared to dd6d2c2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/post-preview-published-autosave branch Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment