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

[Auto embed] Wrong insertion position when using the context menu #2757

Closed
oleq opened this issue Sep 14, 2018 · 8 comments · Fixed by ckeditor/ckeditor5-media-embed#68
Closed
Assignees
Labels
package:media-embed type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oleq
Copy link
Member

oleq commented Sep 14, 2018

  1. <h1>Foo</h1>[]<h1>Bar</h1>...
  2. Paste.

Expected

<h1>Foo</h1><media><h1>Bar</h1>...

Actual

<h1>Foo</h1><h1>Bar</h1><media>...

kapture 2018-09-14 at 11 28 51

It could be related to https://github.com/ckeditor/ckeditor5-media-embed/issues/12 and #1243. But the strange thing is it works fine when Cmd+V is used instead.

cc @pjasiun @jodator @pomek

@Reinmar
Copy link
Member

Reinmar commented Sep 14, 2018

My guess:

  1. Right-clicking makes a non-collapsed selection <h1>Foo[</h1><h1>]Bar</h1>.
  2. Paste deletes that selection so both blocks get merged.
  3. But then... I don't get why autoembedding happens in a different place than the link was.

@Reinmar
Copy link
Member

Reinmar commented Sep 14, 2018

If this is seriously impairing embedding via paste via ctx menu, then you can raise this ticket to one of the current iterations.

@jodator
Copy link
Contributor

jodator commented Sep 14, 2018

My guess is that right-click/contex menu changes selection... also I wonder how this will work with #1243.

@pjasiun
Copy link

pjasiun commented Sep 14, 2018

  1. But then... I don't get why autoembedding happens in a different place than the link was.

Then, the paste occurs in the block and we do not break the block in such case but insert block after (or before).

I think that insert content needs to be smarter than just: delete content + insert in the new position, to handle cases like this. This is a different case than inserting content with the collapsed position inside a block.

@Reinmar
Copy link
Member

Reinmar commented Sep 14, 2018

Then, the paste occurs in the block and we do not break the block in such case but insert block after (or before).

But autoembedding (IMO) should happen directly where you paste a link. It should not use that smart insertion mechanism, IMO.

How does it work now?

@pjasiun
Copy link

pjasiun commented Sep 14, 2018

But then the block is inserted and it uses this smart insertion mechanism. I think it should be applied for dropping.

We tested it for images and it is hard to set selection precisely. It would be very easy to break a paragraph even if you do not want to if we would allow paragraph breaking on paste. This is why I think that the mechanism should be applied and do not break the paragraph.

However, it should work only for collapsed selection, and if it is true what you said that in this case the selection is not collapsed, we need to fix such selection too and put the collapsed selection between blocks.

@Reinmar
Copy link
Member

Reinmar commented Sep 14, 2018

But this ticket is about autoembed. When you paste a link in, usually, a very precisely chosen location. It'd be weird that you have such a content:

<p>Foo. []Bar</p>
<p>Bom</p>

You paste a link so you see this for a second:

<p>Foo. https://youtube.com/whataver[]Bar</p>
<p>Bom</p>

And suddenly you get this:

<p>Foo. []Bar</p>
<media></media>
<p>Bom</p>

@scofalik
Copy link
Contributor

scofalik commented Oct 4, 2018

I +1 the suggestion that auto-embed should be done in-place. This seems like a natural and logical solution.

@jodator jodator self-assigned this Dec 12, 2018
oleq referenced this issue in ckeditor/ckeditor5-media-embed Jan 4, 2019
Fix: The `AutoMediaEmbed` feature should insert media in place of a pasted link. Closes #36. Closes #49.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-media-embed Oct 9, 2019
@mlewand mlewand added this to the iteration 22 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:media-embed labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:media-embed type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants