Problem/Motivation

View pages have a unnecessary

with class "views-element-container" surrounding them. Let's get rid of this.

Proposed resolution

Remove it.

Remaining tasks

  • Evaluate if div is really not needed
  • Provide a patch
Files: 
CommentFileSizeAuthor
#17 issue_2381277.tar_.gz1.38 KBjaxxed
#15 interdiff-2575405-7-15.txt2.09 KBmaijs
#15 is_the_div_with_class-2575405-15.patch2.89 KBmaijs
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,066 pass(es). View
#7 is_the_div_with_class-2575405-7.patch2.96 KBmartins.kajins
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,747 pass(es). View
#5 2575405-test-update-2-interdiff.txt2.25 KBmartins.kajins
#5 is_the_div_with_class-2575405-5.patch2.89 KBmartins.kajins
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,203 pass(es), 422 fail(s), and 22 exception(s). View
#2 is_the_div_with_class-2575405-2.patch652 bytesLukas von Blarer
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,089 pass(es), 4 fail(s), and 0 exception(s). View

Comments

Lukas von Blarer created an issue. See original summary.

Lukas von Blarer’s picture

Status: Active » Needs review
FileSize
652 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,089 pass(es), 4 fail(s), and 0 exception(s). View

The patch removes the container. We still need to fix the tests...

Status: Needs review » Needs work

The last submitted patch, 2: is_the_div_with_class-2575405-2.patch, failed testing.

The last submitted patch, 2: is_the_div_with_class-2575405-2.patch, failed testing.

martins.kajins’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,203 pass(es), 422 fail(s), and 22 exception(s). View
2.25 KB

Deleted "views-element-container" from tests

Status: Needs review » Needs work

The last submitted patch, 5: is_the_div_with_class-2575405-5.patch, failed testing.

martins.kajins’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,747 pass(es). View
jaxxed’s picture

in reviewing this patch with a colleague, we are concerned about the fact that this container is used for several js form classes (as can be seen in the test.)

Can we confirm that it is ok to remove the container? does it break any form js?

jaxxed’s picture

A colleague bisected and discovered that the container was added in this related issue : https://www.drupal.org/node/2381277

Colleague: https://www.drupal.org/u/maijs

jaxxed’s picture

the div contains a couple of classes which may have functional impact on the view:

    <div data-drupal-selector="edit-view" class="views-element-container js-form-wrapper form-wrapper" id="edit-view">

This indicates that in the case of an element render as a part of a form, the classes "js-form-wrapper form-wrapper" are added to the container div. Are these necessary?

Lukas von Blarer’s picture

As far as I see it the patch committed in the issue #2381277: Make Views use render caching and remove Views' own "output caching" only added the container in a different place as it did before.

Here are the two relevant sections of the patch.

The container was removed here:

diff --git a/core/modules/views/src/Element/View.php b/core/modules/views/src/Element/View.php
index 9c11d3b..676f6d0 100644
--- a/core/modules/views/src/Element/View.php
+++ b/core/modules/views/src/Element/View.php
@@ -23,7 +23,6 @@ class View extends RenderElement {
   public function getInfo() {
     $class = get_class($this);
     return array(
-      '#theme_wrappers' => array('container'),
       '#pre_render' => array(
         array($class, 'preRenderViewElement'),
       ),
@@ -31,6 +30,7 @@ public function getInfo() {
       '#display_id' => 'default',
       '#arguments' => array(),
       '#embed' => TRUE,
+      '#cache' => [],
     );
   }
 
@@ -38,8 +38,6 @@ public function getInfo() {
    * View element pre render callback.
    */
   public static function preRenderViewElement($element) {
-    $element['#attributes']['class'][] = 'views-element-container';
-
     if (!isset($element['#view'])) {
       $view = Views::getView($element['#name']);
     }
@@ -47,6 +45,17 @@ public static function preRenderViewElement($element) {
       $view = $element['#view'];
     }

And added again here:

diff --git a/core/modules/views/src/Element/View.php b/core/modules/views/src/Element/View.php
index 9c11d3b..676f6d0 100644
--- a/core/modules/views/src/Element/View.php
+++ b/core/modules/views/src/Element/View.php
@@ -70,7 +79,18 @@ public static function preRenderViewElement($element) {
           $element['#title'] = &$element['view_build']['#title'];
         }
 
-        views_add_contextual_links($element, 'view', $view, $view->current_display);
+        if (empty($view->display_handler->getPluginDefinition()['returns_response'])) {
+          // views_add_contextual_links() needs the following information in
+          // order to be attached to the view.
+          $element['#view_id'] = $view->storage->id();
+          $element['#view_display_show_admin_links'] = $view->getShowAdminLinks();
+          $element['#view_display_plugin_id'] = $view->display_handler->getPluginId();
+          views_add_contextual_links($element, 'view', $view->current_display);
+        }
+      }
+      if (empty($view->display_handler->getPluginDefinition()['returns_response'])) {
+        $element['#attributes']['class'][] = 'views-element-container';
+        $element['#theme_wrappers'] = array('container');
       }
     }

The question is if this is somehow relevant for the render caching. I guess not, right?

@jaxxed In what cases do you get those attributes?

dawehner’s picture

Category: Support request » Bug report

@jaxxed
Good observation. Did you figured out which patch exactly introduce that? Maybe that comment provides some insight why it might have been important at some point. If we don't need the class, I think we should remove the test code entirely, assertFalse() doesn't really help much IMHO.

jaxxed’s picture

I have actually not applied the patch, but am mentoring a sprint, and it was brought to my attention. Neither i, nor my colleague are certain the role, nor the impact of those classes, but we are looking at it next (I have one issue in queue ahead of this.)

colleague: https://www.drupal.org/u/maijs

I'm going to ask @maijs to comment here.

maijs’s picture

Going through the commit that introduced the div in question, it seems that view render element has been wrapped in container theme wrapper since the beginning without particular reason. The subsequent commits merely moved the container theme wrapper and views-element-container class from place to place, but it doesn't seem that there's any functional reason to have the div.

maijs’s picture

FileSize
2.89 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,066 pass(es). View
2.09 KB

Assertions that test for existence of (now removed) views-element-container class have been removed.

jaxxed’s picture

I've done a quick test on my hunch about the js-form-wrapper class, and how it can be found in the states functionality. If you remove the container div, as the patch does, then you will lose #states functionality on the view element.

If you had a form like this:

  public function buildForm(array $form, FormStateInterface $form_state) {
    
    $form['show_articles'] = array(
      '#type' => 'checkbox',
      '#title' => $this->t('Show Articles')
    );
    $form['articles'] = array(
      '#type' => 'view',
      '#name' => 'articles',
      '#display_id' => 'block_1',

      '#states' => array(
        'invisible' => array(
         ':input[name=show_articles]' => array('checked' => FALSE),
        ),
      ),
    );

    return parent::buildForm($form, $form_state);
  }

With the container div, the #states functionality will properly show and hide the view, when you check and uncheck the checkbox element. Without the container div, the #states functionality does not work.

This is an example of a possible reason to keep that div.

jaxxed’s picture

FileSize
1.38 KB

I've attached a module that I created to test the issue (ignore the fact that it's labelled after a different issue number)

Form can be seen at /test/issue_2381277/testembeddedviewin

maijs’s picture

I'm with @jaxxed, the <div class="views-element-container"> around view's own <div class="view"> allows setting #states and arbitrary attributes that you cannot easily set on the view's div directly, thus is should not be removed for the time being. In ideal world, I'd like to see the following:

$form['content'] = array(
  '#type' => 'view',
  '#name' => 'content',
  '#display_id' => 'page_1',
  '#attributes' => array(
    'class' => ['my-view-class'],
  ),
  '#states' => array(
    'invisible' => array(
      ':input[name=show_articles]' => array('checked' => FALSE),
    ),
  ),
);

being rendered into

<div class="view view-content view-id-content view-display-id-page_1 js-view-dom-id-c8d8a511fef3bf96418d89dc67f424b05cd7e4d26860805dfec21166e62c9ec2 my-view-class" data-drupal-states="{&quot;invisible&quot;:{&quot;:input[name=show_articles]&quot;:{&quot;checked&quot;:false}}}">

Unfortunately, this cannot be achieved easily, partly to the fact that views_view theme does not accept #attributes variable (take a look at the output of views_theme() for reference).

maijs’s picture

Status: Needs review » Active

Setting status to active to have the issue opened for discussion.

Lukas von Blarer’s picture

Issue tags: +Twig

Ok, this sounds quite unclean and is worth looking into further.

mgifford’s picture

Now we're at RC2, we have to deal with this a bit different if it is going to get in. I'm thinking this might be better bumped to 8.1 though since it's dealing with the HTML.

dawehner’s picture

Issue tags: +Needs manual testing

If we remove it we need to ensure that both ajax and more important probably context link handling still works, but yeah I'm not sure whether changing this now is a great idea.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

finalred’s picture

now i use drupal 8.3.0-rc1 and "views-element-container" still exist...