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

Images → Gallery transform fails for ID-less images #3698

Closed
2 tasks
mcsf opened this issue Nov 28, 2017 · 10 comments
Closed
2 tasks

Images → Gallery transform fails for ID-less images #3698

mcsf opened this issue Nov 28, 2017 · 10 comments
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@mcsf
Copy link
Contributor

mcsf commented Nov 28, 2017

Issue Overview

Steps to Reproduce (for bugs)

  1. a. Paste content that includes multiple images in sequence (either from the front-end or from the classic editor) into an empty Paragraph block, or
  2. b. Paste content that includes images into a Classic Text block, then convert it to blocks
  3. Select multiple of those newly created image blocks
  4. Transform to Gallery

Expected Behavior

The selected images show up as a gallery block containing all of the original images.

Current Behavior

An empty gallery shows up.

Possible Solution

The conversion process in steps 1. a. / 1. b. results in image blocks which are missing the ID attribute:

<!-- wp:image -->
<figure class="wp-block-image"><img src="http://example.org/wp-content/uploads/2017/11/example.jpg" alt="" /></figure>
<!-- /wp:image -->

The Gallery transform obviously needs these IDs, hence the failure.

Mutually compatible solutions include:

  • Expanding the transforms API to be able to express certain requirements, whose failure to meet means that a transform shouldn't appear in BlockSwitcher; e.g., disable Image → Gallery transform if any image is missing its ID.
  • Improve the Classic → Blocks / Image conversion so that Gutenberg can detect if an image belongs to the site's media library; if so, add its ID during conversion.
  • Improve the Gallery block and transform so that it can handle ID-less images.

Screenshots / Video

gutenberg-convert-regression-gallery

Related Issues and/or PRs

Todos

  • Tests
  • Documentation
@mcsf mcsf added [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Nov 28, 2017
@mcsf mcsf changed the title Images → Gallery transform fails for certain images Images → Gallery transform fails for ID-less images Nov 28, 2017
@tg-ephox
Copy link
Contributor

tg-ephox commented Nov 30, 2017

An uploaded image has an ID? If the image is on a remote sever then this will not be possible without a CORS proxy. Image Block does something similar on cDM but would need to fetch the image instead (via CORS proxy). We could detect that the image is on a remote server and show an appropriate error?

@mcsf
Copy link
Contributor Author

mcsf commented Nov 30, 2017

An uploaded image has an ID?

An image uploaded using the controls offered by both Image and Gallery blocks will, once pushed to the media library via utils#createMediaFromFile, be assigned an ID.

We could detect that the image is on a remote server and show an appropriate error?

Perhaps, but long-term it'd be great to support external images in general—thus presumably ID-less—more gracefully for both Image and Gallery blocks. This wouldn't be an issue most of the time, as the canonical flow would still require images to be uploaded to the media library; one thing to improve is to allow uploading via URL, as seen in the classic editor's media modal:

screen shot 2017-11-30 at 12 21 53

However, non-canonical flows like freeform-to-blocks conversion and clipboard-to-blocks conversions will still exist, and this is where we have to deal with external images.

  • Of course, one way out of it would be to automatically upload to the library any images found during block conversion. Given WP's tradition of handling HTML content in an non-opinionated fashion, the implications of automatic uploads may be an immediate dealbreaker. I honestly have no idea.

  • The other way is thus to make Image and Gallery support both ID-ful and ID-less images. The hard part is to not make it a complete mess in terms of design / cognitive load for the user. Right now, the edit action for both blocks brings up the media modal, a single mode of interaction for a single source of media; beating that simplicity is hard.

Looping in @jasmussen and @mtias to ponder.

@jasmussen
Copy link
Contributor

Some thoughts:

  1. How often is an end user likely to encounter this particular flow? It's common if you use the demo content in Gutenberg, where we inserted external images. It would certainly also happen if someone manually entered an image URL in the code editor, or if someone inserted using the "URL" tool as you screenshotted. But is this common?

  2. If it is common enough that we feel this is an issue that should be addressed, then converting three images into a gallery should "just work" — whether through uploading them in the background, or by making the gallery support images without an ID. Perhaps I'd lean towards the latter, because there might be a future where the editor had better offline support, in which case background uploading might cause headaches (though I suppose in that situation you wouldn't see external images anyway) ¯_(ツ)_/¯

  3. If we decide that this flow is not common, then we should simply fail gracefully. This could be one way to do so:

  • select 3 images, one of them is an external image without an ID
  • convert to gallery
  • result: the 2 images now form a gallery, the 3rd is untouched, and we throw an orange-colored notice informing that "The transformation failed for all images, because one image is hosted externally".

@mcsf
Copy link
Contributor Author

mcsf commented Nov 30, 2017

  1. is this common?

My take is that we support pasting of unknown (HTML) content, which includes image handling. It's not the main flow, but I also wouldn't consider it a niche one. That said, I can't even start to quantify how common it is. :)

  1. If it is common enough that we feel this is an issue that should be addressed

My point is more that we should be aware of this limitation. As we move forward, actual usage of Gutenberg should inform how to address it. Right now, I'm fine if we ignore it, or if we handle it in a basic way—cf. graceful degradation you described in 3).

Perhaps I'd lean towards the latter, because there might be a future where the editor had better offline support, in which case background uploading might cause headaches (though I suppose in that situation you wouldn't see external images anyway) ¯_(ツ)_/¯

External images doesn't sound like the greatest showcase for offline abilities ¯_(ツ)_/¯, but 💯 for keeping an eye out for offline in general.

@ellatrix
Copy link
Member

I would love it if we could get rid of linking external images directly in the image block. It can still be allowed in a custom HTML block of course, if the user would really want that. There could even be a special explicit block for this. (Thinking "Embedded Image" as it works more or less like an embed.) We already don't allow it in galleries, so it would make sense to not allow it in the image block either? If we do detect an image without ID, we should upload it.

I think auto uploading is particularly useful in the case of pasting or in some cases drag and drop. The user has no idea that, after paste, the image still links to the site they pasted from, or they have no idea what the difference is. I have seen people adding pictures from Facebook to their website as external images (by accident). It worked, so that's all they care about. But when the links at Facebook change, or the original is deleted, the image on the website will be gone as well. Maybe irreversibly so. I think it would therefore be good to always upload, unless the user knows what they are doing in a custom HTML block or similar.

We will need to do this in some cases anyway. When pasting from Google Docs, we'll need to upload the pasted images from the Google CDN. We can't just leave them like that.

@ellatrix
Copy link
Member

Would be nice if the REST API supports uploading by URL pointing to a valid resource. At the moment there is media_sideload_image (which probably needs to be generalised then later for other media).

@ellatrix
Copy link
Member

Surprisingly, media_sideload_image has no uses at all in core...

@mtias mtias added the [Feature] Media Anything that impacts the experience of managing media label Mar 6, 2018
@mtias mtias modified the milestones: Merge Proposal, WordPress 5.0 Mar 6, 2018
@mtias mtias added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Jul 9, 2018
@mtias mtias modified the milestones: WordPress 5.0, Try Callout Jul 12, 2018
@mcsf
Copy link
Contributor Author

mcsf commented Jul 18, 2018

Related: #1451, #5760

@pento
Copy link
Member

pento commented Jul 25, 2018

I'm moving this issue out of the Try Callout milestone, as blockers for that milestone are primarily concerned with behaviour when converting an existing classic post to blocks.

The only way I've found to reproduce this when converting to blocks is to add the images in Classic, then remove the wp-image-{id} class from each of them, then open in Gutenberg and convert to blocks.

Figuring out a way to deal with that particular flow would be nice, but it isn't necessary for the Try Callout, particularly considering that the Classic Editor stops linking the image to it's media library entry when the class is removed, too.

@pento pento modified the milestones: Try Callout, WordPress 5.0 Jul 25, 2018
@antpb antpb removed this from the WordPress 5.0 milestone Oct 5, 2018
@aaronjorbin
Copy link
Member

Based on @pento's comment about the only way to reproduce it, I think this can be closed. If a way to reproduce it that is more commonly found is known, please comment and let's reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants