web-stories-wp
web-stories-wp copied to clipboard
GoogleForCreators
Media: Processing items missing after opening media dialog
Context
Summary
Add onUpload and onDelete callbacks to media upload component. This has a number of key benefits. This allow for media added and removed from within the editor. For example, if an image is delete from the dialog, it is removed from the media library and on the canvas.
I have also taken this opportunity to use this new onDelete to improve background audio and captions. If these files are deleted within the dialog, then background audio and caption are reset / removed.
Relevant Technical Choices
To-do
User-facing changes
https://user-images.githubusercontent.com/237508/179557163-02d60356-78a7-4eb7-ac6e-9083b1fcca83.mov
Testing Instructions
- [ ] This is a non-user-facing change and requires no QA
This PR can be tested by following these steps:
- Create a story.
- Open media dialog.
- Upload image.
- Insert into canvas.
- See new image into media library.
- Create a story.
- Open media dialog.
- Insert image into canvas.
- Open media dialog.
- Select image ( the one used before ).
- Delete image
- Close dialog.
- See image removed from canvas and in media library.
- Create a story.
- Select page
- Go to style tab.
- Upload audio file.
- Select file.
- Click replace file.
- Delete image ( the one used before ).
- Close dialog.
- See background audio is reset.
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
- [x] This PR addresses an existing issue and I have linked this PR to it in ZenHub
- [x] I have tested this code to the best of my abilities
- [ ] I have verified accessibility to the best of my abilities (docs)
- [ ] I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
- [ ] This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
- [ ] I have added documentation where necessary
- [x] I have added a matching
Type: XYZlabel to the PR
Fixes #9963
Size Change: +40 B (0%)
Total Size: 2.66 MB
ℹ️ View Unchanged
| Filename | Size | Change |
|---|---|---|
assets/css/carousel-view-rtl.css |
702 B | 0 B |
assets/css/carousel-view.css |
701 B | 0 B |
assets/css/web-stories-block-rtl.css |
4.52 kB | 0 B |
assets/css/web-stories-block.css |
4.56 kB | 0 B |
assets/css/web-stories-embed-rtl.css |
318 B | 0 B |
assets/css/web-stories-embed.css |
317 B | 0 B |
assets/css/web-stories-list-styles-rtl.css |
2.36 kB | 0 B |
assets/css/web-stories-list-styles.css |
2.39 kB | 0 B |
assets/css/web-stories-theme-style-twentyeleven-rtl.css |
102 B | 0 B |
assets/css/web-stories-theme-style-twentyeleven.css |
102 B | 0 B |
assets/css/web-stories-theme-style-twentyfifteen-rtl.css |
251 B | 0 B |
assets/css/web-stories-theme-style-twentyfifteen.css |
251 B | 0 B |
assets/css/web-stories-theme-style-twentyfourteen-rtl.css |
287 B | 0 B |
assets/css/web-stories-theme-style-twentyfourteen.css |
287 B | 0 B |
assets/css/web-stories-theme-style-twentyseventeen-rtl.css |
288 B | 0 B |
assets/css/web-stories-theme-style-twentyseventeen.css |
288 B | 0 B |
assets/css/web-stories-theme-style-twentysixteen-rtl.css |
224 B | 0 B |
assets/css/web-stories-theme-style-twentysixteen.css |
224 B | 0 B |
assets/css/web-stories-theme-style-twentyten-rtl.css |
143 B | 0 B |
assets/css/web-stories-theme-style-twentyten.css |
143 B | 0 B |
assets/css/web-stories-theme-style-twentytwelve-rtl.css |
256 B | 0 B |
assets/css/web-stories-theme-style-twentytwelve.css |
256 B | 0 B |
assets/css/web-stories-theme-style-twentytwenty-rtl.css |
86 B | 0 B |
assets/css/web-stories-theme-style-twentytwenty.css |
86 B | 0 B |
assets/css/web-stories-theme-style-twentytwentyone-rtl.css |
326 B | 0 B |
assets/css/web-stories-theme-style-twentytwentyone.css |
326 B | 0 B |
assets/css/web-stories-widget-rtl.css |
482 B | 0 B |
assets/css/web-stories-widget.css |
482 B | 0 B |
assets/css/wp-dashboard-rtl.css |
657 B | 0 B |
assets/css/wp-dashboard.css |
659 B | 0 B |
assets/css/wp-story-editor-rtl.css |
737 B | 0 B |
assets/css/wp-story-editor.css |
738 B | 0 B |
assets/js/1590.js |
1.14 MB | 0 B |
assets/js/1814.js |
7.46 kB | 0 B |
assets/js/2505.js |
34.9 kB | 0 B |
assets/js/3617.js |
224 kB | 0 B |
assets/js/4422.js |
49.3 kB | 0 B |
assets/js/5980.js |
5.48 kB | 0 B |
assets/js/carousel-view.js |
3.41 kB | 0 B |
assets/js/chunk-colorthief.js |
2.64 kB | 0 B |
assets/js/chunk-ffmpeg.js |
5.64 kB | 0 B |
assets/js/chunk-focus-visible.js |
1.01 kB | 0 B |
assets/js/chunk-getStoryMarkup.js |
5.86 kB | 0 B |
assets/js/chunk-html-to-image.js |
4.6 kB | 0 B |
assets/js/chunk-opentype.js |
96 B | 0 B |
assets/js/chunk-react-calendar.js |
12.4 kB | 0 B |
assets/js/chunk-react-color.js |
44.3 kB | 0 B |
assets/js/chunk-resize-observer-polyfill.js |
2.57 kB | 0 B |
assets/js/chunk-web-animations-js.js |
14.6 kB | 0 B |
assets/js/chunk-web-stories-template-0-metaData.js |
546 B | 0 B |
assets/js/chunk-web-stories-template-0.js |
10.6 kB | 0 B |
assets/js/chunk-web-stories-template-1-metaData.js |
540 B | 0 B |
assets/js/chunk-web-stories-template-1.js |
9.01 kB | 0 B |
assets/js/chunk-web-stories-template-10-metaData.js |
533 B | 0 B |
assets/js/chunk-web-stories-template-10.js |
6.91 kB | 0 B |
assets/js/chunk-web-stories-template-11-metaData.js |
540 B | 0 B |
assets/js/chunk-web-stories-template-11.js |
8.51 kB | 0 B |
assets/js/chunk-web-stories-template-12-metaData.js |
496 B | 0 B |
assets/js/chunk-web-stories-template-12.js |
9.48 kB | 0 B |
assets/js/chunk-web-stories-template-13-metaData.js |
525 B | 0 B |
assets/js/chunk-web-stories-template-13.js |
7.3 kB | 0 B |
assets/js/chunk-web-stories-template-14-metaData.js |
582 B | 0 B |
assets/js/chunk-web-stories-template-14.js |
7.58 kB | 0 B |
assets/js/chunk-web-stories-template-15-metaData.js |
544 B | 0 B |
assets/js/chunk-web-stories-template-15.js |
8.21 kB | 0 B |
assets/js/chunk-web-stories-template-16-metaData.js |
588 B | 0 B |
assets/js/chunk-web-stories-template-16.js |
10.3 kB | 0 B |
assets/js/chunk-web-stories-template-17-metaData.js |
539 B | 0 B |
assets/js/chunk-web-stories-template-17.js |
8.52 kB | 0 B |
assets/js/chunk-web-stories-template-18-metaData.js |
585 B | 0 B |
assets/js/chunk-web-stories-template-18.js |
9.05 kB | 0 B |
assets/js/chunk-web-stories-template-19-metaData.js |
501 B | 0 B |
assets/js/chunk-web-stories-template-19.js |
9.99 kB | 0 B |
assets/js/chunk-web-stories-template-2-metaData.js |
586 B | 0 B |
assets/js/chunk-web-stories-template-2.js |
9.16 kB | 0 B |
assets/js/chunk-web-stories-template-20-metaData.js |
548 B | 0 B |
assets/js/chunk-web-stories-template-20.js |
8.59 kB | 0 B |
assets/js/chunk-web-stories-template-21-metaData.js |
534 B | 0 B |
assets/js/chunk-web-stories-template-21.js |
9.16 kB | 0 B |
assets/js/chunk-web-stories-template-22-metaData.js |
525 B | 0 B |
assets/js/chunk-web-stories-template-22.js |
7.37 kB | 0 B |
assets/js/chunk-web-stories-template-23-metaData.js |
605 B | 0 B |
assets/js/chunk-web-stories-template-23.js |
6.99 kB | 0 B |
assets/js/chunk-web-stories-template-24-metaData.js |
518 B | 0 B |
assets/js/chunk-web-stories-template-24.js |
10.8 kB | 0 B |
assets/js/chunk-web-stories-template-25-metaData.js |
544 B | 0 B |
assets/js/chunk-web-stories-template-25.js |
7.07 kB | 0 B |
assets/js/chunk-web-stories-template-26-metaData.js |
601 B | 0 B |
assets/js/chunk-web-stories-template-26.js |
6.85 kB | 0 B |
assets/js/chunk-web-stories-template-27-metaData.js |
543 B | 0 B |
assets/js/chunk-web-stories-template-27.js |
7.36 kB | 0 B |
assets/js/chunk-web-stories-template-28-metaData.js |
532 B | 0 B |
assets/js/chunk-web-stories-template-28.js |
8.49 kB | 0 B |
assets/js/chunk-web-stories-template-29-metaData.js |
561 B | 0 B |
assets/js/chunk-web-stories-template-29.js |
8.49 kB | 0 B |
assets/js/chunk-web-stories-template-3-metaData.js |
540 B | 0 B |
assets/js/chunk-web-stories-template-3.js |
8.22 kB | 0 B |
assets/js/chunk-web-stories-template-30-metaData.js |
576 B | 0 B |
assets/js/chunk-web-stories-template-30.js |
7.67 kB | 0 B |
assets/js/chunk-web-stories-template-31-metaData.js |
503 B | 0 B |
assets/js/chunk-web-stories-template-31.js |
9.61 kB | 0 B |
assets/js/chunk-web-stories-template-32-metaData.js |
551 B | 0 B |
assets/js/chunk-web-stories-template-32.js |
12.2 kB | 0 B |
assets/js/chunk-web-stories-template-33-metaData.js |
492 B | 0 B |
assets/js/chunk-web-stories-template-33.js |
8.86 kB | 0 B |
assets/js/chunk-web-stories-template-34-metaData.js |
571 B | 0 B |
assets/js/chunk-web-stories-template-34.js |
7.57 kB | 0 B |
assets/js/chunk-web-stories-template-35-metaData.js |
565 B | 0 B |
assets/js/chunk-web-stories-template-35.js |
8.81 kB | 0 B |
assets/js/chunk-web-stories-template-36-metaData.js |
576 B | 0 B |
assets/js/chunk-web-stories-template-36.js |
11.6 kB | 0 B |
assets/js/chunk-web-stories-template-37-metaData.js |
528 B | 0 B |
assets/js/chunk-web-stories-template-37.js |
6.47 kB | 0 B |
assets/js/chunk-web-stories-template-38-metaData.js |
572 B | 0 B |
assets/js/chunk-web-stories-template-38.js |
7.96 kB | 0 B |
assets/js/chunk-web-stories-template-39-metaData.js |
589 B | 0 B |
assets/js/chunk-web-stories-template-39.js |
7.67 kB | 0 B |
assets/js/chunk-web-stories-template-4-metaData.js |
565 B | 0 B |
assets/js/chunk-web-stories-template-4.js |
11.5 kB | 0 B |
assets/js/chunk-web-stories-template-40-metaData.js |
556 B | 0 B |
assets/js/chunk-web-stories-template-40.js |
9.13 kB | 0 B |
assets/js/chunk-web-stories-template-41-metaData.js |
572 B | 0 B |
assets/js/chunk-web-stories-template-41.js |
7.75 kB | 0 B |
assets/js/chunk-web-stories-template-42-metaData.js |
522 B | 0 B |
assets/js/chunk-web-stories-template-42.js |
7 kB | 0 B |
assets/js/chunk-web-stories-template-43-metaData.js |
558 B | 0 B |
assets/js/chunk-web-stories-template-43.js |
8.37 kB | 0 B |
assets/js/chunk-web-stories-template-44-metaData.js |
582 B | 0 B |
assets/js/chunk-web-stories-template-44.js |
10.1 kB | 0 B |
assets/js/chunk-web-stories-template-45-metaData.js |
564 B | 0 B |
assets/js/chunk-web-stories-template-45.js |
7.12 kB | 0 B |
assets/js/chunk-web-stories-template-46-metaData.js |
531 B | 0 B |
assets/js/chunk-web-stories-template-46.js |
5.01 kB | 0 B |
assets/js/chunk-web-stories-template-47-metaData.js |
592 B | 0 B |
assets/js/chunk-web-stories-template-47.js |
8.46 kB | 0 B |
assets/js/chunk-web-stories-template-48-metaData.js |
556 B | 0 B |
assets/js/chunk-web-stories-template-48.js |
8.31 kB | 0 B |
assets/js/chunk-web-stories-template-49-metaData.js |
518 B | 0 B |
assets/js/chunk-web-stories-template-49.js |
9.7 kB | 0 B |
assets/js/chunk-web-stories-template-5-metaData.js |
555 B | 0 B |
assets/js/chunk-web-stories-template-5.js |
9.38 kB | 0 B |
assets/js/chunk-web-stories-template-50-metaData.js |
504 B | 0 B |
assets/js/chunk-web-stories-template-50.js |
8.26 kB | 0 B |
assets/js/chunk-web-stories-template-51-metaData.js |
527 B | 0 B |
assets/js/chunk-web-stories-template-51.js |
9.89 kB | 0 B |
assets/js/chunk-web-stories-template-52-metaData.js |
602 B | 0 B |
assets/js/chunk-web-stories-template-52.js |
10.1 kB | 0 B |
assets/js/chunk-web-stories-template-53-metaData.js |
553 B | 0 B |
assets/js/chunk-web-stories-template-53.js |
5.79 kB | 0 B |
assets/js/chunk-web-stories-template-54-metaData.js |
547 B | 0 B |
assets/js/chunk-web-stories-template-54.js |
7.52 kB | 0 B |
assets/js/chunk-web-stories-template-55-metaData.js |
574 B | 0 B |
assets/js/chunk-web-stories-template-55.js |
6.56 kB | 0 B |
assets/js/chunk-web-stories-template-56-metaData.js |
543 B | 0 B |
assets/js/chunk-web-stories-template-56.js |
9.5 kB | 0 B |
assets/js/chunk-web-stories-template-57-metaData.js |
528 B | 0 B |
assets/js/chunk-web-stories-template-57.js |
14.1 kB | 0 B |
assets/js/chunk-web-stories-template-58-metaData.js |
556 B | 0 B |
assets/js/chunk-web-stories-template-58.js |
5.61 kB | 0 B |
assets/js/chunk-web-stories-template-59-metaData.js |
588 B | 0 B |
assets/js/chunk-web-stories-template-59.js |
8.52 kB | 0 B |
assets/js/chunk-web-stories-template-6-metaData.js |
569 B | 0 B |
assets/js/chunk-web-stories-template-6.js |
7.04 kB | 0 B |
assets/js/chunk-web-stories-template-60-metaData.js |
509 B | 0 B |
assets/js/chunk-web-stories-template-60.js |
8.89 kB | 0 B |
assets/js/chunk-web-stories-template-7-metaData.js |
569 B | 0 B |
assets/js/chunk-web-stories-template-7.js |
7.21 kB | 0 B |
assets/js/chunk-web-stories-template-8-metaData.js |
569 B | 0 B |
assets/js/chunk-web-stories-template-8.js |
8.4 kB | 0 B |
assets/js/chunk-web-stories-template-9-metaData.js |
581 B | 0 B |
assets/js/chunk-web-stories-template-9.js |
8.49 kB | 0 B |
assets/js/chunk-web-stories-templates.js |
443 B | 0 B |
assets/js/chunk-web-stories-textset-0.js |
5.08 kB | 0 B |
assets/js/chunk-web-stories-textset-1.js |
6.64 kB | 0 B |
assets/js/chunk-web-stories-textset-2.js |
7.67 kB | 0 B |
assets/js/chunk-web-stories-textset-3.js |
15.1 kB | 0 B |
assets/js/chunk-web-stories-textset-4.js |
4.16 kB | 0 B |
assets/js/chunk-web-stories-textset-5.js |
5.49 kB | 0 B |
assets/js/chunk-web-stories-textset-6.js |
5.3 kB | 0 B |
assets/js/chunk-web-stories-textset-7.js |
10.2 kB | 0 B |
assets/js/generateBlurhash.worker.worker.js |
1.1 kB | 0 B |
assets/js/imgareaselect.js |
3.77 kB | 0 B |
assets/js/lightbox.js |
550 B | 0 B |
assets/js/tinymce-button.js |
2.84 kB | 0 B |
assets/js/web-stories-activation-notice.js |
26.9 kB | 0 B |
assets/js/web-stories-block.js |
22.7 kB | 0 B |
assets/js/web-stories-embed.js |
20 B | 0 B |
assets/js/web-stories-widget.js |
587 B | 0 B |
assets/js/wp-dashboard.js |
72.6 kB | 0 B |
assets/js/wp-story-editor.js |
327 kB | +40 B (0%) |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Plugin builds for 621f5c3412ae692e3881057583b7f6fb57485c42 are ready :bellhop_bell:!
- Download development build
- Download production build
This pull request introduces 1 alert when merging bd2c0d96b0fc9652ef5153f12db71b68c8ecf1d8 into 77bc376ee9e406bf69e9534fc9d2b72617ae0b48 - view on LGTM.com
new alerts:
- 1 for Unused variable, import, function or class
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request introduces 1 alert when merging d80973fe473d9b4afcb1c654bcb62232ba90461c into 22cf54d030e407f469350f21273f6d1c223aa99b - view on LGTM.com
new alerts:
- 1 for Unused variable, import, function or class
@swissspidy I am seeing blurhash and base color generating twice on every upload now. Sadly, I can't think of way around this, as the second is done after inserting in canvas and update the element on page. Without that, it means the elements are inserted into page after the base color is generated and elements in pages are updated. Any thought on this one?
This pull request introduces 1 alert when merging 36cfa022b266f05b5c1103055bcae2a7fd6a559d into 22cf54d030e407f469350f21273f6d1c223aa99b - view on LGTM.com
new alerts:
- 1 for Unused variable, import, function or class
So just an update here.
I did some playing around with this. I basically adding some state, to add an array of new resources added and then when the dialog is completed only add new resources on dialog close. Something like this.
useEffect(() => {
if (doProcess && media) {
console.log(media);
prependMedia({
media,
});
setDoProcess(false);
setMedia([]);
}
}, [doProcess, media, prependMedia]);
const onClose = useCallback(() => {
if (onCloseCallback) {
onCloseCallback();
}
setDoProcess(true);
}, [onCloseCallback]);
const uploadMedia = useCallback(
(resource) => {
if (['image', 'video', 'gif'].includes(resource.type)) {
setMedia((state) => [resource, ...state]);
}
if (onUploadMedia) {
onUploadMedia(resource);
}
},
[onUploadMedia]
);
However, I got into state based problems. The above code worked well on the first run, add these callbacks are not rehooked, then on close i was getting an empty array. I am not sure I was around this. If anyone else has any thoughtst that would be great.
Quick summary of my chat with Jonny about this yesterday:
UX-wise instantly adding the media to the library would be preferable. But that currently causes issues with race conditions. Example scenario:
- Open media modal
- Upload file
- File is added to the editor media library in the background -> this triggers things like blurhash processing, etc. due to this effect:
https://github.com/GoogleForCreators/web-stories-wp/blob/621f5c3412ae692e3881057583b7f6fb57485c42/packages/story-editor/src/app/media/local/useContextValueProvider.js#L278-L282 - In the media modal, click on "Insert" media -> the
onSelect/onInsertcallback callspostProcessingResourcehttps://github.com/GoogleForCreators/web-stories-wp/blob/621f5c3412ae692e3881057583b7f6fb57485c42/packages/story-editor/src/components/form/mediaUploadButton.js#L135 - The problem: at this point the
postProcessingResourcecallback will be stale and is thus not "aware" that the processing already happened in step 3. This causes everything to run twice.
We discussed that we could perhaps remove the postProcessingResource call and instead trigger this via an effect (similar to the existing effect).
AIs:
- Pascal to take a closer look into this to see what kind of options we have here.
@swissspidy I think I have found a solution to this problem using a ref. See https://github.com/GoogleForCreators/web-stories-wp/pull/11941/commits/2d34582d4b0140d9cdf1ea4fab1c1750101b4d81.
Take a look at the commit and see what I am doing here.
The change is a bit hard to read/follow, i's not obvious what it does, can you elaborate a bit?
As per my above comment I plan on taking a closer look at this problem to better understand it. But that probably won't be today anymore I'm afraid
@swissspidy That is happening in last commit.
The instance of wp.media in now stored / cached in a ref. This means, that a new instance is not created on every usage, keeping state.
On adding / remove of media in the dialog, an array ( queue ) of resources is saved. On closing the dialog, resources are added to the media library all at once. Stopping race conditions, where adding resource in the background results in base color/blurhash/poster is generated in the background and not updated image/video element later inserted into canvas.
On adding / remove of media in the dialog, an array ( queue ) of resources is saved. On closing the dialog, resources are added to the media library all at once.
As per my previous comments and the previous discussion, I would really prefer feedback straightaway, no queue.
@swissspidy @timarney is one of you going to take this PR on? Or will wait until I get back.
I think it is close to mergable but I haven't got review yet.
Pascal to take a closer look into this to see what kind of options we have here.
@swissspidy are you still going to look at options for this one?
Yes I still intend to look into it. Haven't found time yet to fully dig into it though, but it's not urgent anyways
I went down quite the rabbit whole trying to analyze this one and coming up with a good solution. While it feels like we're 90% there, it's clear now that this needs some more thinking and tinkering to nail down the remaining 10%. And given the impact of this change, also tests.
Some preliminary findings in no particular order:
- Storing the
wp.mediainstance in a ref had negative side effects like stale data. For example when deleting an item in the modal and closing & reopening it, you’d still see the deleted item there. - Adding items to the library (either directly or in a queue) that were uploaded to the media modal is not always wanted.
- For example MOV files should not be added to the library since they require transcoding
- It also causes issues for animated GIFs, where the GIF is added to the library, and then we convert it to an MP4 and you briefly see 2 items in the library instead of just 1.
- This could be solved by some kind of in-place update of the GIF element in the media library
Related: #12123
This PR is now out of date. Closing and re-openning if I have a better idea.