Problem/Motivation

Toggling the source button when you have embed code inside a link leads to an extra p tag being added after the a tag containing the embed code. See the video.

Steps to reproduce:
1) embed an entity
2) click source button
3) wrap the embed code in an a tag, this mimics the behavior of the drupallink plugin.js
4) click the source button repeatedly

expected: the p tags after the a tag with the dupal-entity tag within it do not change
actual: an extra p tag is added each time the preview is re-rendered.

Here's what I think might be happening: the preview functionality returns a link and places it inside the drupal-entity tag, at which point CKEditor detects the link inside a link, and fixes it by moving the top level link to just after the tag it was wrapping, since you can't have a link inside a link. At this point, the auto paragraph features wraps that in a paragraph tag:

<p><a data-cke-saved-href="https://www.wikipedia.org" href="https://www.wikipedia.org"> </a></p>

And then when you click the source button again, some other process removes the empty link, replacing the paragraph's contents with a non-breaking space!
At the same time (when you click the source button), the widget is downcast, putting the link back as a wrapper, and now you have the link around the drupal-entity again and an extra paragraph after the widget.
If you keep toggling the source button, the process keeps repeating.

Proposed resolution

TBD

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 entity-embed-p-tag-generation-fix-3070941-14.patch1.61 KBthaddeusmt
#6 entity-embed-p-tag-generation-fix-3070941-6.patch1.48 KBoknate
extra_p_tags.mov1.24 MBoknate

Comments

oknate created an issue. See original summary.

oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
StatusFileSize
new entity-embed-p-tag-generation-fix-3070941-6.patch1.48 KB

Here's an initial patch that stops the endless generation, but it's not the right solution, I'm sure. But it gets the ball rolling.

First off, it seems wrong if the widget is wrapped in an a tag to pass that to the preview route, so I'm updating the code sent to the preview route to strip off the a tag. if you have and you insert the link again into the tag, that's just wrong. The a tag should not be sent to the preview route and added back inside the drupal-entity element.

           var widget = this;
+          var previewElement = this.downcast();
+          // If element is wrapped in an a tag, remove the a tag.
+          if (previewElement.name === 'a' && previewElement.getFirst().name === 'drupal-entity') {
+            previewElement = previewElement.getFirst();
+          }
           jQuery.get({
-            url: Drupal.url('entity-embed/preview/' + editor.config.drupal.format + '?text=' + encodeURIComponent(this.downcast().getOuterHtml())),
+            url: Drupal.url('entity-embed/preview/' + editor.config.drupal.format + '?text=' + encodeURIComponent(previewElement.getOuterHtml())),

Next, the above code doesn't seem to do it, so I threw in this hack to remove the extra element:

           }).done(function(previewHtml) {
             widget.element.setHtml(previewHtml);
+            var next = widget.element.getParent().getNext();
+            if (next && typeof next.getName === 'function' && next.getName() === 'p') {
+              var child = next.getFirst();
+              if (child && typeof child.getName === 'function' && child.getName() === 'a') {
+                if (child.getText() === " ") {
+                  next.remove();
+                }
+              }
+            }
             callback(widget);

This checks for the extra p tag with an empty link in it after the preview is added. This works, but is obviously a hackish bandaid that is treating the symptom not the source of the problem. Also I think this could be improved using xpath.

I'm not sure exactly at what point this extra p tag with the link in it is added. If it's added in this done function, it may make sense to remove the a tag, then insert the preview, and then add the a tag back. That's a strategy I haven't tested yet.

oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Embed wrapped in a tag + toggling source button creates extra p tags » [upstream] Embed wrapped in a tag + toggling source button creates extra p tags

This is the kind of edge case that requires super deep CKEditor knowledge. Let's ask the CKEditor team to provide guidance on this. I remember there being lots of special snowflake browser things that CKEditor needs to work around, and if I remember correctly automatically generating <p> tags is related to that.

Pinged @kkrzton from the CKEditor team.

kkrzton’s picture

kkrzton commented

Hello @wim-leers and @oknate!

Glad to see you are making progress on the plugin. This one is kind of complex due to few reasons. First of all I created a codepen sample which reproduces this issue so one (mostly me) can easily experiment (you can see the issue is reproducible there). Also, a similar issue was already reported (apparently by @wim-leers) some time ago - the end result is the same, however the case is a little different.

Now, the cause of this issue is a mix of some specific behaviours and assumptions of CKEditor. These are there to help CKEditor create and manage content which is clean and semantic.

Top-level elements can be block elements only

The assumption is that the top-level elements can only be block ones to keep proper and semantic structure of the content. For that reason DTD (CKEDITOR.dtd) was introduced which defines the structure, elements groups and how they can be nested. As mentioned, only block elements can be top-level, which means any inline ones (doesn't matter how they get there) will be modified - which in most cases means wrapping them into default block element which is a paragraph.

Empty elements are removed

Since empty elements are not visible and also caret cannot be placed inside (that's the reason empty paragraphs have &nbsp inside) they are removed as considered useless from editing point of view (ofc there are some cases when one would like to keep them, but that's another story).

What's going on here?

It's similar to what @oknate described in the issue report, but a little different. When switched to the source mode,
drupal-entity is transformed into <a ...><drupal-entity ...></drupal-entity></a>. Upon switching back to WYSIWYG mode:

1. The <drupal-entity></drupal-entity> part is upcasted to widget. Since it cannot be placed inside a link it is extracted and moved before the link in the DOM structure (not sure exactly why it happens as there is dtd['a']['drupal-entity'] = 1; - I assume widget is first transformed into div element and then DTD checks it).
2. Left link is an inline element at the content top-level, it's not allowed there so it's wrapped into paragraph.
3. Now since paragraph contains empty link element - the mechanism removing empty elements kicks in removing empty link.
4. As mentioned earlier, empty elements are not selectable (the paragraph from which link was removed), so &nbsp; is added inside paragraph.

When linked drupal-entity widget is downcast it gets wrapped into a link (a element), however on upcast, only the `drupal-entity` element is upcasted - the wrapping a is left untouched.
Let's say upcasting widget will leave the widget inside the a element - after every switch between source/wysiwyg mode you will get widget wrapped in an additional a element (since these are not removed and on every downcast new is added). Now because of how things works in CKEditor you get new p element instead of an additional a.

Possible solutions

Defining a as a block element - it still behaves in a similar manner, widget upon upcast is extracted, however a is not wrapped into a paragraph. It's still left empty and removed - it fixes the issue, however may have strange side effect (`a` is now a block element) - https://codepen.io/f1ames/pen/oKeQyj?editors=0010.

Wrap linked widget into a additional element - you can wrap it into additional div. This way a will not be a top-level element and will be simply removed as empty element not being wrapped into paragraph. Not perfect solution as it requires changing existing output HTML structure.

Upcast link element instead of drupal-widget element - I believe this is a proper solution. Since wrapping link element is a part of the widget it should be upcasted as a most outer widget element. Upcasting not the top widget element results in
some leftovers (like in this case) which may have strange side effects, see implementation on https://codepen.io/f1ames/pen/JgyZam?editors=0010.

Hope I shed some light on this issue, so you can better understand what's happening and try to solve it ;)

oknate’s picture

oknate
Location Greater New York City Area
commented
Title: [upstream] Embed wrapped in a tag + toggling source button creates extra p tags » [upstream] Embed wrapped in an a tag + toggling source button creates extra p tags
emek’s picture

emek commented

We have encountered the same problem on our sites. For me the error occurs as long as I have a linked image and then edit some other text in the wysiwyg and save.
The patch seems to fix it, but it seems like you maybe don't think it's enough? Is it someone working on this or have some ideas on what's left to do?

thaddeusmt’s picture

thaddeusmt commented
StatusFileSize
new entity-embed-p-tag-generation-fix-3070941-14.patch1.61 KB

This patch was working to fix this for us, but the latest release prevented the patch from applying, so I did a simple re-roll against the latest 8.x-1.x branch so I can re-apply it. It auto-merged just fine. This is my first time trying something like this, let me know if I am out of line on anything. Thanks.

annerl’s picture

annerl commented

My problem is very similar: when embedding a node as teaser, the embedded entity gets surrounded by empty paragraphs. Like this:

<p></p>
<article>embedded entity</article>
<br> 
</p>

I testet patch #14 in Drupal Core 9.2.7. But this didn't fix the problem.

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

See #3131055-26: Wrapping drupal-media image in a link produces empty P tags after image for some steps which may fix this problem from Drupal 9.4/10.0 onwards by using the new CKEditor 5.

I was not able to text entity_embed, so if someone wants to try the same steps I followed to confirm if switching to CKEditor 5 does indeed fix this problem then unless this issue is resolved before the release of 9.4/10.0 perhaps this could be closed as a won't fix.

thaddeusmt’s picture

thaddeusmt commented

The patch proposed in #3282295: Additional paragraphs added when embedding an entity in CKEditor and linking it by @Graber (a ckeditor config option, actually) seems to be working for me.