Problem/Motivation

This bug seems to have something to do with vertical tabs not rendering at small screen sizes. The CKEditor plugin admin code expects the vertical tab structure to be present at all times.

Screenshot of a JavaScript error thrown by the DrupalImage CKEditor plugin

This affects drupalimage and stylescombo CKEditor plugins.

Proposed resolution

I monkey-patched the code by checking for the VerticalTab variable first:

// React to added/removed toolbar buttons.
$context
  .find('.ckeditor-toolbar-active')
  .on('CKEditorToolbarChanged.ckeditorDrupalImageSettings', function (e, action, button) {
    if (button === 'DrupalImage' && $drupalImageVerticalTab) {
      if (action === 'added') {
        $drupalImageVerticalTab.tabShow();
      }
      else {
        $drupalImageVerticalTab.tabHide();
      }
    }
  });

This doesn't strike me as the right approach, but it might be.

Remaining tasks

Fix it.

User interface changes

None.

API changes

None.

#2035721: Figure out how to translate Drupal-specific CKEditor plugins

Original report by @jessebeach

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
Issue tags: -ckeditor +Spark, +CKEditor in core
FileSize
2.54 KB

It also happens when you enable CKEditor for a text format that didn't have a text editor enabled yet (e.g. for the Restricted HTML text format).

This is the solution I came up with. I kept it as self-contained as possible to not conflict with #1872206: Improve CKEditor toolbar configuration accessibility.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Spark, -CKEditor in core

The last submitted patch, tab_hiding-2089631-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Spark, +CKEditor in core

#1: tab_hiding-2089631-1.patch queued for re-testing.

Wim Leers’s picture

Issue tags: +sprint

Status: Needs review » Needs work
Issue tags: -JavaScript, -sprint, -Spark, -CKEditor in core

The last submitted patch, tab_hiding-2089631-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +sprint, +Spark, +CKEditor in core

#1: tab_hiding-2089631-1.patch queued for re-testing.

nod_’s picture

Status: Needs review » Needs work

doesn't fix the problem on small screens.

nod_’s picture

Wim Leers’s picture

Title: CKEditor plugins throw JavaScript error on the Text Format configuration screen at small viewport sizes » Showing/hiding of CKEditor plugin settings is fragile, automate it (breaks in narrow viewports and when enabling CKEditor)
Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
Issue tags: +DX
FileSize
10.26 KB

Reviewed + RTBC'd #2102019: Vertical tabs looks broken on small screens. I was then pulled in to other things, so by now that issue has been committed already — yay :)

While working on this, I also noticed a related problem in the showing/hiding of vertical tabs for filter settings, for which I've also provided a patch: #2108955: Settings for disabled filters are not hidden in narrow viewports, because Vertical Tabs are disabled in narrow viewports.


Not only should this bug be solved, the show/hiding logic for the plugin-specific settings should be abstracted and handled automatically, so that not every CKEditor plugin with a settings form has to duplicate this. The CKEditor developers pointed out that it would be annoying/painful to duplicate this code all over the place, and they're absolutely right.

Solution

There are fundamentally 3 problems that we need to solve in this issue, because they all overlap:

  1. Mobile/responsive. The code used so far doesn't work on mobile devices, because on narrow screens, Vertical Tabs are disabled automatically.
  2. New text formats. Due to race conditions, the existing code to show/hide vertical tabs for CKEditor plugin settings doesn't reliably work when selecting CKEditor as the text editor for a text format that didn't have CKEditor as its text format before.
  3. Automation/duplication. The existing code has to be repeated for every single CKEditor plugin that is configurable (provides a settings form). That's crazy and silly. It should be automated.

The attached patch meets all three goals. The first two can be solved with better code. The third is solved by removing the duplicating code and generalizing it. We add some data- attributes on the server-side to let the client-side know which plugins are enabled whenever one of its buttons are enabled. Then, if those buttons are indeed enabled on the client-side, some JavaScript will automatically show the corresponding plugin settings. Automation achieved.

More complex plugins, such as those that are enabled based on some context (i.e. those that implement CKEditorPluginContextualInterface) will not be shown/hidden automatically, because arbitrarily complex logic may be used to determine when that CKEditor plugin (and hence its settings) should be enabled.
Buttons-only plugins (CKEditorPluginButtonsInterface) are much simpler, and hence we can automate them.

Status: Needs review » Needs work
Issue tags: -JavaScript, -sprint, -Spark, -CKEditor in core, -DX

The last submitted patch, automate_cke_plugin_visibility-2089631-9.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +sprint, +Spark, +CKEditor in core, +DX
jessebeach’s picture

$ckeditorPluginSettings.find('[data-ckeditor-plugin-id]').each(function () {
  if ($(this).data('verticalTab')) {
    $(this).data('verticalTab').tabHide();
  }
  else {
    // On very narrow viewports, Vertical Tabs are disabled.
    $(this).hide();
  }
  $(this).data('ckeditorButtonPluginSettingsActiveButtons', []);
});

Let's store $(this) in a variable rather than invoking the jQuery function so many times.

Wim Leers’s picture

Status: Needs review » Needs work

D'oh, of course!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
10.28 KB

Et voilà!

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thanks for the update Wim. This patch is ready to go.

webchick’s picture

+++ b/core/modules/ckeditor/ckeditor.module
@@ -78,7 +78,10 @@ function ckeditor_library_info() {
+      // Depend on Vertical Tabs, so that Vertical Tabs' JavaScript is executed
+      // first, which ensures its behavior runs first.
+      array('system', 'drupal.vertical-tabs'),

Hm. I'm a little worried about tying CKEditor module to vertical tabs. What if someone themes the admin form heavily and decides to put these filter settings into, I dunno, quicktabs or something? Wouldn't we be back to square one?

nod_’s picture

then they change the library definition and get rid of this dependency. they'd have to write lots of js anyway.

Wim Leers’s picture

#16: what #17 says. It's easy enough to hook_library_info_alter() dependencies away. Plus, there's no harm in depending on something and then not using it — it only harms performance. This page however performs inherently poorly in page load time anyway, because there's so much going on.
Finally, the root cause of this is that we don't have a way to declare a "conditional dependency", i.e. "if this other library is loaded, then my code must run before or after it" — that's what sdboyer is making possible as well with his Assetic work.
In conclusion: there is no real problem here, only a work-around, that contrib can even disable, and the root cause lies elsewhere in core.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, works for me. Thanks for the explanation!

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Thanks!

webchick’s picture

Oops, very sorry. I accidentally reverted this when trying to test the CKE accessibility patch, so re- committed and pushed to 8.x. ;)

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