Follow-up to #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored

Problem/Motivation

From #2579979-27: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored:

I'm getting strange JS errors after this:

Uncaught TypeError: Cannot set property 'float' of undefined

Looks like styles is not initialized, but I have no idea what's different in my case that it only happens for me?

This for example happens when switching from an text format without drupalimage to one that has it enabled and completely breaks wysiwyg then.

The following fixes it for me, I can open a follow-up tomorrow, but I don't know enough JS to know if the problem is really somewhere else?

diff --git a/core/modules/ckeditor/js/plugins/drupalimage/plugin.js b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
index 724fa50..5160797 100644
--- a/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
+++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
@@ -43,6 +43,7 @@
         });
         var allowedContentDefinition = {
           element: 'img',
+          styles: { },
           attributes: {
             '!data-entity-type': '',
             '!data-entity-uuid': ''

Proposed resolution

Fix it.

Remaining tasks

Write patch & review

User interface changes

None

API changes

None

#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
#2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored

CommentFileSizeAuthor
#3 i2589779-3.patch1.42 KBJelle_S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jelle_S created an issue. See original summary.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S at Attiks commented
Issue summary: View changes
Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S at Attiks commented
Status: Active » Needs review
FileSize
i2589779-3.patch1.42 KB

Here's the patch. Sorry I didn't catch this in the other issue :-/

attiks’s picture

attiks CreditAttribution: attiks at Attiks commented
Status: Needs review » Reviewed & tested by the community

Nice catch of berdir, works

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Active
Issue tags: +Needs steps to reproduce

This for example happens when switching from an text format without drupalimage to one that has it enabled and completely breaks wysiwyg then.

I can't reproduce this.

  1. I removed the "Image" button from Full HTML's CKEditor. Created a node using Full HTML. Then editing the node and switching to Basic HTML: no error.
  2. Then I figured it could be the fact that Full HTML has no HTML restrictions at all, which could prevent this error from happening in CKEditor. So I restricted Full HTML's allowed HTML. Still not able to reproduce.
Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S at Attiks commented

I was able to reproduce this earlier. If I remember correctly, I used these steps:

  1. Created a node with image using Full HTML
  2. I removed the "Image" button from Filtered HTML's CKEditor.
  3. I removed the img tag from the allowed HTML in the Filtered HTML format.
  4. Then editing the node and switching to Filtered HTML: error.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: -Needs steps to reproduce

In trying to replicate #6, I found much easier steps to reproduce:

  1. Go to /admin/config/content/formats/manage/restricted_html
  2. Enable CKEditor
  3. Doing this has triggered that error.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Reviewed & tested by the community

Fix confirmed.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Issue tags: +rc target triage

Change is risk free and fixes a rather problematic, user facing bug. Not exactly sure how it affects the settings page, but wouldn't be surprised if configuring it there would not work then after the error, especially when aggregation is enabled?

So, tagging with rc target triage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: -rc target triage +rc target

Dicussed with @effulgentsia - we agree that this should be an RC target since #7 seems very simple for any user to do and as we have no test coverage for JS it seems obvious that we're going to discover more bugs during RC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed cea9c14 and pushed to 8.0.x. Thanks!

  • alexpott committed cea9c14 on 8.0.x 
    Issue #2589779 by Jelle_S, Wim Leers: Uncaught TypeError: Cannot set...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

This appears to have caused a regression in #2598070: [regression] CKEditor Link button does not show if HTML filtering is enabled. Can anyone here help review that issue?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.