Formatting guidelines toggle script relies on Classy selectors

Changing the version since this is a bug.

Fix: adding js--prefixed selectors and data-drupal-format-id attribute in Textformat element and in tempate preprocess.

1. +++ b/core/modules/filter/filter.module
   @@ -376,6 +376,9 @@ function template_preprocess_filter_guidelines(&$variables) {
   +  // Add format id for `filter/drupal.filter`.
   

   I think we have usually referenced the JS file directly rather than the name of the library.

2. +++ b/core/modules/filter/src/Element/TextFormat.php
   @@ -250,7 +256,8 @@ public static function processFormat(&$element, FormStateInterface $form_state,
   -   * \Drupal\Core\Session\AccountInterface
   +   * @return \Drupal\Core\Session\AccountInterface
   +   *   The current user's account interface.
   
   @@ -260,6 +267,7 @@ protected static function currentUser() {
   +   *   The config factory.
   
   @@ -269,6 +277,7 @@ protected static function configFactory() {
   +   *   The element info service.
   

   Let's remove these unrelated changes from the patch.

3. I think we should be able to make this change as it shouldn't be too disruptive, but we should create a small change record just in case.

+++ b/core/modules/filter/src/Element/TextFormat.php
@@ -124,7 +124,7 @@ public static function processFormat(&$element, FormStateInterface $form_state,
+      '#attributes' => ['class' => ['filter-wrapper', 'js-filter-wrapper']],

@@ -164,7 +165,12 @@ public static function processFormat(&$element, FormStateInterface $form_state,
+          'filter-guidelines',

@@ -183,7 +189,7 @@ public static function processFormat(&$element, FormStateInterface $form_state,
+      '#attributes' => ['class' => ['filter-list', 'js-filter-list']],

I'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.

I'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.

We 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.

We 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.

Couldn't we add them back in a pre_render function?

I cannot see the benefits.

I cannot see the benefits.

This 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 the js- prefixed class in its JavaScript.

Updated Drupal version to 8.7.x.

Attached patch addresses comments in #7 and #8

+++ b/core/themes/stable/stable.theme
@@ -23,3 +23,29 @@ function stable_preprocess_links(&$variables) {
+  $element['format']['#attributes']['class'][] = 'js-filter-wrapper';
+  $element['format']['guidelines']['#attributes']['class'][] = 'js-filter-guidelines';
+  $element['format']['format']['#attributes']['class'][] = 'js-filter-list';

Thank 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.

Switched 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.

Now 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 🤷‍♂️

+++ b/core/modules/filter/src/Element/TextFormat.php
@@ -195,7 +196,6 @@ public static function processFormat(&$element, FormStateInterface $form_state,
-      '#attributes' => ['class' => ['filter-help']],

This class should be also added back in the Stable pre_render function.

filter-help class re-added in stable.

filter.admin.css file removed from filter module, and references from filter.libraries.yml removed.

+++ b/core/modules/filter/src/Element/TextFormat.php
@@ -79,7 +79,7 @@ public function getInfo() {
-  public static function processFormat(&$element, FormStateInterface $form_state, &$complete_form) {
+  public static function processFormat(array &$element, FormStateInterface $form_state, array &$complete_form) {

Adding these type hints it outside the scope of this issue so let's remove them from the patch.

Type hints removed.

Stable 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.

#22 addressed via hook_library_info_alter() implementation in stable theme.

Reuploaded 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.

The 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.

+++ b/core/themes/stable/stable.theme
@@ -25,6 +41,33 @@ function stable_preprocess_links(&$variables) {
+    $info['text_format']['#pre_render'][] = 'stable_pre_render_text_format';
...
+function stable_pre_render_text_format(array $element) {

I 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.

#pre_render changed to #process as requested in #26

This looks good! I created a draft change record for this.

I've confirmed manually that:

1. Themes extending stable get the pre-existing classes and CSS from Stable
2. Library overrides remain intact in themes that have overridden Stable filter.admin.css
3. Classes and CSS have been removed from markup in themes that are not extending Stable

This looks good for me! Thank you @Eli-T!

Thanks 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.

This 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.

Committed 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).