Early Bird Registration for DrupalCon Atlanta is now open! By registering during our Early Bird Registration window, you’ll save $100. This window ends on 19 January 2025 and will go by quickly, so don’t wait!Steps to reproduce:
- Fresh install with Standard install profile, enable the media_library module
- Enable the embedded media filter for a text format and add the Media Library button to the CKEditor configuration.
- Use layout builder to create a new custom "Basic block" with an embedded media, without making any changes to the media. The default alignment should be "center."
- Edit the newly created inline block, edit the embedded media and change the alignment from "Center" to "Right" but do not change anything else. Save the block.
Expected Results
That the textarea field submits with the changed attribute.
<textarea class="js-text-full text-full form-textarea resize-vertical" data-media-embed-host-entity-langcode="en" data-drupal-selector="edit-settings-block-form-body-0-value" data-editor-value-is-changed="false" data-editor-value-original="<drupal-media data-align="right" data-entity-type="media" data-entity-uuid="3ded04c3-64c6-4125-9144-7073c7ecd2a5" data-view-mode=""></drupal-media>
" id="edit-settings-block-form-body-0-value--RScU9kHtPt0" name="settings[block_form][body][0][value]" rows="9" cols="60" placeholder="" data-editor-active-text-format="full_html" style="visibility: hidden; display: none;" tc-textcontent="true" data-tc-id="0.5266607481983859"><drupal-media data-align="right" data-entity-type="media" data-entity-uuid="3ded04c3-64c6-4125-9144-7073c7ecd2a5" data-view-mode=""></drupal-media>
</textarea>
Actual Results
That the textarea field submits with unchanged data attribute.
<textarea class="js-text-full text-full form-textarea resize-vertical" data-media-embed-host-entity-langcode="en" data-drupal-selector="edit-settings-block-form-body-0-value" data-editor-value-is-changed="false" data-editor-value-original="<drupal-media data-align="center" data-entity-type="media" data-entity-uuid="3ded04c3-64c6-4125-9144-7073c7ecd2a5" data-view-mode=""></drupal-media>
" id="edit-settings-block-form-body-0-value--RScU9kHtPt0" name="settings[block_form][body][0][value]" rows="9" cols="60" placeholder="" data-editor-active-text-format="full_html" style="visibility: hidden; display: none;" tc-textcontent="true" data-tc-id="0.5266607481983859"><drupal-media data-align="center" data-entity-type="media" data-entity-uuid="3ded04c3-64c6-4125-9144-7073c7ecd2a5" data-view-mode=""></drupal-media>
</textarea>
Note the data-editor-value-is-changed attribute on the textarea is still false in this case, and the contents of the textarea have not been updated.
I am trying to figure out if this is a layout builder bug, or a general CKEditor-embedded-in-a-modal bug.
| Comment | File | Size | Author |
|---|---|---|---|
| #65 | Screenshot 2023-06-24 at 12.19.00 AM.png | 159.65 KB | shobhit_juyal |
| #63 | 3102249-port-for-D9.3.22-63.patch | 454 bytes | gxleano |
| #60 | 3102249-port-for-D9.4.1-60.patch | 1.1 KB | andre.bonon |
| #52 | 3102249-52.patch | 8.71 KB | jcnventura |
Comments
Comment #2
dave reidComment #3
dave reidComment #4
dave reidI can reproduce this with the steps above. It does not happen when I create a custom block library block, this only happens when being used with layout_builder to create the block.
Comment #5
dave reidSo the issue seems to be when CKEditor is inside of a ui-dialog (like the off-canvas tray, or if you were to use the Layout Builder Modal module).
Comment #6
dave reidComment #7
dave reidAHA! In drupalmedia/plugin.js:
I was curious why we weren't just using 'saveSnapshot' here like we are in other places. I read on https://stackoverflow.com/questions/19878442/ckeditor-4-how-to-make-snap...
So this isn't committing the change to CKEditor without an additional
editor.fire('saveSnapshot') call after the unlockSnapshot call. This resolves the issue with the field not getting saved correctly for me.Comment #8
dave reidComment #9
dave reidWith this patch I now see the change event firing for the CKEditor instance when I change existing embed attributes in the dialog and save.
Comment #10
maacl CreditAttribution: maacl commentedThank you for filing the issue and creating a patch! I noticed the same behaviour, and can confirm the patch fixes this.
Comment #11
lauriiiThank you for the bug fix! It would be awesome to see test coverage extended for this use case.
Comment #12
dave reidComment #13
dave reidHrm, looks like there's no FunctionalJavascript test coverage for editing an existing embed at all. :/
Comment #14
dave reidConfirmed that this doesn't seem to affect an embedded media in a non-dialog or non-modal CKEditor, like on a regular node form.
Comment #15
dave reidThis seems to also resolve #3073294: Remove obsolete @todo for "Undo bug when first inserting media into unfocused CKEditor" as with this change I now have the Undo button available after inserting or editing an embedded media in the CKEditor.
Comment #16
mark_fullmerI can also corroborate the original problem under the same circumstances and confirm that the addition of
editor.fire('saveSnapshot');per #9 resolves it.Since there are currently no FunctionalJavascript tests for editing an existing embed, should a separate issue be created to add test coverage, with this issue dependent on its resolution?
Comment #18
nod_Comment #19
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded a reroll of patch #9.
It still need works for tests.
Comment #20
u_tiwari CreditAttribution: u_tiwari at Axelerant commentedMissing a "s" in "saveSnapshot" @ravi
Comment #21
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedThanks @u_tiwari
Updated patch.
Comment #22
narendra.rajwar27Comment #23
phenaproximaI can see this being really disruptive for authors (and it's basically a mild case of data loss). Escalating the priority accordingly.
Comment #24
janmejaig CreditAttribution: janmejaig at Material for Drupal India Association commentedI have Checked the above ticket for Drupal 9.x.1 found that it is working fine for the "Left & Right" alignment without applying a patch. But it is getting failed for the "center"alignment with or without patch, it is failing in both the cases, please look into the issue .
Steps to add alignment for media library
Comment #25
janmejaig CreditAttribution: janmejaig at Material for Drupal India Association commentedPlease look this ticket for reference for the center alignment issue as mention below :
#3126783: Image align center not working
Comment #27
jastraat CreditAttribution: jastraat at Technivant commentedThis is still a problem when using ckeditor in a modal (at least in 8.9; I haven't explicitly tested in D9). It affects all attributes of the media item including changing the display/view mode. The patch in 21 fixes the problem. It seems like this is a really nasty bug to be languishing without attention.
Comment #28
phenaproximaThanks for reviewing it, @jastraat! I think this issue will need automated test coverage before it can be committed, though (as evidenced by the "Needs tests" tag).
If anyone would like to work on that, there is some documentation on test writing that might offer some guidance: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal. The first two topics on that page probably will prove most helpful in this case.
Comment #29
staceroni CreditAttribution: staceroni commentedI have a similar issue using the Media Library embed button within Inline Entity Form. When you embed a media, but make no other changes in the textarea, and update the IEF reference, the changes are not saved. Toggling between the view source and preview of Ckeditor will fix the behavior at that time. This patch happens to fix the broken behavior, although from the conversation here, looks to be a happy coincidence.
Comment #31
chrisgross CreditAttribution: chrisgross commentedI can confirm that this is still an issue for Drupal 8.9.15 and that patch #21 seems to work.
Comment #32
paul.linney CreditAttribution: paul.linney commentedCan also confirm that patch in #21 works with Drupal 9.2.9 with Acquia Site Studio.
Thanks
Comment #33
chrissnyderTested patch #21 with Drupal 9.2.9 successfully. It also solves a related issue I was having where CKEDITOR's change event was not firing right after embedding a media entity.
Comment #35
yeebot CreditAttribution: yeebot as a volunteer and commentedI was experiencing the same issue with Drupal 9.2.11 with a Text (formatted, long) field with 2 text formats enabled: Full HTML and Basic HTML. (I'm not sure if relevant but note that Basic HTML format does not allow media entity embedding). The issue occurred when trying to update the alignment of a media entity while in Full HTML mode. I was able to make the change but it would not save.
The patch from #21 resolves the issue.
However I was alternatively able to resolve the issue without the patch by simply removing the Basic HTML format which in my case was redundant, so that only the Full HTML text format applied.
Comment #36
_KurT_ CreditAttribution: _KurT_ at JAKALA (formerly FFW) commentedCan confirm patch from #21 works for drupal 9.3.6
Comment #37
chrisgross CreditAttribution: chrisgross commentedPatch #21 still works on 9.3.6. This really needs to get merged.
Comment #38
phenaproximaAs per #28, this is still needing automated test coverage…
Comment #39
rajab natshah CreditAttribution: rajab natshah at Vardot for Vardot commentedTested 3102249-21.patch with Drupal 9.3.x
Thank you Ravi
Comment #40
jschref CreditAttribution: jschref commentedTested patch #21 with Drupal 9.3.9 successfully.
Comment #41
johnzzonChiming in to say patch in #21 works perfectly. Adding a new media embed without changing any text now properly updates the CKEditor snapshot.
Comment #42
yogeshmpawarUpdated patch will resolve CS issues.
Comment #43
yogeshmpawarKeeping it in NW as automated test coverage required for this issue.
Comment #44
tim bozeman CreditAttribution: tim bozeman at CyberSolution for Tag1 Consulting commentedThanks @Dave Reid!
I can manually reproduce this in the browser and the patch fixes it for my project, but for some reason when I create a test for it, it passes in the test environment. 🤔
@Dave Reid, is there some nuance I'm missing with this test?
Comment #45
tim bozeman CreditAttribution: tim bozeman at CyberSolution for Tag1 Consulting commentedWhoops, last try to get test-bot to run! 🚀
Comment #48
narendra.rajwar27Adding re-rolled patch for Drupal 9.5.x, combining patch from comment #42 and #45
Comment #51
kbeck303 CreditAttribution: kbeck303 at Oomph, Inc. commentedThis patch also fixes the display of the view mode not getting updated in the same 'edit media' modal. We are running Core 9.4.8 and used the patch in comment 42
Comment #52
jcnventuraRe-roll of #48 for current state of 9.5.x.
Comment #53
dpagini CreditAttribution: dpagini at Voya Financial commentedI added the test for 9.5.x and it looks like there is one failure here that needs to be looked at, but we are using this patch and it is fixing the problem for us. Seems like this is pretty close otherwise.
Comment #54
jcnventura@dpagini Yes, but this now needs to be changed for Drupal10's ckeditor5, and maybe also move the ckeditor changes to the spun-off ckeditor 4 module.
Comment #55
jhedstromIs this still relevant for CKEditor 5? I'm not finding where this code might apply there, so perhaps this issue can be moved to the CKEditor 4 contrib module's queue?
Comment #56
mark_fullmerI confirm that I cannot reproduce this problem using CKEditor 5. The original steps to reproduce, in fact, involve a very different process for changing the image alignment (involving a Drupal-provided modal), whereas CKEditor 5's equivalent is done via the native balloon pop-up. I agree that this probably should be moved to the CKEditor 4 contrib module queue.
Comment #57
chrisgross CreditAttribution: chrisgross commentedPatch #21 worked for me on 9.3.6, but #52 does not on 9.5.5. The patch applies, but image attributes still do not seem to be saved. Is anyone else experiencing this?
Comment #58
joegraduateFWIW, it looks like a ckeditor contrib project issue was already created as a child of this issue: #3330723: Changing an existing embedded media's alignment or alt data attributes does not get saved with CKEditor.
Comment #59
chrisgross CreditAttribution: chrisgross commented@joegraduate Yeah, I just realized we switched to using CKEditor 4 in contrib due to the core change, which explains it. The patch in the issue you linked works. Thanks!
Comment #60
andre.bonon CreditAttribution: andre.bonon at ImageX commentedUpdating the patch to apply to the D9.4.1.
Comment #61
leymannxComment #62
smustgrave CreditAttribution: smustgrave at Mobomo commentedIssue was previously tagged for tests which still appear to be needed.
Also proposed solution should be highlighted in issue summary please
Thanks.
Comment #63
gxleano CreditAttribution: gxleano at Factorial GmbH commentedUpdating the patch to apply to the D9.3.22.
Comment #65
shobhit_juyal CreditAttribution: shobhit_juyal at Material for Drupal India Association commented3102249-port-for-D9.3.22-63.patch
Patch failed for Drupal 10.1.0
Comment #66
socialnicheguru CreditAttribution: socialnicheguru commentedComment #67
sijumpk CreditAttribution: sijumpk at QED42 commentedTried recreating the issue in Drupal 10 and was unsuccessful. D10 is using CKEditor 5 and this patch is not needed for the alignment to work.