Lock post saving during image uploads

[adamsilverstein]

What?

Fixes #39223

Improves the media upload experience and prevents saving/publishing while images are still uploading.

Why?

When users are uploading media and navigate away, uploads and image display break.

Gutenberg displays an "Unsaved Changes" warning if users try to navigate away from the editor with unsaved changes (triggered by window.beforeunload). The check for unsaved changes misses images that are still being uploaded allowing users to navigate away while WordPress is still uploading or processing an image.

In progress uploads may never complete regeneration if PHP is able to generate all sub sizes in one go, because our retry mechanism will not fire. Regardless, Gutenberg will continue marking the image as "in progress" with a spinner when the upload doesn't complete.

How?

Locks the save/publish mechanism until all image uploads have completed.

Testing Instructions

1. Create a new post and publish it.
2. drag in one or more very large images that will take several seconds to be processed
3. before image processing completes, hit update to update the post
4. click view post to navigate away
5. the image may or may show, the srcset will be incomplete (missing sizes)
6. edit the post again, note the images are stuck in an uploading state.

Screenshots or screencast

share.getcloudapp.com/8LuDGjnD

[ryanwelcher]

Just added a couple of comments.

[adamsilverstein]

@ryanwelcher / @Mamaduka - I cleaned this up a bit and this code now does lock the publish/update buttons while image uploads are processing, however I noticed some issues remain I'm not sure how to address...

• if you delete an image block while it is uploading, post publishing will never unlock. I guess we could catch this when the component is unmounted?
• the "Save draft" button is not disabled, if you click this during an image upload, the post will be saved in a broken state (exit and revisit the page and you will see it is perpetually loading the image)
• nothing warns users if they navigate away during an upload

I feel like we may need an additional editor store attribute for "imagesUploading" added to the conditionals, or maybe we can tap into experimental "entity saving" logic instead? or maybe we can somehow change isSaving or forceIsSaving - basically the editor shows the correct state during saves, and in a sense the image are being saved so it feels like the right fit, but i'm not sure how to control it or if we need another attribute.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: ryanwelcher <welcher@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: mtias <matveb@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[adamsilverstein]

@Mamaduka I updated the PR against trunk and it should be ready for review. I am open to suggestions for a better mechanism and also I'm not sure what may have changed in the interim that could make this cleaner.

@annezazu pinged me recently about this though, so I guess it is still an issue.

[annezazu]

Just tested this and can confirm the option to save remains greyed out while you have a photo uploading, making for a clearer experience:

no.option.to.update.mov

This is compared to the current experience:

current.experience.for.media.upload.mov

[ellatrix]

Perhaps I am doing something wrong, but I'm not seeing a disabled save or publish button while an image is uploading:

Or is locking not disabling the buttons? If not, it probably should be?

[adamsilverstein]

Or is locking not disabling the buttons? If not, it probably should be?

Looks like this is currently not locking the Publish button, only the Save button after you have published a post. Something must have changed with the publish button state, this was working previously...

Can you try testing with an already published post? In the meantime, I'll try fixing the publish button, I guess there are two now, the initial button:

and then the confirmation publish button -

(which could already be open when an image is uploaded). I guess for now the second button at least should be disabled during media uploads.

[adamsilverstein]

@ellatrix - actually, I wasn't testing correctly, this works as expected. the inner "publish confirmation" button is what gets disabled for an unpublished post.

To test, first open the publish confirmation panel by clicking the initial publish button. then try uploading one or more very large image(s) (or throttle your network connection). note the inner publish button is disabled for the duration of the upload(s).

after clicking this button you see another publish button that I am calling the publish confirmation button:

Here is a quick screencast, in it you can see I added some console logging as each image upload locks saving and then unlocks it when uploading completes.

Monosnap.screencast.2024-07-08.14-53-35.mp4

[adamsilverstein]

any additional feedback @ellatrix @Mamaduka or @youknowriad?

[youknowriad]

This looks great to me 👍

It feels perfect for an e2e test but can also be done separately.

[youknowriad]

I think the PR description deserves to be expanded a bit. We should not expect reviewers to click all the links to be able to understand what's happening here.

[youknowriad]

I think you need to run npm install to fix the package lock (the failing test here)

[adamsilverstein]

I think you need to run npm install to fix the package lock (the failing test here)

done

[adamsilverstein]

I think the PR description deserves to be expanded a bit. We should not expect reviewers to click all the links to be able to understand what's happening here.

Expanded with details from the Issue.