While I don't think it needs to be a blocker, we should definitely have an end-to-end test case covering this behavior.

My thinking was something in packages/e2e-tests/specs/editor/various/change-detection.test.js:

it( 'Should not prompt to confirm unsaved changes when trashing an existing post', async () => {
    await createNewPost( {
        title: 'My New Post',
    } );

    await saveDraft();
    
    // ...trash post and assert no dialog
} );

Per #17427 (comment), do we still have the potential that a 500 server response can occur when the save occurs after trashing the post? My thinking is something along the lines of:

1. Add title
2. Save post
3. Edit title
4. Trash post

At step 4, isn't it true that since the save occurs after the trashing, the payload (the edited title) may cause a failed response?

Even after that was fixed by waiting for saving to finish completely before navigating away, the UnsavedChangesWarning listener fired immediately with stale values.

Why are those values "stale" ? Is it that componentDidUpdate is triggered for BrowserURL before the same props can take effect for UnsavedChangesWarning ? It feels to me fragile or non-obvious, and at the very least I would suggest we add a comment explaining why we are calling the selector directly.

While I don't think it needs to be a blocker, we should definitely have an end-to-end test case covering this behavior.

Done 😄

At step 4, isn't it true that since the save occurs after the trashing, the payload (the edited title) may cause a failed response?

Yeah, but you would want to know you have unsaved changes then.

Why are those values "stale" ? Is it that componentDidUpdate is triggered for BrowserURL before the same props can take effect for UnsavedChangesWarning ? It feels to me fragile or non-obvious, and at the very least I would suggest we add a comment explaining why we are calling the selector directly.

Yes, exactly. I already had a comment there, and I've made it clearer now.

Per #17427 (comment), do we still have the potential that a 500 server response can occur when the save occurs after trashing the post? My thinking is something along the lines of:

The current Travis test failure appears to suggest an issue exists:

FAIL packages/e2e-tests/specs/editor/various/change-detection.test.js (65.521s)
  ● Change detection › should not prompt to confirm unsaved changes when trashing an existing post
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 500 (Internal Server Error)"]
      at Object.expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)
          at runMicrotasks (<anonymous>)

It's also not surfaced to the user in any meaningful way: There is no notice, and the "Saving" button never changes back to a finished state.

I suspect the 500 seen in the test log is a result of the fact that a draft save is issued via assertIsDirty (edit: this isn't the behavior that happens) at the same time the post is being trashed. Running through the behavior in my browser, it all works correctly, and I see no 500 error.

I will suggest that we revert the commit adding the test case, and reintroduce a fixed implementation in a subsequent pull request.

We needed to wait for the post to save again before asserting:

#18290

This PR fixes and reintroduces the test case from #18275.

It also replaces some ad-hoc saving code with the correct e2e test util, saveDraft.