Saving: Open preview window before setting URL

[aduth]

Fixes #8318
Fixes #6956

This pull request seeks to resolve an issue where previewing can cause a new tab to be created, even when an existing preview tab exists to be reused. This also enables the related end-to-end test to be simplified to avoid needing to accommodate both cases where a tab is reused or a new one is created. It is hoped that this simplification will remedy intermittent failures in the end-to-end test suite.

Implementation notes:

If I'm being honest, I'm not entirely clear what it is about this specific flow of logic that resolves the issue. It may be one of:

• Using '' empty string instead of about:blank avoids cross-origin conflicts which might cause a separate window tab to be created
  • The empty string is documented as an explicitly handled "blank page" behavior
• Opening the window to a blank page before setting its location explicitly

Testing instructions:

Ensure preview end-to-end tests pass (notably in Travis CI).

npm run test-e2e test/e2e/specs/preview.test.js

Ensure PostPreviewButton unit tests pass:

npm run test-unit packages/editor/src/components/post-preview-button/test/index.js

Verify that Preview button will open a tab if one does not already exist, or reuse an existing open tab.

Caveat: While the open tab will be reused, it's not guaranteed it will be given focus. This is consistent with Classic Editor behavior and, to my knowledge, it is not possible to force focus on the opened tab.

[gziolo]

It looks promising given that Travis is happy. Code looks good. I will give it a spin locally 😃

I will also restart the job which runs e2e tests a few times, to see if Travis likes it.

[gziolo]

Travis sometimes times out for the test we are trying to fix. I hope that #8301 will help to fix it.

I run it several times locally in a few different setups and it is passing all the time.

[notnownikki]

The problem here is sometimes targetcreated isn't caught, and this causes intermittent failures (which, for some reason, I can reproduce 90% of the time on a local baremetal linux machine). What happens is that sometimes, the listener doesn't get triggered as soon as the target is created, so the test carries on and previewPage is undefined when we try to interact with it. There's an issue talking about this at ChromeDevTools/devtools-protocol#77 , showing targetcreated missing early page activity, and the Google peeps consider popup behaviour an edge case, so the underlying issue isn't likely to be fixed anytime soon. So in #8301 I count the number of open pages and wait for the new page to appear, and that's reliable.

[gziolo]

With the changes introduced in this PR we open new popup only once. So this shouldn't be as problematic as before where we had cases where additional popups were opened. Does this test fail for you on the latest master?

[notnownikki]

Does this test fail for you on the latest master?

Not as frequently as it used to, but sometimes, yes.

[gziolo]

Can you rebase #8301 on top of the master and change only the part which opens tab for the first time?

[notnownikki]

Sure! Will start on that this morning.

[notnownikki]

So I've run this branch 10 times now, and twice it's crashed waiting for the preview navigation (not opening a new preview page) with

Error: Timeout - Async callback was not invoked within the 100000ms timeout specified by jest.setTimeout.

I'll resolve the conflicts and fix this in #8301

[gziolo]

Thanks for looking into it. It's still much better than before and what's more important this PR fixes a real issue user can encounter. Let's make #8301 bulletproof 🚀

[notnownikki]

Oh yeah, it's much better than it was :) it just seems like the machine I have running e2e tests here is excellent at surfacing these errors!