<drupal-entity> tag not being automatically added when an Entity Embed button is added to active editor toolbar

http://docs.ckeditor.com/#!/guide/widget_sdk_tutorial_1-section-adjustin... has part of the answer.

In js/plugins/drupalentity/plugin.js, change:

        allowedContent: 'drupal-entity[*]',
        requiredContent: 'drupal-entity[*]',

to something like:

        allowedContent: 'drupal-entity[data-entity-type,data-entity-uuid,data-entity-embed-*]',
        requiredContent: 'drupal-entity[data-entity-type,data-entity-uuid,data-entity-embed-*]',

Now, because this is a custom element, that seriously complicates matters. I'm not sure about that either. Pinging the CKEditor folks.

Hrm, that is interesting (but especially weird).

Can you rename the commend from editdrupalentity to just drupalentity for a minute and see if that helps?

I'll debug it when I find the time — hopefully this weekend, but quite possibly after DrupalCon Barcelona. This is not RC-blocking, I need to prioritize issues that are.

I'm not 100% sure, but I think that the problem is related to the way how you defined the button. There's no relation between the button and the allowed content. Usually it works like this:

1. When a button is added to the toolbar, the editor.addFeature() method is called with that button.
2. That button has a `command` property defined. This means that clicking this button will execute that command, but it also means that button.toFeature() will return the related command. Alternatively, the button can implement the CKEDITOR.feature interface directly, by defining the `allowedContent` and other properties in itself.
3. The command implements the CKEDITOR.feature interface, so it has the `allowedContent` and `requiredContent` properties. All that is registered to the ACF.
4. Later on, Drupal gets information about all features and their allowed/required content.

This does not work in your case, because your button does not define either the `allowedContent` property directly or the `command` property. That's because your case it's a bit special because you have one command which is executed by many different buttons. In this case I would define the `allowedContent` on the button despite the fact that it's already defined on the command.

Wow, thanks, I'd never have been able to figure that out.

Dave Reid: hope this helps!

#2614350: entity_embed's text filter should be enabled by default … but in which text formats? proposed to auto-enable relevant text filter too. It seems that we could do both in one go.

Is technique in #9 also useful to solve that?

I'll have a look :)

@axe312: How is it going? :)

Attended to dotCSS today, no Wifi was over there :)

Could you please share any info you got from the ckeditor guys? You mentioned in the IRC that you had a talk with them.

We should mention that Dave Reid did some work on this over here: https://github.com/drupal-media/entity_embed/pull/179

All the info is in #9. Reinmar works for CKEditor.

According to this comment (https://www.drupal.org/node/2554687#comment-10330983) the functionality to automatically add the drupal-entity tag to the list of allowed content for HTML-correction needs to be added by defining this on the button itself due to the fact that these buttons/commands are not individually defined.

I tested that as you can see in this PR and when you now add the button to the editor toolbar you get the desired result.
I had to specify the attributes as required (!) otherwise only without attributes would have been added to the list.

I am not sure, if we can now get rid of the definition of the allowedContent attribute in other places, but that might need further testing.

Please review!

I created a PR on Github:
https://github.com/drupal-media/entity_embed/pull/205

or use attached patch

Sorry, got my diff reversed :(

find the correct patch attached here

I tested this and it works as expected. I am also not sure about removing allowedContent from other places.

Will set to RTBC and keep it like this so other people can provide feedback.

+++ b/js/plugins/drupalentity/plugin.js
@@ -136,6 +136,7 @@
+            allowedContent: 'drupal-entity[!data-entity-type,!data-entity-id,!data-entity-uuid,!data-entity-embed-display,!data-entity-embed-settings]',

This should just match what #2638788: Required attributes wildcard in CKEditor plugin not compatible with filter system did. This is basically a follow-up to that issue.

Here i just make change for allowedContent list of elements match to list from #2638788: Required attributes wildcard in CKEditor plugin not compatible with filter system, still works as expected.

Patch does not apply.

Sorry for that, probably old version of entity_embed... I hope now will be ok.

#28 I don't understand. The allowedContent on a button is only necessary to know what the absolute minimum tags/attributes are for this button's functionality to work correctly. This will then cause those tags/attributes to be whitelisted automatically for the filter_html filter's settings when dragging this button into the toolbar.

This change has absolutely zero effect while using CKEditor. Drupal tells CKEditor which tags/attributes are allowed.

So, if you configure additional attributes to be allowed in your filter settings, it'll be allowed. This patch won't change that.

I am a bit confused. Which alt and title attributes are we talking about? As far as I know entity_embed won't generate or use those two arguments in any way. There are data-align and data-caption attributes, which entity_embed mentions in some parts of documentation and we might want to include. However, none of the two actually work at the moment. We expose align in UI and we don't even do that for caption. There are already issues that are addressing this: #2645458: data-align and data-caption don't work with entity_embed and #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog). Let's fix those problems there.

Other's module needs is a valid point, but I'd say it is their responsibility to provide everything they need. Entity embed simply can't predict every possible situation that might appear in the future.

Another problem that I noticed with #27 is data-entity-id. Some time ago we decided to keep supporting it in text filters while completely removing it from CKEditor integration part. Somehow this attribute still sneaked into allowedContent and requiredContent for widget and command definition. This causes it to be required and results in embed button not working unless data-entity-id is included into the list of allowed attributes. I suspect we should remove it from requiredContent of both?

#31 looks great. Tentative RTBC, hopefully Dave Reid +1s.

alt and title attributes are fully supported if embedding an image entity using an image formatter.

That is stored in data-entity-embed-settings AFAIK, which is allowed.

The problem is that those other modules do not have a way to hook into providing their new attributes to be automatically whitelisted, aside from doing it manually, which is not idea DX. I would rather whitelist all data attributes entirely.

I agree with @WimLeers in #2638788-7: Required attributes wildcard in CKEditor plugin not compatible with filter system. Allowing everything isn't the best practice. Explicitly whitelisting things that are really needed seems much safer.

Modules will extend functionality via EntityEmbedDisplay plugins and use data-entity-embed-settings to store things they need.

alt and title attributes are fully supported if embedding an image entity using an image formatter.

An image formatter would run at filter time. So, the filter that transforms Entity Embed markup into its final form simply must run after the filter_html filter.

Which you would need/want anyway, because image formatters could generate any markup, even crazy things like <iframe> or <canvas>.

So, I don't understand the reasoning in #33 at all.

I assumed that the data-entity-embed-settings should encapsulate the settings directly related to the display plugin itself, and not assumed to be a dumping ground for everything else. Especially important since those serialized settings are not namespaced in any way, so there is a desire to avoid having things dump arbitrary key/values in there.

Ahhhh!

This makes sense. I am very much afraid about the current design, in which data-entity-embed-settings is indeed a dumping ground.

Basically, the simplicity I described in #2628168-2: Clean up Entity Embed's data model.

I rerolled #31 and tested it extensively. It seems to be working, but I noticed that it took quite some effort to clear browser cache on filter configuration page. Maybe that was the issue.

I don't think this is a beta blocker. While is is a nice UX improvement it can be added later (hopefully that won't be the case).

IS mentions two things: enabling required tag and attributes in the HTML filter and enabling entity embed filter. Current patch fixes the first problem. Solving the second problem will require a bit more custom code and will be much easier to achieve when/if #2561473: Namespace buttons is committed.

I suggest that we fix required tag/attrbutes as part of this issue and work on enabling the filter in a follow-up when the namespace issue is solved.

The browser cache was also the problem for me. Other than that it works.

Committed. Thank you all!

Sorry to blow the dust off this issue, but I noticed that ImageFieldFormatter requires alt and title attributes on the drupal-entity in order to function properly. I have added an issue for that: see #3022768: Validate that `alt` and `title` are required attributes for `<drupal-entity>`, and ensure they're added by default.