Class HTML attribute is present even when no class is assigned to the list

It would be great for people to fix the issue if you woul have provided a view configuration which results in such an output, but I guess people will be able to figure out.

This fixes it for me. I applied the same logic to the $wrapper_class as well. Views adds a default wrapper class but if you clear it out, it also adds an empty class tag.

Missed a new line for coding standards.

Last diff added the new line AND unwanted spaces. Time to find a better editor.

Let's try to write a patch as well, so this never happens in the future.

Let's try to write a patch as well, so this never happens in the future.

I assume you mean a test? If so, I'll see what I can do. I'm new to drupal's test framework.

Ups, yeah indeed.

So we would want a test that verifies we don't have an empty class on any field for all of Views, right? Not just for the HTML list?

I cannot reproduce the bug.

Here are the steps I took:
1. Installed the latest version of D 8.x
2. Created an article and a basic page
3. Created a view listing published content.

Result: Cannot find any ul html tag on my page (inspected with firebug).

Never mind. The issue applies if we select the format of the view to be HTML list.

I reviwed the working patches from #3 and #5 and they work, everything's fine about them but I thought it would be nice to have them both in one .patch file and made a new patch combining those two patches.

Previous comment submitted by mistake. It was incomplete.

So, This is how I tested the patch. It works!
1. Created sample content of content-type `article`
2. Created a view and selected display as "HTML List"
3. when browsed to the view the html generated is as show:

4. applied patch and then then the html generated is as shown:

5. Under "Format Settings for that view" added `custom-list-class` to "List Class". HTML generated is as shown.

Re #15: For some attributes, i.e. data attributes (and others) empty attributes *should* be displayed, it's just that for class it doesn't make any sense.

Again, that's not necessarily correct. <div data-foo> is totally correct and wanted markup whereas <div data-foo="whatever"> is not as correct/semantic.

Looks like this is still in need of an automated test, per #6/8. #14 looks like it has some good steps to reproduce.

Also, minor, but if we could clean this up in the next re-roll:

+++ b/core/modules/views/views.theme.inc
@@ -944,20 +944,27 @@ function template_preprocess_views_view_list(&$variables) {
+  if($handler->options['class']) {
...
+  if($handler->options['wrapper_class']) {
...
+  if(isset($class)) {

Should have a space separating "if" from "("

As per #14, patch looks good and working fine , as per #19 i am uploading patch after cleanup.

+++ b/core/modules/views/views.theme.inc
@@ -944,20 +944,26 @@ function template_preprocess_views_view_list(&$variables) {
+  if(isset($class)) {
+    $variables['list']['attributes'] = new Attribute(array('class' => $class));
+  }

I think the space should be added here as well?

Patch with correct spacing for if statements.

Patch #22 worked for me.

This still seems to lack a test. It seems like you could take one of the existing tests and check the output to make sure there's no extra class output?

Working on tests!

Added a test to check for this and a patch to with the fix as well.

1. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleHtmlListTest.php
   @@ -0,0 +1,50 @@
   + * Definition of Drupal\views\Tests\Plugin\StyleHtmlListTest.
   

   Nitpick alarm, sorry. We do use now @file\nContains ... all over the place, but forget to convert some places. anyone wants to volunteer to convert all the other instances?

2. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleHtmlListTest.php
   @@ -0,0 +1,50 @@
   +class StyleHtmlListTest extends StyleTestBase {
   

   Just a quick note: you should be able to use ViewUnitTestBase which executes much faster

3. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleHtmlListTest.php
   @@ -0,0 +1,50 @@
   +    $view = Views::getView('test_view');
   +    $view->setDisplay();
   +    $style = $view->display_handler->getOption('style');
   +    $style['type'] = 'html_list';
   +    $style['options']['class'] = '';
   +    $view->display_handler->setOption('style', $style);
   

   I guess we want to provide some configured view instead.

4. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleHtmlListTest.php
   @@ -0,0 +1,50 @@
   +
   +    // Check that an empty class attribute is not added if no class has been set.
   +    $this->assertTrue(strpos($output, '<ul>') !== FALSE, 'Make sure no empty class is added to UL when no class is set');
   

   This approach of testing is a bit odd, but it certainly works.

here is some initial improvements on the test patch
wanted to implement more of the changes that dawehner suggested but this seems to work as it is a copy of Drupal\views\Tests\Plugin\StyleUnformattedTest
maybe catch me on irc if you think it needs more work and maybe point me in the right direction?
THank you :)

Rewrote test. Please review.

Any feedback?

Tested and works as described:
1) no class was set (no empty class tag) with no class assigned
2) custom classes on row and list came through after they were assigned

I hope this is helpful. Sorry about the screenshots in 2 comments : (. Still getting my head around how to embed those...

(review and testing part of #Drupal8NZ)

I think we can simply the preprocess function and have less if's by doing something like:

diff --git a/core/modules/views/views.theme.inc b/core/modules/views/views.theme.inc
index 2a23621..4d9fee3 100644
--- a/core/modules/views/views.theme.inc
+++ b/core/modules/views/views.theme.inc
@@ -901,23 +901,17 @@ function template_preprocess_views_view_list(&$variables) {
   if ($handler->options['class']) {
     $class = explode(' ', $handler->options['class']);
     $class = array_map('drupal_clean_css_identifier', $class);
+    // Initialize a new attribute class for $class.
+    $variables['list']['attributes'] = new Attribute(array('class' => $class));
   }
 
   // Fetch wrapper classes from handler options.
   if ($handler->options['wrapper_class']) {
     $wrapper_class = explode(' ', $handler->options['wrapper_class']);
-    $wrapper_class = array_map('drupal_clean_css_identifier', $wrapper_class);
+    // Initialize a new attribute class for $wrapper_class.
+    $variables['attributes']['class'] = array_map('drupal_clean_css_identifier', $wrapper_class);
   }
 
-  // Initialize a new attribute class for $wrapper_class.
-  if (isset($wrapper_class)) {
-     $variables['attributes']['class'] = $wrapper_class;
-   }
-
-  // Initialize a new attribute class for $class.
-  if (isset($class)) {
-    $variables['list']['attributes'] = new Attribute(array('class' => $class));
-  }
   $variables['list']['type'] = $handler->options['type'];
 
   template_preprocess_views_view_unformatted($variables);

Also lets improve the test to check that adding a class and a wrapper class works as expected since we've changed that code.

Thanks for reviewing. Here is new patch.

+++ b/core/modules/views/src/Tests/Plugin/StyleHtmlListTest.php
@@ -0,0 +1,58 @@
+    //Check that an empty class attribute is not added if the wrapper class is
+    //not set.
...
+    //Check that an empty class attribute is not added if the list class is
+    //not set.
...
+    //Set wrapper class and list class in style options.
...
+    //Check that class attribute is present if the wrapper class is set.
...
+    //Check that class attribute is present if the list class is set.

These comments need spaces after the // added per https://www.drupal.org/node/1354#inline

I fixed the comments as per #38. I also changed assertion comments to make them shorter and simpler and made a few other whitespace changes. There is an interdiff attached too.

Assigned to me for re-test > I am going to run another simplytest.me test before re-set to reviewed and tested by community

retested as per #34
without class

with class

working as described! Ta.

Committed 0d1b898 and pushed to 8.0.x. Thanks!