Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(8P) External Media: Improve External Media Copy/Insert Flow #15867

Closed
jeryj opened this issue May 20, 2020 · 24 comments · Fixed by #16081
Closed

(8P) External Media: Improve External Media Copy/Insert Flow #15867

jeryj opened this issue May 20, 2020 · 24 comments · Fixed by #16081
Assignees
Labels
[Extension] External Media Extend all block editor media tools to support external providers [Status] Needs Design [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Comments

@jeryj
Copy link
Contributor

jeryj commented May 20, 2020

At the moment we have a “Copy and Insert” button on the External Media modals. When you select some images and click that, you then see a modal showing you the images being copied. This blocks you from moving on, while providing nothing else for you to do.

copy-insert-modal

Describe the solution you'd like

Proposal (I have no idea how feasible it is)

  • Change the “Copy and Insert” button to “Select” (what the button is in the core Media Library)
  • When you click Select, go immediately back to the editor, so the user isn't blocked from moving on
  • Selected images are shown on the canvas in the loading state, like they would be if you drag/dropped an image on or uploaded it manually.

Todos:

  • Remove the existing Copying Modal
  • Add that spinner overlay to selected images as they get copied
  • Update the Insert button text to Inserting as images get copied
  • Add a polite speak() announcement that says the copying action is occurring and what to expect.
@jeryj jeryj added [Status] Needs Design [Extension] External Media Extend all block editor media tools to support external providers labels May 20, 2020
@jancavan
Copy link

Change the “Copy and Insert” button to “Select” (what the button is in the core Media Library)
When you click Select, go immediately back to the editor, so the user isn't blocked from moving on
Selected images are shown on the canvas in the loading state, like they would be if you drag/dropped an image on or uploaded it manually.

Thanks @jeryj. I thought about this same solution. It's absolutely not necessary to have to inform the user that their images "are being copied to their Media Library" as that information isn't relevant to the task, so we should make it function as if they were just uploading an image.

@obenland obenland added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label May 21, 2020
@obenland
Copy link
Member

The question that came out of the Backlog Grooming was, can this be made to behave similarly with when an image gets uploaded and inserted? What are the steps necessary to achieve that?

@jancavan
Copy link

Doing a quick test, currently, when you upload an image unsuccessfully, this is the error you get:

Screen Shot 2020-05-21 at 2 26 04 PM

I'm not sure if there are other types of errors, but this is what I found. The error message could be more helpful, but let's see how feasible this is first before we dive into that.

I think both uploading your own image and selecting from external libraries should behave the same way if possible. @jeryj @obenland

@jeryj
Copy link
Contributor Author

jeryj commented May 21, 2020

When this error would occur, the media library would already have been closed. I was thinking more along the lines of the error that happens when you drag and drop an image into the image block. That's the state you'd be at after trying to insert the images and when an error would occur.

Screen Shot 2020-05-21 at 4 42 00 PM

@jancavan
Copy link

Got it. I think in any case, we should try to mirror the behavior, if possible.

@obenland
Copy link
Member

@marekhrabe Do you think we could reuse some of your asset replacement work from FSE for this?

@marekhrabe
Copy link
Contributor

The code you've linked works in a synchronous way - the same as our current copy flow. It waits until everything is ready before inserting final content into the editor.

@marekhrabe
Copy link
Contributor

This is what happens when you drag & drop local image into the image block and the actual upload fails. Not really well handled…

Screenshot 2020-05-26 at 15 52 47

@obenland
Copy link
Member

The code you've linked works in a synchronous way - the same as our current copy flow

I wondered if we could use the remote URL for the image on modal close and then run the asset replacement in the background?

@johngodley
Copy link
Member

Noting that the reason it works like this is that at the time it wasn't possible to indicate to the user that any copying was happening. This is important because the user can save and leave the editor while copying is happening, resulting in a broken image (as the block isn't updated with the real URL).

The loading state doesn't stop the user leaving, but does tell them that something is happening. This matches existing Gutenberg behaviour.

The modal is obviously not an ideal situation, but was added as a workaround until possible to indicate the image status to the user.

@obenland
Copy link
Member

obenland commented Jun 2, 2020

The current Calypso implementation shows the media library and its upload process, and doesn't insert the image straight away. Is that something we can replicate?

@marekhrabe
Copy link
Contributor

I'm looking into the technical side of this now…

@marekhrabe
Copy link
Contributor

marekhrabe commented Jun 3, 2020

External Media Image Copying

I was looking into ways to avoid the "copying" modal.

First alternative - use exising upload mechanisms in block editor

Starting at core image block, trying to see how we can use it to our advantage. It has a really nice feature for uploading images. All you need to do is to insert the core/image block with an attribute url, to which you pass a Blob URL. This is what happens behind the scenes when you drag & drop image file into the editor.

The image block recognizes the URL is a Blob object, displays the image immediately with a spinner on top of it and starts the media upload in background. It also handles the upload success and replaces the temporary image with the new URL that points to the uploaded file.

The only way to trigger this automatic upload flow is to start with a Blob URL and we can only do that if we first download the original image in its full resolution into the browser. We cannot really use that in the External Media integration, since the image download would take some time and we would need to show some sort of loading in the meantime.

Even if that approach worked, it will only be usable for the image block. Not for any other block or different places where media are used. For example, the Featured Image control in the sidebar.

Second alternative - leverage media library

Alternative approach suggested was to open the media library in its uploading state. Most places use the core media library (available in window.wp.media) but we also run a completely custom media library replacement on wpcom when you are inside iframed block editor in Calypso. The later one uses window.postMessage to communicate between the iframed block editor and the media library in the top frame, which is a part of Calypso codebase.

Both versions of the media library support uploading images but similar to the image block, this expects the image to be locally present first, which is not true for External Media. We only have small resolution previews available.

Again, we would either need to download full res image first just so we can upload it or rewrite a significant amount of core and Calypso code to allow us to do things we need. I find the effort to orchestrate all codebases and come up with new features and API far beyond reasonable for the integration with External Media.

Also note that for any core update, we need to wait for several WP releases until it's safely available for use in the Jetpack codebase.

Conclusions

Researching all this, reading the code of most of pieces involved and knowing it's technical limits, I personally don't see any better solution other than the "copying" modal we have right now.

It ensures the media is present on the site first and its id from the WP database is known, as other pieces of the block editor rely on this info and sometimes we only have one shot to send it from the modal back to UI. If we send some temporary state, we might not have a chance later to replace it easily with the uploaded version.

I understand this brings an undesirable friction and delay into the UI but if we don't do this, I'm worried many people would run into strange and hard to debug problems with inconsistent data and in general, broken editor. We might also end up with a lot of code just to handle this integration and risk that any update to the block editor and media library will break it.

Suggested way forward

I'll suggest keeping the copy modal and to improve it's design so it's clear what is happening, why, and maybe use better visual way to communicate the process.

@obenland
Copy link
Member

obenland commented Jun 3, 2020

@jancavan Do you think you'll have time to look at possible design improvements for the loading modal until tomorrow's backlog grooming?

@jancavan
Copy link

jancavan commented Jun 3, 2020

The best solution is still to mirror the flow of inserting media from the media library. Just to illustrate:

  1. User inserts an image block and selects media library:

Screen Shot 2020-06-03 at 10 40 00 AM

  1. User select one of the external media libraries:

Screen Shot 2020-06-03 at 10 45 38 AM

  1. From the modal, they select an image (button here will say "Insert" just like the normal media library flow):

Screen Shot 2020-06-03 at 10 46 19 AM

  1. Clicking insert will immediately add the image to their post/page:

Screen Shot 2020-06-03 at 10 42 25 AM

I'll suggest keeping the copy modal and to improve it's design so it's clear what is happening, why, and maybe use better visual way to communicate the process.

I don't really know how else to improve this because no matter how we communicate it, copying the media over is still irrelevant to the user's task and it's not necessary for us to inform them of what's going on. But that said, here's another idea:

When a user picks an image from an external media library, we display the "Insert" button...

Screen Shot 2020-06-03 at 10 59 52 AM

...Once user "inserts" the image, instead of displaying the image's loading state in a modal, can the loading state be displayed within the editor instead?

Screen Shot 2020-06-03 at 11 05 07 AM

So essentially, the copying (if we really can't do away with this) is still occurring behind the scenes, just that it doesn't happen in a modal and it's unbeknownst to the user. The preloading/copying state displays an enlarged, opaque thumbnail.

One question though and this might've been mentioned before - what currently happens if the user copies an image that does not exist? ie - it shows up as a thumbnail, but we're not actually able to obtain the hi-res version of it. Is this possible?

Thoughts? @Automattic/ajax @lcollette

@obenland
Copy link
Member

obenland commented Jun 3, 2020

instead of displaying the image's loading state in a modal, can the loading state be displayed within the editor instead?

Unfortunately not, Marek and I discussed that today as well (p1591201403131300-slack-ajax). We can't extend the way these blocks render their media.

what currently happens if the user copies an image that does not exist? ie - it shows up as a thumbnail, but we're not actually able to obtain the hi-res version of it. Is this possible?

I've not encountered that in my testing so far. I suppose we just trust these services that the data they give us maps to images that are actually available.

@obenland
Copy link
Member

obenland commented Jun 3, 2020

If there are no design improvements that come to mind, let's at least reduce the things that make the loading modal bad?

Should we remove the modal body and just keep the title with "Copying Media…" and a spinner?

@jancavan
Copy link

jancavan commented Jun 3, 2020

Even if that approach worked, it will only be usable for the image block. Not for any other block or different places where media are used. For example, the Featured Image control in the sidebar.

Can you please elaborate more as to why this is?

If there are no design improvements that come to mind, let's at least reduce the things that make the loading modal bad?

It isn't just the modal, but the entire process really. The user expectation would be that they're "inserting" an image, but the succeeding action says "copy." After that, the image lands in their media library, after which they'd have to insert it.

Another idea - for the button's default state to say "Insert", but as it's being copied over, it changes to "inserting..." (disabled state).

Screen Shot 2020-06-03 at 3 45 19 PM

@obenland
Copy link
Member

obenland commented Jun 3, 2020

Even if that approach worked, it will only be usable for the image block. Not for any other block or different places where media are used. For example, the Featured Image control in the sidebar.

Can you please elaborate more as to why this is?

Only the image block supports that functionality of uploading an image from a blob URL. The Featured Images component for example doesn't deal with how images get uploaded at all. It just want's an attachment ID.

Another idea - for the button's default state to say "Insert", but as it's being copied over, it changes to "inserting..." (disabled state).

Sure, we can try that. Do you think that'll be obvious enough?

After that, the image lands in their media library, after which they'd have to insert it.

I noticed your screenshots are from Calypso's external media integration, the one in Jetpack works a little differently. There the user never gets to see the media library and images get inserted directly into the blocks. I set up testing instructions in the Happiness heads-up post (p7DVsv-8FV-p2) if you want to give that a shot

@jancavan
Copy link

jancavan commented Jun 4, 2020

Sure, we can try that. Do you think that'll be obvious enough?

Obvious that the image is being inserted? I think so. Hopefully, it doesn't get stuck at that state for a long time.

the one in Jetpack works a little differently.

I'll check it out.

@lcollette
Copy link

Another idea - for the button's default state to say "Insert", but as it's being copied over, it changes to "inserting..." (disabled state).

I think what @jancavan's suggested here makes sense. What's the average amount of time it takes to copy and insert an image?

@obenland
Copy link
Member

obenland commented Jun 4, 2020

Sounds good!

What's the average amount of time it takes to copy and insert an image?

I'd say anywhere between 2s and 10s? It's hard to say. Copying five pexels images for a gallery took 22s in a test I just did.

@obenland obenland changed the title External Media: Improve External Media Copy/Insert Flow (8P) External Media: Improve External Media Copy/Insert Flow Jun 4, 2020
@obenland
Copy link
Member

obenland commented Jun 4, 2020

Todos:

  • Remove the existing Copying Modal
  • Add that spinner overlay to selected images as they get copied
  • Update the Insert button text to Inserting as images get copied
  • Add a polite speak() announcement that says the copying action is occurring and what to expect.

obenland added a commit that referenced this issue Jun 5, 2020
@obenland obenland linked a pull request Jun 5, 2020 that will close this issue
obenland added a commit that referenced this issue Jun 9, 2020
External Media: Improve Insert Flow

See #15867

Remove unused import

Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

Announce Inserting action along with alt text of selected images.

Update modal headers to reflect status

Props @jancavan.
obenland added a commit that referenced this issue Jun 9, 2020
* External Media: Improve Insert Flow

See #15867

* Remove unused import

* Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

* Announce Inserting action along with alt text of selected images.

* Fix mobile mobile width to take up the full available modal space

* Change is-copying to modifier instead of element to follow BEM syntax

Co-authored-by: Konstantin Obenland <obenland@gmx.de>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>
obenland added a commit that referenced this issue Jun 10, 2020
* [not verified] # This is a combination of 5 commits.

External Media: Improve Insert Flow

See #15867

Remove unused import

Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

Announce Inserting action along with alt text of selected images.

Update modal headers to reflect status

Props @jancavan.

* External Media: Fix mobile modal size (#16098)

* External Media: Improve Insert Flow

See #15867

* Remove unused import

* Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

* Announce Inserting action along with alt text of selected images.

* Fix mobile mobile width to take up the full available modal space

* Change is-copying to modifier instead of element to follow BEM syntax

Co-authored-by: Konstantin Obenland <obenland@gmx.de>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>

* External Media: Loading and disabled states for the copying state (#16103)

* disable pagination while loading

* fade out all images when copying

* disable event handlers and mark as disabled when copying

* disable pexels search when copying

* disabled google filters when copying

* add text to copying indicator

* disable google photos view selector when copying

* [not verified] remove debugging condition

* Pass functions not booleans to event emitters

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

* Removed :not() copying css state since they are the same modal now

* Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal.

* Cosmetically rearranging code order

* External Media: Screen Reader accessible states for Insert flow (#16113)

* Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state

* [not verified] Remove title attribute from grid images

* [not verified] Move Inserting Image... copy to before the image so it is announced first.

* Separate-out description and element id

Helps with keeping describedby id in sync

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
@obenland
Copy link
Member

Fixed in #16081.

obenland added a commit that referenced this issue Jun 24, 2020
* [not verified] # This is a combination of 5 commits.

External Media: Improve Insert Flow

See #15867

Remove unused import

Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

Announce Inserting action along with alt text of selected images.

Update modal headers to reflect status

Props @jancavan.

* External Media: Fix mobile modal size (#16098)

* External Media: Improve Insert Flow

See #15867

* Remove unused import

* Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

* Announce Inserting action along with alt text of selected images.

* Fix mobile mobile width to take up the full available modal space

* Change is-copying to modifier instead of element to follow BEM syntax

Co-authored-by: Konstantin Obenland <obenland@gmx.de>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>

* External Media: Loading and disabled states for the copying state (#16103)

* disable pagination while loading

* fade out all images when copying

* disable event handlers and mark as disabled when copying

* disable pexels search when copying

* disabled google filters when copying

* add text to copying indicator

* disable google photos view selector when copying

* [not verified] remove debugging condition

* Pass functions not booleans to event emitters

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

* Removed :not() copying css state since they are the same modal now

* Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal.

* Cosmetically rearranging code order

* External Media: Screen Reader accessible states for Insert flow (#16113)

* Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state

* [not verified] Remove title attribute from grid images

* [not verified] Move Inserting Image... copy to before the image so it is announced first.

* Separate-out description and element id

Helps with keeping describedby id in sync

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
obenland added a commit that referenced this issue Jun 25, 2020
* External Media: Render Media after authentication (#16044)

See #15717 (review)

* [not verified] Handle 401 responses (#16041)

See #15717 (review)

* External Media: Better reflect accepted media types (#16071)

* External Media: Account for multiple images (#16077)

See #16071 (comment)

* External Media: Fix Modal focus loss on arrow keypress (#16055)

Co-authored-by: Marek Hrabe <marekhrabe@me.com>

* External Media: Add API unit tests (#15988)

* [not verified] Add External media endpoint

* [not verified] Add unit tests

* [not verified] Add class coverage info

* Simplify schema

* Fix image copy test

* Use local image

* Remove duplicate file ending

Props @frontdevde.

* External Media: Improve Insert Flow (#16081)

* [not verified] # This is a combination of 5 commits.

External Media: Improve Insert Flow

See #15867

Remove unused import

Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

Announce Inserting action along with alt text of selected images.

Update modal headers to reflect status

Props @jancavan.

* External Media: Fix mobile modal size (#16098)

* External Media: Improve Insert Flow

See #15867

* Remove unused import

* Lock pointer events when copying
This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending.

* Announce Inserting action along with alt text of selected images.

* Fix mobile mobile width to take up the full available modal space

* Change is-copying to modifier instead of element to follow BEM syntax

Co-authored-by: Konstantin Obenland <obenland@gmx.de>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>

* External Media: Loading and disabled states for the copying state (#16103)

* disable pagination while loading

* fade out all images when copying

* disable event handlers and mark as disabled when copying

* disable pexels search when copying

* disabled google filters when copying

* add text to copying indicator

* disable google photos view selector when copying

* [not verified] remove debugging condition

* Pass functions not booleans to event emitters

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

* Removed :not() copying css state since they are the same modal now

* Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal.

* Cosmetically rearranging code order

* External Media: Screen Reader accessible states for Insert flow (#16113)

* Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state

* [not verified] Remove title attribute from grid images

* [not verified] Move Inserting Image... copy to before the image so it is announced first.

* Separate-out description and element id

Helps with keeping describedby id in sync

Co-authored-by: Konstantin Obenland <obenland@gmx.de>

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Bart <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>

* External Media: Restore focus on modal close (#16102)

* External Media: Disable for existing galleries (#16176)

* External Media: Disable for existing galleries
* Update comment

* External Media: Improve upload permission check (#16185)

* Use `value` to disable for existing galleries

`addToGallery` is not used by all gallery-type blocks, like tiled-gallery or slideshow.

Props @jeherve.

Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Extension] External Media Extend all block editor media tools to support external providers [Status] Needs Design [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants