Problem/Motivation

Adding caption from modal dialog on an entity without caption fails to render upon dialog save.

https://www.drupal.org/files/issues/2019-06-08/deleting%20caption%20bug.mov

Steps to reproduce:
From editor with caption on entity embed:

  1. Reopen dialog
  2. delete caption
  3. save
  4. Reopen dialog
  5. add caption
  6. save

Expected:

New caption displays

Actual:

The embed is seemingly still captionless. Hitting the back button and the forward button renders the new caption. So it seems to be a render issue rather than a data issue.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

oknate created an issue. See original summary.

oknate’s picture

Version: 8.x-1.0-alpha1 » 8.x-1.x-dev
Issue summary: View changes
oknate’s picture

Issue summary: View changes
oknate’s picture

I have test coverage demonstrating this bug in this patch:

https://www.drupal.org/files/issues/2019-06-09/entity-embed-3060396-11.p...

    // @todo remove the back and forward.
    // See https://www.drupal.org/project/entity_embed/issues/3060439
    $this->assertSession()->elementNotExists('css', 'figcaption');
    $this->pressEditorButton('undo');
    $this->pressEditorButton('redo');

    $this->getSession()->switchToIFrame('ckeditor');
    $figcaption = $this->assertSession()
      ->waitForElementVisible('css', 'figcaption');
oknate’s picture

Status: Active » Needs review
StatusFileSize
new21.79 KB
new22.21 KB

I suspect the cause is this:

Changes made to the data fire the data event.

From https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_plugins_widget.h...

I suspect it's that when you delete the property and add it back, it doesn't detect that a property has changed. Not confirmed, just a hunch. But this seems to fix it:

               if (values.attributes['data-caption']) {
                 values.attributes['data-caption'] = CKEDITOR.tools.htmlDecodeAttr(values.attributes['data-caption']);
               }
+              else {
+                values.attributes['data-caption'] = '';
+              }

My reasoning is that by keep the attribute set to a blank string, when it's updated, it detects that it has changed.

I didn't want to write a separate test for this issue, so I'm posting an updated patch to #3060396: Add CKEditor Widget JS test coverage.

The test only patch should fail and demonstrate the bug.

The last submitted patch, 5: entity-embed-3060439-5--TEST-ONLY.patch, failed testing. View results

wim leers’s picture

Title: Caption fails to render when adding back to captionless embed » Caption fails to render when adding back to captionless embed when edited via EntityEmbedDialog
StatusFileSize
new883 bytes
new1.54 KB

Nice find!

I landed #3060396: Add CKEditor Widget JS test coverage. Rebased this patch.

oknate’s picture

We might want to just reinitialize the widget. That would probably also fix #3060634: Editing link text in captions via EntityEmbedDialog fails to update link text in CKEditor Widget

Although I'm sure there are advantages with undo/redo not to reinitialize unnecessarily.

Can we just reinitialize the editable caption, when it changes?

The last submitted patch, 7: 3060439-7-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

oknate’s picture

We could also put this in as a stop-gap, since it doesn't seem to hurt anything, until we find a fix that encompasses both bugs (and perhaps others).

wim leers’s picture

Issue tags: +Media Initiative
StatusFileSize
new24.77 KB
new2.14 KB

Adding data.hasCaption instead, just like CKEditor's image2 plugin does. This means we don't have to pretend a data-caption attribute should still exist when it doesn't need to exist (which is a negative consequence of the current patch).

This also sets us up to add the same checkbox-triggered caption enabling that EditorImageDialog and drupalimagecaption have in core.

wim leers’s picture

StatusFileSize
new1.41 KB

That was the wrong interdiff, sorry!

  • Wim Leers committed bd37861 on 8.x-1.x
    Issue #3060439 by Wim Leers, oknate: Caption fails to render when adding...
wim leers’s picture

Status: Needs review » Fixed

That came back green, pushed. 🚢

wim leers’s picture

Issue tags: +DevDaysCluj
rosinegrean’s picture

Issue tags: -DevDaysCluj +DevDaysTransylvania
wim leers’s picture

Component: Code » CKEditor integration

Status: Fixed » Closed (fixed)

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