Problem/Motivation
The javascript provided by filter module to hide/show the filter guidelines uses selectors only provided in Classy. This means that the script does not work in themes that don't use Classy as a base theme.
To reproduce
- Set a theme that does not use Classy as the base theme as the theme to be used for editing content
- Configure another text format that does not use a WYSIWYG editor (this will just help to illustrate the issue)
- Add or edit some content, and try using the filter guidelines
Expected Behaviour
- If the selected text format uses the WYSIWYG editor, all formatting guidelines should be hidden.
- If the selected text format does not use the WYSIWYG editor, only the formatting guidelines for the selected text format should display
Actual Result
- Formatting guidelines for all non-WSYIWYG text formats are displayed. They do not change when the text format select option changes
This happens because the selectors used in filter.js are not available in the stable/default templates.
Proposed resolution
Add the expected classes to the default & Stable filter-guidelines.html.twig template.
Release notes snippet
Formatting guidelines markup now only contains necessary classes and attributes to attach JavaScript. All visual classes (filter-wrapper, filter-guidelines, filter-list and filter-help has been removed, as well as all of the CSS (filter.admin.css).
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff_24-27-8.7.x.txt | 1.07 KB | Eli-T |
#27 | filter-tips_toogle_js_fix-2881212-27-8.7.x.patch | 8.95 KB | Eli-T |
#27 | interdiff_25-27-8.8.x.txt | 1.07 KB | Eli-T |
#27 | filter-tips_toogle_js_fix-2881212-27-8.8.x.patch | 9.21 KB | Eli-T |
#25 | filter-tips_toogle_js_fix-2881212-25-8.8.x.patch | 9.22 KB | Eli-T |
Comments
Comment #5
huzookaChanging the version since this is a bug.
Comment #6
huzookaFix: adding
js-
-prefixed selectors anddata-drupal-format-id
attribute inTextformat
element and in tempate preprocess.Comment #7
lauriiiI think we have usually referenced the JS file directly rather than the name of the library.
Let's remove these unrelated changes from the patch.
Comment #8
lauriiiI'm wondering if we should remove these classes now that the JS is using the
js-
prefixed classes. We would then add it back in Stable theme to ensure that themes using Stable wouldn't suffer from this change.Comment #9
huzookaComment #10
huzookaWe shouldn't remove them because it is impossible to add them back to the container or to the select without a theme suggestion like
container__filter
and a preprocess function.Comment #11
lauriiiCouldn't we add them back in a pre_render function?
Comment #12
huzookaI cannot see the benefits.
Comment #13
lauriiiThis would be work towards an old goal set back in DrupalCon Austin to have as few classes as possible in render elements and preprocess functions. The classes that were left to preprocess and pre-render functions have some functionality attached to them. This could be either CSS of JavaScript. To distinguish classes that have functionality attached to them, the group decided to
js-
prefix classes used by JavaScript. We didn't get far enough to think about form elements, but this is essentially just continuing the work from back then. As part of the process, the old class was removed from core and moved to Classy, and core could use thejs-
prefixed class in its JavaScript.Comment #14
Eli-TUpdated Drupal version to 8.7.x.
Attached patch addresses comments in #7 and #8
Comment #15
lauriiiThank you for working on this! This seems like great progress. Instead of adding the js- prefixed classes here, we should add the non js- prefixed classes here. Then we could remove them from the render element itself.
Comment #16
Eli-TSwitched the classes added in the stable theme pre-render function from js to non-js as requested in #15, and removed the non-js classes from the render element.
Also removed the filter-help class from the render element as discussed with @lauriii outside this issue.
Comment #17
lauriiiNow that we've removed all the classes, the
filter.admin.css
becomes dead code. Luckily the file contains only positioning and design styles so it shouldn't exist in core. Therefore we could remove it from core. There's also issue to move all admin.css from core to Seven in #2566775: [Voltron patch] Move all remaining *.admin.theme.css to Seven, so I think removing this from core should be fine. What I'm wondering is how do we ensure that the file gets added to Seven eventually? It's a pre-existing issue and we've been making other changes without caring about Seven or Bartik 🤷♂️Comment #18
lauriiiThis class should be also added back in the Stable pre_render function.
Comment #19
Eli-Tfilter-help class re-added in stable.
filter.admin.css file removed from filter module, and references from filter.libraries.yml removed.
Comment #20
lauriiiAdding these type hints it outside the scope of this issue so let's remove them from the patch.
Comment #21
Eli-TType hints removed.
Comment #22
lauriiiStable is extending the library that the latest patch removes, meaning that the Stable CSS doesn't get loaded anymore after. We have fixed a similar issue in #3045467: Allow overriding module CSS without overriding their toolbar styles which you might be able to use as an inspiration.
Comment #23
Eli-T#22 addressed via hook_library_info_alter() implementation in stable theme.
Comment #24
Eli-TReuploaded patch in #23 and added separate patch for 8.7.x as stable.theme is too different between branches to patch both with the same file.
Comment #25
Eli-TThe 8.8.x patch in #24 had yarn.lock changes that are unrelated to this issue. Here is a version that does not.
The 8.7.x patch in #24 is correct, subject to review of course.
Comment #26
lauriiiI just noticed that the render element is using process function, not pre_render function. We should convert this to a process function as well. Other than that, this looks good. I've confirmed manually that themes depending on Stable still have pre-existing classes after this change, as well as the newly added classes.
Comment #27
Eli-T#pre_render changed to #process as requested in #26
Comment #28
lauriiiThis looks good! I created a draft change record for this.
I've confirmed manually that:
This looks good for me! Thank you @Eli-T!
Comment #29
lauriiiThanks for considering Drupal 8.7.x backport by providing separate patch! Unfortunately, I don't think we could introduce this in a patch release since production sites are affected by the new js- prefixed classes.
Comment #30
lauriiiThis affects Bootsrap theme which is a widely used theme not extending Stable. It seems to be only a visual regression. I opened an issue in their issue queue: #3061715: Update formatting guidelines markup after a Drupal Core change.
Comment #31
alexpottCommitted 1598f3e and pushed to 8.8.x. Thanks!
I talked to @lauriii about this issue and now that the CR is clear that we're only affecting themes that don't extend from stable we're good to go here. I agree that this should be only 8.8.x though as the fix is disruptive to such themes (even though they've opted into that disruption).
Comment #33
lauriii