Add js- prefixed classes for separation of JS functionality & CSS of Dropbutton element

I reviewed and tested and looks good to me. I attach a new version with few new catches where the 'js-' class is missing

I'm wondering if https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/langua... should be updated to the new one, as long is more functional than CSS, but not JS. I added it just in case.

Finally is not clear why in some tests is it updated to the prefixed class and some case the other one is kept. I don't change anything in tests as long looks they are passing right now.

1. +++ b/core/misc/dropbutton/dropbutton.es6.js
   @@ -3,7 +3,7 @@
   -(function($, Drupal) {
   +(($, Drupal) => {
   

   🤓 Übernit: This seems out of scope to change.

2. +++ b/core/misc/dropbutton/dropbutton.es6.js
   @@ -36,7 +36,7 @@
   -    this.$list = $dropbutton.find('.dropbutton');
   +    this.$list = $dropbutton.find('.js-dropbutton');
   

   ✅ 😨 This looks like a big BC break! — see next point 😅

3. +++ b/core/misc/dropbutton/dropbutton.js
   @@ -94,7 +94,7 @@
   -      return '<li class="dropbutton-toggle"><button type="button"><span class="dropbutton-arrow"><span class="visually-hidden">' + options.title + '</span></span></button></li>';
   +      return '<li class="dropbutton-toggle js-dropbutton-toggle"><button type="button"><span class="dropbutton-arrow"><span class="visually-hidden">' + options.title + '</span></span></button></li>';
   
   +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -504,7 +504,7 @@ public function testAdminBookNodeListing() {
   -    $elements = $this->xpath('//table//ul[@class="dropbutton"]/li/a');
   +    $elements = $this->xpath('//table//ul[@class="dropbutton js-dropbutton"]/li/a');
   

   👍 This proves that there is no BC break: the old classes continues to exist, we're only adding new ones!

4. +++ b/core/modules/language/config/optional/tour.tour.language.yml
   @@ -44,7 +44,7 @@ tips:
   -      data-class: dropbutton-wrapper
   +      data-class: js-dropbutton-wrapper
   

   😲First reaction: wow, incredible that you thought of the selectors in tours! :D

   🤔 Second reaction: The old class continues to exist, so why this change? Even if it's harmless, tours are config entities, and those are not updated automatically, so this opens a can of worms wrt updating tours that I'd rather avoid :)

All easy things to fix, once those are fixed, this will be an easy RTBC :) Nice work!

A reroll was needed to apply the patch from #6 because in the meanwhile there where some changes.
Applied all suggestions from #7. Thank you for the review! :)

I really wanted to RTBC this. The second point is so minor I could've RTBC'd, but the first one needs to be reverted or at least needs to be justified.

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -233,8 +233,8 @@ protected function doTestTranslationOverview() {
   -        $this->assertEqual((string) $elements[0], t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
   ...
   +        $this->assertEqual((string) $elements[0], t('Edit'), format_string('Edit link correct for %language translation.', ['%language' => $langcode]));
   

   🤔 Why this change? That did not exist in #6.

2. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -628,4 +628,4 @@
   -})(jQuery, Drupal, drupalSettings);
   \ No newline at end of file
   +})(jQuery, Drupal, drupalSettings);
   

   🤓 Übernit: this change was probably applied by your IDE/text editor, but we should revert it nonetheless.

I corrected your suggestions! :)
The new line was introduced in #6, I also checked my IDE. Thanks for pointing that out!

Perfect! Ship it :) 🚢

+++ b/core/modules/system/templates/dropbutton-wrapper.html.twig
@@ -13,7 +13,7 @@
 #}
 {% if children %}
   {% spaceless %}
-    <div class="dropbutton-wrapper">
+    <div class="dropbutton-wrapper js-dropbutton-wrapper">
       <div class="dropbutton-widget">
         {{ children }}
       </div>

+++ b/core/modules/views_ui/src/ViewEditForm.php
index ca68716db2..0bbc877154 100644
--- a/core/themes/classy/templates/form/dropbutton-wrapper.html.twig

--- a/core/themes/classy/templates/form/dropbutton-wrapper.html.twig
+++ b/core/themes/classy/templates/form/dropbutton-wrapper.html.twig

+++ b/core/themes/classy/templates/form/dropbutton-wrapper.html.twig
@@ -11,7 +11,7 @@
 {% if children %}
   {% spaceless %}
-    <div class="dropbutton-wrapper">
+    <div class="dropbutton-wrapper js-dropbutton-wrapper">
       <div class="dropbutton-widget">

+++ b/core/themes/stable/templates/form/dropbutton-wrapper.html.twig
@@ -11,7 +11,7 @@
 {% if children %}
   {% spaceless %}
-    <div class="dropbutton-wrapper">
+    <div class="dropbutton-wrapper js-dropbutton-wrapper">
       <div class="dropbutton-widget">
         {{ children }}
       </div>

I wonder if these changes are going to result in regressions for themes. It looks as some do override this markup - see http://codcontrib.hank.vps-private.net/search?text=dropbutton-wrapper&fi... - for example https://git.drupalcode.org/project/material_admin/blob/8.x-1.x/templates...

I think we need a change record here but maybe also we need to think about a different approach that is less disruptive? Going to ping @lauriii as a frontend framework manager.

Going to ping @lauriii as a frontend framework manager.

alexpott 59 minutes ago
@lauriii Am I on the wrong track here - https://www.drupal.org/project/drupal/issues/3057281#comment-13268776

lauriii  42 minutes ago
I think you’re right

lauriii  42 minutes ago
there’s a high risk of regression there

#10 Patch Failed

#20 Please review the patch

Patch #20 failed the tests and doesn't apply.

I am curious about the cost/benefit of this change. If it's such a huge potential source for BC-breaking changes, what benefits are we getting besides a separation of classes for styles & scripts? Is it still worth doing?

If the changes are still worth making, do we actually need to update the various test files that are contained in previous patches? Shouldn't xpath('//table//ul[@class="dropbutton"]/li/a') work just as well as xpath('//table//ul[@class="dropbutton js-dropbutton"]/li/a')?

Just re-rolled the patch in #20, thanks!

I've never written tests, but it looks like this is failing because my assumption in #23 is incorrect.

After applying the patch in #24, line 649 fails. Line 648 is checking the dropbutton elements to count how many book pages exist and is using xpath('//table//ul[@class="dropbutton"]/li/a') to do so. It looks like there is another instance we need to add js-dropbutton in line 659 as well.