If a filter wraps the drupal-media tag in a paragraph, the edit button breaks

👈 "someone else having exact same difficulty"
Yesssss thank you for writing it up and posting!!

From my own share on Slack -- *in addition to* what @MrPaulDriver explained above...

...When I have the "edit media" button (by doing what I described above) and I use it, and I choose a different view mode ("default", or a custom view mode), the view mode selection works (data-view-mode is updated in the source, and it "sticks" if I save and re-edit the page), but edit button goes away.

One other thought, inspired by the uninstall/reinstall thing:
Maybe something was borked bc the site was originally installed with 8.7.x (only a couple months ago -- it's a new site, still under development), then I updated to an 8.8.x-dev version back in early October, then alpha1 then beta1 -- that's when I realized the UI I had wasn't the complete UI (the issue didn't start with beta1, I just didn't know I was missing anything til then) -- and then I updated to rc1 earlier this week.
(I really don't know if this progression of versions has anything to do with anything, I'm just thinking "out loud.")

please clarify, you're in the 'media' queue for 'contrib' it's not the same as core 'media'

Are you using the media module included with 'core' or did you actually download the media module (not recommended) from contrib?

Please clarify and I will triage this appropriately.

Moving to drupal core queue , media subsystem.

@MrPaulDriver, what browser/OS is this? I wonder if it is a browser-specific thing we wouldn't have caught, since Drupal CI's automated tests run against Chrome.

I've experienced this issue in Firefox (70something) and Chrome (I don't remember the version, it's managed by my office, it's usually the latest version or close), on Windows 10.

I've also experienced this issue in Firefox and Chromium on Ubuntu 19.04.

If it helps I'm also getting a deprecation message on Safari prior to the same error above:

[Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.

I do not however get either error on a fresh install of Drupal via a tarball from Drupal.org so maybe I'm missing something or another module is affecting it adversely.

I narrowed down the issue. If you have Convert line breaks into HTML (i.e. <br> and <p>) enabled for the text format then it throws the error. If it is not enabled the edit button shows up and works just fine. As to why this is conflicting with that particular setting, I couldn't tell you. Although I have checked this with multiple different text formats and several of our platforms and this seemed to be what the issue was for us.

Nice sleuthing, @catapipper! Updating the issue summary with your findings.

Yup, this is unambiguously a bug. I'm going to escalate it to Major, since this conflict means that a common filter configuration can render the media embedding system unusable.

(also caught up in the Slack thread -- thank you everyone!) (but/and...)

........ugh I haaaaaate posting this, but, I don't have "convert line breaks into HTML" enabled, never have :(

But I'll poke at it a bit and see what I can see.

I feel like I'm awfully close to a reliable set of steps to reproduce, but I'm not quite there...

My "breakthrough" this afternoon was actually reproducing the problem on a Simplytest site -- feel free to poke around and tweak stuff if you'd like to try to figure it out!
https://stm5dead1ae49b8d-idmgreju1b10khuhb7ifj1tmknm2z8bn.tugboat.qa

For the most part, I've been toggling the "filter_autop" filter, moving the "filter_autop" filter around in the processing order, and tweaking the "media_embed" filter settings (which media types and view modes are enabled, which view mode is selected by default).

And, I grabbed the "allowed HTML" value from my own site (but removed the "entity_embed" filter stuff), along with one image style and image view mode, just like, to see what would happen.

............
As the simplytest.me site sits right now:

• (⚠️) filter_autop is enabled -- it's right before media_embed in the filter processing order.
• When I edit my one test page, I get the error and no edit button.
• If I switch to source and manually add a value for data-caption or data-view-mode and then switch back from source, the edit button is there (and no error).

But/And...

• (iirc...) If filter_autop is at the bottom of filter processing order and save (basic_html)...
• When I edit my test page, the edit button is there (and no error) -- but if I toggle "Source" mode (and back), the edit button is gone (and the error is there).

I went through a ton of other variations, but I can't remember all the details right now.

............
About me

Firefox 70.0.1
Ubuntu 19.04
Drupal 8.8.0

I don't remember how long, I feeeel like it's a certain amount of time after it goes idle, and I just refreshed it now, but good point -- I'll do a config export today just in case.

Mkay, I missed and lost it yesterday -- new environment:
https://stm5ded72e8535b8-0eudlkjyzgxfw9y0w6lqdeduk2qel3gh.tugboat.qa/node/1

Comparable situation:

• filter_autop enabled, it's second in filter processing order.
• If you edit the node, there's the error and there's no edit button -- if you switch to Source and add data-caption="hello caption" and then switch back out of Source, no more error and the edit button is there.

Aaaand attached is a full config export!

Adding credit for the work to reproduce this bug put in by @MrPaulDriver and @alisonjo315.

This is easy to recreate in Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest

Add 'filter_autop' => ['status' => TRUE], in the setUp() function.

And you'll get a failure:

1) Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testEditableCaption
Failed asserting that a NULL is not empty.

/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:117
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/Constraint/LogicalNot.php:123
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/Assert.php:2116
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/Assert.php:659
/Users/nathanandersen/dev/d8test/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:1357
/Users/nathanandersen/dev/d8test/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:369
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/TestCase.php:1071
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/TestCase.php:939
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/TestResult.php:698
/Users/nathanandersen/dev/d8test/vendor/phpunit/phpunit/src/Framework/TestCase.php:894

Same js error in the console.

plugin.js?t=q29cio:243 Uncaught TypeError: Cannot read property 'insertBeforeMe' of null
    at h._setUpEditButton (plugin.js?t=q29cio:243)
    at plugin.js?t=q29cio:174
    at Object.success (plugin.js?t=q29cio:321)
    at c (jquery.min.js?v=3.4.1:2)
    at Object.fireWith [as resolveWith] (jquery.min.js?v=3.4.1:2)
    at l (jquery.min.js?v=3.4.1:2)
    at XMLHttpRequest.<anonymous> (jquery.min.js?v=3.4.1:2)

You can see here how the filter_autop is not playing nice with the embedded media, and is corrupting the output.
Check out the childNodes, where they are getting p tags inserted inside.

This seems to fix it (at least in the context of the test).

Let'e see if anything else fails. I don't like that var embeddedMedia = embeddedMediaContainer.getFirst(isElementNode); can return a p tag. So I'm leaving it as needs work, for now.

Ideally, these extra p tags wouldn't get added by the other filter.

But we shouldn't call something embeddedMedia if it's the p tag in front of the embedded media, even if it works.

Looks like tests failed, but thanks for testing this @oknate!

Yeah, not what I was expecting. It passed locally. Let me see if I messed up the patch.

Good news. The failure in #23 was a copy and paste error. I forgot to delete the first line here:

embeddedMedia.getFirst().insertBeforeMe(editButton);
+          if (embeddedMedia.getFirst()) {
+            embeddedMedia.getFirst().insertBeforeMe(editButton);
+          }
+          else {
+            embeddedMedia.insertBeforeMe(editButton);
+          }

I like this approach better. We just ignore the media embeds in filter_autop. There's already a precedent for this with comments. See
_filter_autop():

$comment = (substr($chunk, 0, 4) == '<!--');
      if ($comment) {
        // Nothing to do, this is a comment.
        $output .= $chunk;
        continue;
      }

We can do the same thing for media embeds. I think it's inappropriate to be auto breaking lines. Especially where it's doing it. Where you have a tag like <drupal-media> and it's adding an empty p before the tag. That's not good.

Adding test coverage to FilterKernelTest::testLineBreakFilter().

Same as the last one, but providing a FAIL patch to show the test result in FilterKernelTest changes based on the fix.

Sorry, the patch in 29 was the same as the fail patch inadvertently. I just need to re-upload 28 again.

This patch has the fix and the test coverage. This is the patch to review.

Woah, so exciting, thank you @oknate! I'll plan on trying it tomorrow, unless someone beats me to it.

I think we should probably do the js change here, there might be other filters that do auto-paragraphs with a similar issue to filter_autop around.

Thanks, @catch. Let's proceed with the approach from #26.

+++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
@@ -337,7 +337,13 @@
+          if (embeddedMedia.getFirst(isElementNode)) {
+            embeddedMedia.getFirst(isElementNode).insertBeforeMe(editButton);
+          }
+          else {
+            embeddedMedia.insertBeforeMe(editButton);
+          }

This is a bit repetitive, and it could use a comment explaining what we're doing here. How about something like this:

// Add a comment here explaining this logic
const firstElement = embeddedMedia.getFirst(isElementNode);
if (firstElement) {
  firstElement.insertBeforeMe(editButton);
}
else {
  embeddedMedia.insertBeforeMe(editButton);
}

I realize now that there's no real reason we put the edit button inside first element within the embedded media wrapper. It seems more robust to just put it within the wrapper. Then whether the first element is a `p` tag or the `article` element doesn't matter. I also tested this with #3092795: [regression] Restore styling for embedded media edit button in Seven theme and made sure the styles still work, when that patch is applied with this one.

I created a follow-up issue for the drupal-media tag and the filter: #3100066: "Convert line breaks into HTML" filter should exclude <drupal-media> tag. I agree that it would be more robust to handle it both within the plugin.js and in the "Convert line breaks into HTML" filter.

#47 seems to work for me on 8.8.0, with or without filter_autop.

Notes:

• FWIW, I didn't try the patches before #47.
• FWIW, I have media_library.settings:advanced_ui set "true."
• I didn't test with embedded video, only an embedded image.
• data-align is getting automatically set to "center" when I embed the image, which seems weird to me -- but probably not related to this issue, only mentioning "just in case."

Thank you, @oknate!

Crap, I'm sorry, the issue metadata field values were "out of date" in my open browser tab, I hate when I forget to do a fresh tab before posting a comment...

(Changing status back to "needs review," but it won't let me change the assignment back to oknate.)

Assigning back to myself, if any further changes are needed. Thanks for the review, @alisonjo315!

Modify the client-side JavaScript for the media embed filter so that it is able to anticipate the P tag added by filter_autop.

-1

We should fix filter_autop.

If other filters wrap tags in <p> tags, then those filters need to be fixed too. Core JS shouldn't defend against arbitrary crazy things filters could do. The same problem would arise if it were getting wrapped in <div> or <custom> tags. We wouldn't be defending against those either.

Besides, most Drupal 8 sites use CKEditor, and hence don't use filter_autop.

In fact, using a text format that has filter_autop enabled and uses CKEditor will result in weird behavior. That's why Drupal 8's basic_html text format does not use filter_autop, unlike its predecessor (the filtered_html text format in Drupal 7/6/5).

Thanks for the feedback, Wim. Now that #3100066: "Convert line breaks into HTML" filter should exclude <drupal-media> tag is RTBC, we may not need this issue, at least to fix this particular issue, i.e. p tags wrapping the drupal-media tag.

Although, the current patch's solution will handle your counterexample too, so it provides additional stability:

The same problem would arise if it were getting wrapped in

or tags

.

The solution puts the edit button directly inside the widget, rather than as a child of the first grandchild of the widget, which is kind of brittle. We were looking for the first element's first element, and it broke because the first element is empty. So this solution will make the edit button placement less brittle. So I think, even if 3100066 is committed, this issue's current patch should still be considered.

Are you saying the patch here would end up not being used, in favor of #3100066: "Convert line breaks into HTML" filter should exclude <drupal-media> tag, so we should test the other patch for fixing the edit button issue...? (It doesn't feel to me like the other issue would fix the edit button issue, but)

Yes, I think we should use that other patch to fix the issue. Then we can leave this one open, if we decide it's possible another filter does the same thing, i.e. wraps the drupal-media tag in markup.

Iiiiiinteresting, ok........ Adding a note to the issue summary. I'll be following closely :) Thanks!

Now that the other issue has landed, this is less urgent (#3100066: "Convert line breaks into HTML" filter should exclude <drupal-media> tag).

I can see some value in fixing this if something else breaks the markup, but perhaps we should wait to see if anyone else has the issue.

I think the JavaScript change would make the plugin more robust, but if we commit it now, we're fixing a problem that no longer exists. So let's leave this open for now, and see if anyone encounters it from an interaction with a different filter plugin.

Changing the title to de-emphase filter_autop.

Drupal 8.8.2
The filter_autop filter (Convert line breaks into HTML) is disabled for the editor profile

Still having the js error when adding a media entity into the editor (and no edit media button appears)

Uncaught TypeError: Cannot read property 'insertBeforeMe' of null
    at h._setUpEditButton (plugin.js?t=q5n7m0:242)
    at plugin.js?t=q5n7m0:174
    at Object.success (plugin.js?t=q5n7m0:320)
    at c (jquery.min.js?v=3.4.1:2)
    at Object.fireWith [as resolveWith] (jquery.min.js?v=3.4.1:2)
    at l (jquery.min.js?v=3.4.1:2)
    at XMLHttpRequest.<anonymous> (jquery.min.js?v=3.4.1:2)

I'm seeing this issue, strangely enough though -- on my DEV/TEST environment.

But it works as expected on my localhost.

localhost is a lando environment

DEV/TEST (and eventually PROD) are on Pantheon.

Uncaught TypeError: Cannot read property 'insertBeforeMe' of null
    at h._setUpEditButton (plugin.js?t=q5viq0:242)
    at plugin.js?t=q5viq0:174
    at Object.success (plugin.js?t=q5viq0:320)
    at c (js_bB3-IMAzWNovMKR3MvXVSqQbdMS3KDuJ11jrLJb-efs.js:2)
    at Object.fireWith [as resolveWith] (js_bB3-IMAzWNovMKR3MvXVSqQbdMS3KDuJ11jrLJb-efs.js:2)
    at l (js_bB3-IMAzWNovMKR3MvXVSqQbdMS3KDuJ11jrLJb-efs.js:2)
    at XMLHttpRequest.<anonymous> (js_bB3-IMAzWNovMKR3MvXVSqQbdMS3KDuJ11jrLJb-efs.js:2)

For me this Javascript error is present (and the "Edit media" button is missing) when the filter "Limit allowed HTML tags and correct faulty HTML" is configured to be AFTER the "Embed media" filter.

I came across this issue and it turns out, if you remove the <div> or other element surrounding {{ content }} in your media.html.twig file, you will get this error/issue. Adding a div back into the template around {{ content }} solved it for me.

#61 fixed it for me – thank you!

#61 does indeed fix the issue. If the media item isn't wrapped in another element (article, div, etc.), the edit button doesn't show. As soon as it's wrapped, it shows. The core 'Stable' theme wraps media in an article element.

I applied the patch at #47 anyway, as changing my templates late into a project broke things. The patch applied correctly on 8.9.1, though only worked with the Convert line breaks into HTML (i.e. <br> and <p>) option enabled.

#61 also fixed it for me. This is annoying as I prefer to strip empty wrapping div's. Luckily, I already use Role Theme Switcher to provide a child-theme of the main theme to anonymous users. For the anonymous theme, I added my custom template that removes empty div's while the authenticated main theme added this empty div back to the media.html.twig file.

This is really not ideal that an empty div is required for embedded inline media in CKeditor to be editable, and it is not documented outside this issue from what I've found.

We need to figure out how to proceed here; at a quick glance, it looks like this issue stalled due to disagreement about implementation. Let's figure out what to do, and fix it, because this degrades the authoring experience.

Here is a reroll of #47 for Drupal 9.1.
Drupal assumes the media is wrapped in something but this is not the case in our theme.

I applied the #68 patch, which created some UI issues.

+1 for patch #68

Reroll of #68 for D9.5

Can confirm that #76 worked for me to bring back the edit button in Drupal 9.5.2. Most likely unrelated to the patch however is now my selection of alignment / style only works on the initial save. When trying to edit the item a second time and change values the changes are not being saved.

@choneyse it sounds like you're suffering from this bug: #3102249: Changing an existing embedded media's alignment or alt data attributes does not get saved with CKEditor5

Pushing to MR, hiding files.
Thanks everyone for commits 🍻

From what I can tell the MR 3297 is just the patch from #76 no?

As a bug this will need a test case

I was having a same issue and it worked by following the comment https://www.drupal.org/project/drupal/issues/3097338#comment-13371176

Still it's work around as it may impact the frontend on our project as vide mode needs to be updated.

Applying the patch #76 would at least provide the Edit Media button without affecting view mode. Also, not getting any issue with saving the alignment, alt text and caption. The only issue is with the edit media button styling. Sometime appears on left, sometimes at the bottom, sometimes at top while editing the media

Re-rolled the patch from https://www.drupal.org/project/drupal/issues/3097338#comment-13385746 into the ckeditor module here: https://www.drupal.org/project/ckeditor/issues/3342759#comment-15176900

Since this patch was from 2019, it no longer worked with the updated drupalmedia directory location.

Can this bug be reproduced on a clean Drupal 10 install, i.e. with CKEditor 5? I don't think so. At least on my test instance I'm able to change the alt and alignment of the embedded media thanks to the new CKE5 UI.

As CKEditor 4 is now contrib and unsupported, I guess this issue should be in CK4 contrib and not in core?