volto icon indicating copy to clipboard operation
volto copied to clipboard

Fix issue of duplicated blocks upon pasting an image into the Slate E…

Open aryan7081 opened this issue 1 year ago • 9 comments

Fixes Issue

Inside the onImageLoad function in withDeserializers extension of Volto Slate. uploadContent adds the block using Redux and FormData, but the .then promise callback of uploadContent also adds a new Node in the Slate Editor using Transforms.insertNodes(editor, image); which eventually leads to adding multiple images in the editor.

aryan7081 avatar Mar 01 '24 11:03 aryan7081

Deploy Preview for volto canceled.

Name Link
Latest commit 138a634059a0254ba957ca6f0e11a78b60ffc044
Latest deploy log https://app.netlify.com/sites/volto/deploys/65e9ee11d325e500086f471f

netlify[bot] avatar Mar 01 '24 11:03 netlify[bot]

Deploy Preview for plone-components canceled.

Name Link
Latest commit b36387acb0c3c358d226bf4bece2d4277908df0f
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/666d49be95110e00089759de

netlify[bot] avatar Mar 01 '24 11:03 netlify[bot]

@plone/volto-team review requested.

stevepiercy avatar Mar 01 '24 13:03 stevepiercy

@aryan7081 A couple of extra context as upon further testing I noticed the following:

  1. copy and pasting an image from the website or from the internet results in the correct behavior regardless of your change
  2. The code that you disabled indeed prevents 2 images from being uploaded when pasting from the os (MAC OS Sonoma in my case)
  3. That is not enough to prevent the slate empty block from being added or from the image being inserted in the first slate block when pasting from the os clipboard for an image copied from Finder app. It should be tested also in other os systems such as Linux or Windows

ichim-david avatar Mar 02 '24 08:03 ichim-david

@ichim-david Sorry for the delay. Working on this and adding tests as well. Can I test this on linux and windows vm?

aryan7081 avatar Mar 05 '24 13:03 aryan7081

@ichim-david Sorry for the delay. Working on this and adding tests as well. Can I test this on linux and windows vm?

@aryan7081 do the minimum test first on whatever platform you're working on and if you can reproduce the issue there then try to fix it, there is no point in doing cross-testing until necessary.

ichim-david avatar Mar 05 '24 13:03 ichim-david

The createImageBlock function within packages/volto-slate/src/utils/volto-blocks.js currently utilizes addBlock to insert a new block at the index position plus one. However, the addBlock function adds multiple blocks: one for the block being added and another empty block if the type is not 'slate'. To address this, I've attempted to resolve the issue by modifying the createImageBlock function as follows:

const currBlockId = properties.blocks_layout.items[index];
const currBlockHasValue = blockHasValue(properties.blocks[currBlockId]);
let id, newFormData;
if (currBlockHasValue) {
  [id, newFormData] = addBlock(properties, 'image', index + 1);
  newFormData = changeBlock(newFormData, id, { '@type': 'image', url });
} else {
  [id, newFormData] = insertBlock(properties, currBlockId, { '@type': 'image', url });
}

This change ensures that a block is inserted at the (index - 1) position if the current(index) block is empty. Otherwise, it proceeds to create another block as usual. Please let me know your thoughts or any further adjustments needed.

Will push cypress tests once this approach is validated.

https://github.com/plone/volto/assets/71966131/cb836cb1-c490-4525-b06a-015b2e1442da

aryan7081 avatar Mar 07 '24 16:03 aryan7081

@aryan7081 please run code quality checks locally before pushing commits. See Linting.

However our documentation lacks the specific commands that need to be run, so I don't blame you for not running undocumented commands. See https://github.com/plone/volto/issues/5341

The CI jobs call https://github.com/plone/volto/blob/main/.github/workflows/code-analysis.yml. You can look at it for the commands that do not pass the checks, and try running them locally.

stevepiercy avatar Mar 08 '24 01:03 stevepiercy

@stevepiercy Working on cypress tests.

aryan7081 avatar Mar 08 '24 06:03 aryan7081

@plone/volto-team This works fine for me, pity is that we don't have any Cypress test for it. I don't even know if it's possible to model that interaction I've investigated it a bit this morning, we have clipboard mocks for pasting text but I have no clue how to mock the image data. I've tried though, if someone wants to take over:

https://github.com/plone/volto/tree/testingpasteimages

For the rest I'd merge it.

@ichim-david I'd appreciate another review from you!

sneridagh avatar Jun 15 '24 09:06 sneridagh

@sneridagh will have a look at this tonight.

ichim-david avatar Jun 15 '24 10:06 ichim-david

@sneridagh Tonight ended up this morning :) Tested it again and there is no more duplication of images but I still find some strange behavior or some differences in behavior at least.

Pasting from os clipboard will upload and use an image block. Pasting from the same website or other websites will add an img tag with the link to that source. Ex: image-block img-tag-external same-web-image

Not saying that it's the fault of this ticket. It just seems to me less useful this img tag linking since you don't have control over size or alignment like you have when it's uploaded through the image block.

ichim-david avatar Jun 16 '24 06:06 ichim-david

@ichim-david paste from a website you mean copy the html with along text or only the image? Is this different than before?

sneridagh avatar Jun 18 '24 08:06 sneridagh

@ichim-david paste from a website you mean copy the html with along text or only the image? Is this different than before?

@sneridagh right click copy image only. It's not different from before. I guess it would be useful to copy paste alongside text otherwise it's better to drag and image from desktop to get proper image block usage alongside upload and image options.

Ready for merging from my part.

ichim-david avatar Jun 18 '24 17:06 ichim-david