Gallery block / media-placeholder - Preserve changes made while upload in progress.

[Addison-Stavlo]

Description

This PR aims to fix 3 bugs related to the Gallery block component.

Before - The bugs that exist.

1 - When Images are removed while another is uploading, they are re-added once upload is complete.

2 - When two images are uploaded separately, if the first finishes after the second then the second one is removed once the first finishes its upload.

3 - When two images are uploaded separately, if the first finishes before the second then the loader for the second disappears. Once the second finishes, "empty" loading image replaces the first.

How do we go about fixing the above bugs?

Currently, the media-placeholder component destructures the value from props and adds this into the setMedia function as currentValue. This is never updated during the lifetime of the function which is why the above issues exist. In case 1, since these images are initially added to the function in currentValue, they remain in currentValue even after they are deleted from the screen. Once the setMedia function runs when the uploading image completes, it returns currentValue with those unwanted deleted images.

To fix this, I have added another reference to this.props.value (named currentMedia) but declared inside the setMedia function itself. This allows us to compare currentValue (the 'snapshot' of the gallery's images at the beginning of the upload, now renamed mediaToReturn ) with the values that are in the gallery at the time the function is run.

This now allows us to do 2 important checks when the setMedia function runs:

1. Have items been removed from the gallery since the upload started?
2. Have completed items been added to the gallery since the upload started?

With these checks we are then able to add/remove items from mediaToReturn as necessary so that once the upload completes, we are able to return a state representative of changes that have happened since it started.

Note: Why update mediaToReturn and not just use the current values in this.props.value altogether?

You will find that this.props.value contains temporary items returned from the setMedia function, such as the loading placeholders. We don't want to return these placeholders with their corresponding completed uploads, so they would need to be filtered out in that case. Since currently there is not a good way to tell which placeholders correspond to which completed uploads, it is more simple to add items to mediaToReturn than to remove the correctly corresponding ones from the current value of the gallery.

After the described changes.

1 - When Images are removed while another is uploading, they are not re-added once the upload completes.

2 - When two images uploaded separately, if the first finishes after the second they will both remain in the gallery.

3 - When two images are uploaded separately, if the first finishes before the second the loader for the second still disappears. However, once it finishes its upload they both are present in the gallery with no empty loading placeholder.

Some comments on the code itself

1. There is an adjactent *.native.js file to the media-placeholder index file. This has yet to be updated as I am not currently set up to test functionality on native and wanted to get the discussion on this moving before then. None of the changes made should break any functionality on native, but since the logic in the native file has not been updated there should be no change to how the gallery block works on native until this is similarly updated. It may also be worth noting that cases 2 & 3 may be much less commonly experienced through the mobile workflow.

2. For comparisons we are using ids (and in some cases urls) of the media. My first thought was to use lodash isEqual to compare the objects, but noticed there could be some inconsistencies on which properties were present on the objects throughout the lifetime of the function ( fullUrl: undefined for example may be present in the values on the first pass of the function, but absent from the copies returned with the loading placeholder causing isEqual to always return false on a second pass. ).

How has this been tested?

Tested on local gutenberg docker environment.
Run this PR and test initiating different uploads simultaneously, removing images from the gallery during an upload, etc. Recreate the above scenarios listed and test with group uploads as well.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[noahtallen]

Thanks for the super detailed PR description! There were several times I had questions about the code that were answered in the description. Very nice :)

we are using ids of the media

This makes sense to me. Is there a reason why we use both ID and URL, or could we just use one of those?

there should be no change to how the gallery block works on native until this is similarly updated

I'm also not sure how to develop the changes for the react native side of things. My thought would be to merge this before looking into changes to native code.

This may not be necessary but has been done to ensure backwards compatibility with all other components that use this media-placeholder.

Ahh this makes a lot of sense. Do you know if we experience similar bugs on other components that would be fixed once remove this check?

I did notice one more bug which happens when I do the following:

1. Start with an empty gallery
2. Add several images at the same time
3. When the first image finishes uploading but before the others have finished uploading, click into it and remove it.
4. Once the other images upload, you'll see the removed image re-appear in the gallery.

Overall though, great bug fix. I think it's ready to merge unless you want to fix any of the nitpicks! :)

[Addison-Stavlo]

Is there a reason why we use both ID and URL, or could we just use one of those?

@noahtallen Yeah, now that you mention it we may not need the Ids in that first check. I added the urls as a second thought later. Since placeholder images don't come back from the Gallerys props with ids on them, I needed to add something else so they were not checking undefined === undefined.

Do you know if we experience similar bugs on other components that would be fixed once remove this check?

I am not able to find any other components/blocks that use the MediaPlaceholder with the multiple property passed into them. If this is truly the case, this change should not fix any similar bugs on other components. If this is not the case, I would love to hear which other blocks/components use the MediaPlaceholder with the multiple property.

I did notice one more bug which happens when I do the following:
Start with an empty gallery
Add several images at the same time
When the first image finishes uploading but before the others have finished uploading, click into it and remove it.
Once the other images upload, you'll see the removed image re-appear in the gallery.

Yeah, there are a couple other odd bugs like this that exist. While this does not fix them, it does not create them either. I think at that point since it was triggered by a group upload, it is less likely that the user will want to remove it during the group upload. What is more likely is what we are fixing where a user may select a group upload and remove images (that were already on the gallery) while that upload is in progress. I think to fix the last of these bugs we may need to reorganize both how mediaUpload passes newMedia into the setMedia function as well as how the placeholders are set in the Gallery block, which might be a bit larger in scope.

[youknowriad]

@jorgefilipecosta do you have thoughts here?

[Addison-Stavlo]

I have updated the logic a little bit here. It is now a little more simplified and robust. This now also fixes the remaining issue I noted above in example 3.

3 - When two images are uploaded separately, if the first finishes before the second the loader for the second still disappears. However, once it finishes its upload they both are present in the gallery with no empty loading placeholder.

Now, the second loader will no longer disappear once the first upload completes.

Now when setMedia runs, we take this.props.value and filter out anything we had added with newMedia in the last run of the function. We return this with the current value of newMedia added to it, and save these URLs from newMedia for filtering again in the next iteration.

[noahtallen]

nice changes! I think this is almost ready to go

[noahtallen]

I think this might need a rebase to get the test passing again :)

[jorgefilipecosta]

Hi @Addison-Stavlo, I added a PR that removes the need to getValue, the difference compared to what we tried before was to reference this.props.value directly inside setMedia if we use a value variable captured before setMedia is defined the behavior is buggy.
Would you be able to confirm if things continue to work with this change? Thank you in advance :)

[Addison-Stavlo]

@jorgefilipecosta Thanks for taking a look and proposing a cleaner solution. This "mostly" works. There is a small behavioral regression on an edge case but it is still a good improvement to the functionality on master.

Using proposed this.props.value:

As you may notice once the first image finishes uploading, the placeholder for the second image disappears. Once completed, both images are present in the gallery so we do achieve the correct final state. (This applies to groups as well, if we were dragging 3 images each time instead of 1, that second group of 3 would disappear until the upload is complete). This only happens when the starting state is an empty gallery.

Using getValue:

The placeholder for the second uploading image never disappears unexpectedly. I was unsuccessful in getting the behavior to work this way when I tried the other approach.

That being said, this is an edge case that I would not expect many users to encounter, and it does end up with the expected state in the end. I'd be happy either way, the main reason this PR was opened was to handle the case where someone would try to remove existing images while another upload was in progress (the removed images would reappear once the upload completed). Beyond that, I have tried to address other edge cases that I have found along the way and have probably gotten a bit distracted trying to solve those as well.

[jorgefilipecosta]

This solution addresses most of the issues and does not change anything on the public API. I guess we can go with this solution 👍
Thank you for all the work and for debugging on this PR @Addison-Stavlo!

[Addison-Stavlo]

Thanks for all of your time and help on this one @jorgefilipecosta !

Tested again, things work well! I updated a comment that was still referencing getValue, but other than that I think this is good to go. Will merge after Travis approves again. 😄

[talldan]

It looks like this is the cause of #22338, that when first adding a gallery block the Media Library is always in addToGallery mode with all images selected on the Insert Gallery page.

[talldan]

I've read more of the details on this PR, and I see that the issue is related to hasImagesWithId being false when an image upload is in progress and other operations causing the uploading image to be replaced.

I've made naive PR at #22659 to attempt to solve #22338