Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
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.
Related Issues
#2035721: Figure out how to translate Drupal-specific CKEditor plugins
Original report by @jessebeach
Comment | File | Size | Author |
---|---|---|---|
#14 | automate_cke_plugin_visibility-2089631-14.patch | 10.28 KB | Wim Leers |
#14 | interdiff.txt | 1.19 KB | Wim Leers |
#9 | automate_cke_plugin_visibility-2089631-9.patch | 10.26 KB | Wim Leers |
#1 | tab_hiding-2089631-1.patch | 2.54 KB | Wim Leers |
Screenshot of a JavaScript error thrown by the DrupalImage CKEditor plugin | 298.75 KB | jessebeach |
Comments
Comment #1
Wim LeersIt 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.
Comment #3
Wim Leers#1: tab_hiding-2089631-1.patch queued for re-testing.
Comment #4
Wim LeersComment #6
Wim Leers#1: tab_hiding-2089631-1.patch queued for re-testing.
Comment #7
nod_doesn't fix the problem on small screens.
Comment #8
nod_also patched vtabs #2102019: Vertical tabs looks broken on small screens
Comment #9
Wim LeersReviewed + 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:
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.Comment #11
Wim Leers#9: automate_cke_plugin_visibility-2089631-9.patch queued for re-testing.
Comment #12
jessebeach CreditAttribution: jessebeach commentedLet's store
$(this)
in a variable rather than invoking the jQuery function so many times.Comment #13
Wim LeersD'oh, of course!
Comment #14
Wim LeersEt voilà!
Comment #15
jessebeach CreditAttribution: jessebeach commentedLooks good. Thanks for the update Wim. This patch is ready to go.
Comment #16
webchickHm. 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?
Comment #17
nod_then they change the library definition and get rid of this dependency. they'd have to write lots of js anyway.
Comment #18
Wim Leers#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.
Comment #19
webchickOk, works for me. Thanks for the explanation!
Committed and pushed to 8.x. Thanks!
Comment #20
Wim LeersThanks!
Comment #21
webchickOops, very sorry. I accidentally reverted this when trying to test the CKE accessibility patch, so re- committed and pushed to 8.x. ;)