Regression: Views module CSS is not attached

I'm having this same issue. Fresh install of beta12. Created a Grid view. Displays an image field using an image style. Set to 4 columns.

The nodes (images) each show on separate lines, rather than all in a row. This worked fine in beta9 (yeah I know, it's been a while since I've been able to do D8 testing).

Looking at the markup, the columns are set to 25%, but have "display: block". Changing this to "display: inline-block" fixes the problem. But not sure where to apply this change in D8.

I tried it too.

I am looking for the commit that may have done this. It could be #2329917: Move views classes from preprocess to templates but that is just an early guess.

I have looked back as far as beta8 and the regression is there.

Meh, I hate if things randomly breaks. Let's ensure that this doesn't happen in the future.

Grids were working on page displays in beta11, but not in admin preview

Does that mean we also somehow miss to add CSS files in preview?

The earliest version of D8 I have on this machine is beta2, page views work, preview does not work.

An up to date version of D7 + Views have both operating correctly.

Perhaps the admin preview was not ported completely for D8? The page view issue is recent as I generated the image on the Avatars project page with grid view on beta11.

It looks like we're getting the right classes. views-view-grid horizontal cols-4 clearfix

Did we lose some CSS at some point?

Should this be critical? I don't think we want to ship with broken displays.

/* Grid style column align. */
.views-view-grid .views-col {
  float: left;
}

views.module.css is not being loaded.

On beta 7, Bartik works but not Seven.

This has been broken a while - I am back to beta 5 and it is the same thing as beta 7: Bartik works but not Seven

The code to attach the library is in ViewExecutable.php

  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {
    // Reference the storage and the executable to each other.
    $this->storage = $storage;
    $this->storage->set('executable', $this);
    $this->user = $user;
    $this->viewsData = $views_data;
    $this->routeProvider = $route_provider;

    // Add the default css for a view.
    $this->element['#attached']['library'][] = 'views/views.module';
  }

Is this not executing?

The library itself is registered fine. I can force it to be added directly in a theme info file.

Beta 1 is the same thing as beta 7 - I am not looking back any more.

Maybe DisplayPluginBase::buildBasicRenderable() should be attaching the library from the element in ViewExecutable.

This is a test to see if I am in the right place. This fixes it for Bartik. This is not how one would really fix it.

This fixes it in Seven and Bartik. Again, this is not an elegant solution.

I think need to attached styles in views.theme.inc in function template_preprocess_views_view_grid

And screenshot in preview.

I don't think this is actually a grid problem. Views is not attaching the views.module library. It is manifesting itself with the grid, because the only CSS in that file is for table and grid displays.

Update patch #23 add attached for table.

Add screenshot.

+++ b/core/modules/views/views.theme.inc
@@ -670,6 +670,8 @@ function template_preprocess_views_view_table(&$variables) {
+  // Attached style for table.
+  $variables['view']->element['#attached']['library'][] = 'views/views.module';

@@ -771,6 +773,8 @@ function template_preprocess_views_view_grid(&$variables) {
+  // Attached style for grid.
+  $variables['view']->element['#attached']['library'][] = 'views/views.module';

I'm a bit confused, should not the actual preprocess_views_view add the library?

@dawehner may be styles added once in template_preprocess_views_view ? Or in template_preprocess_views_view_table and template_preprocess_views_view_grid attached twice them when they are needed.

Yeah, what I was trying to get across in #25 is that this isn't just about the grid. Attaching the library in that construct() doesn't work. I don't know enough about Views to know if it should work, or we should be moving it elsewhere. Maybe the theme system could once process attachments at that level, but now can't? I don't know. But if the attachment line is moved, it should be someplace general, like preprocess_views_view.

I suppose it could live in _view_grid and _view_table if we are ok with saying that this css only matters for those two and don't want it getting include for others. But, in that case, we should probably change the name of the library.

@davidhernandez I tested and add to template_preprocess_views_view and it works fine. But it is necessary to do it?

I suppose it could live in _view_grid and _view_table if we are ok with saying that this css only matters for those two and don't want it getting include for others. But, in that case, we should probably change the name of the library.

Yeah exactly. Maybe someone adds some generic CSS and then we basically end up with the same bug as this issue again.

@dawehner update patch #27 attached style in template_preprocess_views_view tested works fine. Probably it would be more correct.

Add screenshot.

Update patch #34.

This is a different path, much simpler.

Turns out $view->element was being assigned a new value. This path simply merges in defaults.

Cause #2381277: Make Views use render caching and remove Views' own "output caching" (15c848bd6)

@dpi That is the problem I was trying to overcome in #21. #38 is simpler.

The fail is because the json response data is now missing the add_css command.

    [1] => Array
        (
            [command] => add_css
            [data] => <link rel="stylesheet" href="http://d8.localhost/core/modules/views/css/views.module.css?nrfuxk" media="all" />

        )

...but this is ok, right? Since the css is actually already in the page, it doesn't need to get added again. If so, we just need to update the ViewAjaxTest test.

To actually test this change we can put an assert looking for the css file, instead of creating a new test, if there is some place generic to do it. Look for a plain "Does this View display" web test.

Tested patch #38 it does not work for view (add screenshot). So may be patch #27 is correct?

It works, clear your cache.

@dpi I clear cache, but still not work for edit views.

Three issues that need an answer:

1. Before the patch is applied, it looks like somehow the AJAX request is getting instructions to add the CSS:
   1. But the JS does not add it to the page, see admin previews
   2. CSS is getting discovered for AJAX, but not Pages.
2. I thought I would fix the issue by merging the variables, but the CSS then disappears from the AJAX response.

@Chernous_dn patch does not fix admin previews. That issue existed before the regression and seems unrelated. See #46 1:1

@dpi I'm talking about the patch #27 it works for page, edit views and ajax rebuild views.

I understand it works, but I would like to get a Views maintainer to take a look at the issue to see if it is the correct resolution.

I would like to know why adding the CSS into Element/View::getInfo() is not a better fix, and why that also does not work.

#27 would not be a final fix as you would have to remove existing code from ViewExecutable.

@dpi I understand, thanks for the explanation.

Mh did anyone tried to research why #38 fails for ajax? If the proper defaults are merged it, it sounds like a good change.

See #42.

See #42.

Well right, but why does that break the admin preview, according to @dpi on IRC.

Turns out AJAX test is getting a different response to what my browser receives.

Firefox views preview

[
  {
    "command": "settings",
    "settings": {....},
    "merge": true
  },
  {
    "command": "insert",
    "method": "append",
    "selector": "body",
    "data": ....,
    "settings": null
  },
]

ViewAjaxTest JSON response

[
  {
    "command": "settings",
    "settings": {....},
    "merge": true
  },
  {
    "command": "add_css",
    "data": ....
  },
  {
    "command": "insert",
    "method": "replaceWith",
    "selector": ".js-view-dom-id-",
    "data": ....
    "settings": null
  }
]

Thats why tests are OK in head I guess

Well right, but why does that break the admin preview, according to @dpi on IRC.

Dunno, but I can confirm the preview is not working. I guess the construct in ViewExecutable was a better place to put it initially, and that's why it was there. Does the ajax request then not hit any pre-rendering?

$executable->destroy(); in ViewUI::renderPreview() seems to reset $executable->element and lose the attached default library that is set in __construct, see #16.

Would this work?

Not sure of the fix, but it works. The CSS is there for the page and preview. Obviously, attaching the library for all Views will make it work, I'm just not entirely clear of the original intention.

The add_css would not be in the response because the CSS is already there, so I removed that from the test. Makes sense?

What's the status of this one? Is the patch in #59 the current recommended solution? #59 works for me on both the admin preview and main page showing the view. But don't know enough about the other issues in this thread to mark it RTBC yet.

Looks like we're still missing some test coverage here.

It should be ok to add the CSS on all views (views.module.css is quite small).

This adds a test to check for that file on grid views (I picked the grid test since that prompted this issue initially).

We have test coverage now. Any takers to review?

I applied the patch in #62 and the grid CSS now works in preview and in the Bartik theme.

Before patch in #62

After patch in #62

patch #62 works perfect. Thank you jhedstrom

Committed 3d0a938 and pushed to 8.0.x. Thanks!