Multiple image browsers in CKEditor insert image functionality

[argiepiano]

Description of the bug

When using CKeditor image insert functionality, and when the Text editor settings have the "Image uploading" functionality turned off, multiple instances of the file browser are shown. This was first reported in the Forum. Additionally, the progress bar doesn't stop spinning.

Steps To Reproduce

1. Start with a Vanilla install of Backdrop (e.g. Tugboat demo site)
2. Visit /admin/config/content/formats/filtered_html
3. Turn off Enable image upload
4. Go to the content create form node/add/page
5. Click the image icon and watch the bug in its full glory (see attached image)

Actual behavior

There are 3 instances of the file browser.

Expected behavior

Only one file browser should be shown

Additional information

• Backdrop CMS version: 1.23.0
• PHP 7.4.30

[argiepiano]

A couple of additional observations:

1. The image browser is attached to the form by an ajax call triggered by Javascript. JS basically "clicks" a form submit button with the name library_open (line 237 of filter.js).
2. This causes the server to return a View, which is prepended to the element with class .editor-image-library by _filter_image_library_ajax.inc
3. This works as expected when the Upload image functionality is enabled
4. When it's not, JS clicks the button multiple times, resulting in multiple Views being prepended to the element. Furthermore, if you click the image icon again without refreshing the page, the button is clicked more times every time, and you'll get more Views prepended every time.

[indigoxela]

I can also confirm that bug. It might have to do with the fact that normally (with upload on) the src input isn't visible but gets activated after toggling. When the upload form isn't enabled, the src input is visible initially. Tricky...

No toggling and the triggers fire right away.

[argiepiano]

Bingo! The issue is twofold:

1. The theming function for filter_format_editor_image_form() found in filter.theme.inc inserts an empty <div class="editor-image-library"> within the form itself. This div is not needed here. In fact, Backdrop.behaviors.editorImageDialog in filter.js adds a div with the same class to the DOM, and then populates it (them) with the result of the image browser View obtained via ajax. So, we end up with two divs and two image browsers, one in the left side of the dialog within form, and another one in the correct location. (NB: the image browser in the left side does not appear when we have "Enable image uploads" because there is javascript code that removes this div (or rather, these two divs) when the dialog is first opened on the "Upload an image" option).

2. The main culprit for the multiple (and "multiplying") image browsers on the right side of the dialog is a listener for the dialog:aftercreate custom event, thrown by dialog. Because this listener is located within the attach method for the behavior Backdrop.behaviors.editorImageDialog, it gets attached to the window element multiple times, every time there is an ajax call to get the content of the dialog. This causes multiple js "clicking" of the invisible ajax button that loads the View for display every time we open the dialog and select "Select from library", resulting in multiple views being attached.

Moving this dialog:aftercreate event listener outside the attach method is the solution here. I tried using once() to avoid multiple listener attachments, but once() doesn't seem to work when you use it on window.

Taking the listener out of the method is actually done in a couple of other instances in Backdrop JS. For example, in:

• dialog.ajax.js
• file.js

I've tested this and it works well.

An additional clarification: these multiple listener attachments and calls do exist, but go unnoticed, when you have the "Enable image upload" filter option on, because the default item in the "Insert image" dialog is the "Upload an image" item. The multiple calls don't cause any problems to this item when you open the dialog.

PR coming

[argiepiano]

PR https://github.com/backdrop/backdrop/pull/4260

[indigoxela]

Brilliant! (Again. :wink:) A quick test in the sandbox and I can confirm it works. Will test more thoroughly ASAP.

[argiepiano]

I'm wondering if it may be possible to include this fix in 1.23.1? Testing and code review anyone?

[argiepiano]

Is it possible to have this code-reviewed? It's a nasty bug that affects the UX. @indigoxela, any change you may have the time? 🙏🏽

[klonos]

I also confirmed the bug in a vanilla installation of latest 1.x, and then confirmed that the bug is fixed in the PR sandbox 👍🏼

The code change also seems good to me (removing the additional div from core/modules/filter/filter.theme.inc , and moving the code outside attach within Backdrop.behaviors.editorImageDialog).

I'm not expert in JS, but I'm adding the milestone, and marking RTBC in order to grab the attention of our core committers next time they check the queue. I think that @quicksketch may be more suitable, but other core committers might also be comfortable and proficient with JS.

[quicksketch]

This implementation looks good to me. We could improve it slightly by checking which dialog got opened. Right now it fires and tries to manipulate the dialog contents regardless of which dialog is opened (for example a link dialog also gets acted upon). However, since none of the selectors match nothing actually happens on these other dialogs. In the interest of keeping changes minimal and solving this immediate problem, I think we can merge this as-is.

[quicksketch]

https://github.com/backdrop/backdrop/pull/4260 merged into 1.x and 1.23.x. Thanks @argiepiano, @indigoxela, and @klonos!