Installer is missing all of the global Seven theme stylesheets

HAHA, this is looking not too bad :p Is this about missing assets or about the wrong order of assets? Just curious.

Oh no :( Earlier while working on the patch, I manually tested the installer to verify that it was all working. So it must've been one of the later refactorings to the patch that caused this. I'll take a look at this unless somebody beats me to it.

Can confirm this is an issue.

<style media="screen">
@import url("http://drupal8alt.dev/core/themes/seven/css/base/elements.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/base/typography.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/admin-list.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/admin-options.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/admin-panel.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/block-recent-content.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/content-header.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/breadcrumb.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/buttons.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/buttons.theme.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/comments.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/messages.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/dropbutton.component.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/entity-meta.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/field-ui.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/form.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/help.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/menus-and-lists.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/modules-page.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/node.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/page-title.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/pager.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/skip-link.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/tables.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/tabs.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/tour.theme.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/update-status.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/components/views-ui.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/layout/layout.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/layout/node-add.css?0");
@import url("http://drupal8alt.dev/core/themes/seven/css/theme/appearance-page.css?0");
</style>

This is what we are missing.

This patch fixes the issue and adds a test. Not sure it is the correct fix - should we add the hook firing ability to BareHtmlPageRenderer?

It's not; we specifically don't want to fire hook_page_attachments() from BareHtmlPageRenderer.

The root cause is that this

function seven_preprocess_install_page(&$variables) {
  // Seven has custom styling for the install page.
  $variables['#attached']['library'][] = 'seven/install-page';
}

attaches a library, but the seven/install-page doesn't declare a dependency on the Seven theme's other CSS. We should convert Seven theme's stylesheets property into a libraries property, just like we've done for Bartik already many months ago.

Well yes but - what about maintenance page styling? Shouldn't we be ensuring that that theme's stylesheets are being added?

Here's a patch to make the library but this feels super fragile since the maintenance page and the install pages are only get styles because of seven_preprocess_maintenance_page and seven_preprocess_install_page. If a theme is set as the maintenance theme is it not reasonable to expect that that theme's stylesheets are added?

+++ b/core/lib/Drupal/Component/README.txt
@@ -1,13 +1,7 @@
-Drupal in order to function.
-
-Components MAY depend on other Drupal Components or external libraries/packages,
-but MUST NOT depend on any other Drupal code.
-
-In other words, only dependencies that can be specified in a composer.json file
-of the Component are acceptable dependencies.  Every Drupal Component presents a
-valid dependency, because it is assumed to contain a composer.json file (even
-if it may not exist yet).
+Drupal in order to function.  Components MAY depend on other Components, but
+that is discouraged. Components MAY NOT depend on any code that is not part of
+PHP itself or another Drupal Component.
 

This is from another issue :)

We should convert Seven theme's stylesheets property into a libraries property, just like we've done for Bartik already many months ago.

Are we saying that the stylesheets property is unreliable and every CSS file has to be added into a library? This would be the case for any theme that could be used during the install process right?

Re #13 yep - which is why I kinda disagree with the fix.

Rerolled patch in #10 properly and fixed tests.

This is a tricky one.

Theoretically we could treat every theme as a library internally like 'theme/seven' and automatically add a dependency to that "library" where we also add the active theme cache tag.

OR we go the route and deprecate direct JS / CSS usage and go more into the route of libraries everywhere, which has other major performance advantages as suggested by Wim.

The patch here is good, but the whole system feels still fragile.

#2377357: Several big visual regressions in installer was marked as duplicate. Bumping to critical as per that issue; the failed encoding of language names is just embarrassing. :)

Yeah due to the failed encoding it is impossible to pick the right language unless you know how your language name looks when incorrectly encoded as Latin-1, such as this screenshot for Turkish before / current:

So the patch in #6 fixes the language encoding too.

Whereas the library approach does not.

Bringing into multilingual sprint because this makes it impossible to pick many of the languages in the installer and know what you picked.

But there are other issues with BareHtmlPageRenderer and the installer:

• No favicon
• No generator metatag
• No viewport metatag
• The html before #2352155: Remove HtmlFragment/HtmlPage was <html lang="en" dir="ltr" class="install-background js"> now it is <html lang="en" dir="ltr" class=" js">
• Body was <body class="maintenance-page db-offline install-page path-"> now it is <body class="install-page db-offline path- one-sidebar sidebar-first">

The facts:

1. #6 invokes hook_page_attachments() for maintenance/install pages.
2. #14 converts Seven's stylesheets into libraries and uses a library dependency.
3. #6 indeed ensures all meta tags are also set up for maintenance/install pages, #14 does not.
4. hook_page_attachments() used to be named hook_page_build() and used to allow to do much more than just attaching assets, which is all that you can do with it in HEAD. The full reasoning was documented:
   

   -   * - drupal_prepare_page() and hook_page_build() cannot be invoked on the
   -   *   maintenance and install pages, since possibly enabled page layout/block
   -   *   modules would replace the main page content with configured region
   -   *   content.
   

   Since that is now no longer the case, it is now safe to invoke hook_page_attachments() for maintenance/install pages.

5. A theme's stylesheets were attached using #type => 'html''s #pre_render callback in #2352155: Remove HtmlFragment/HtmlPage, but near the very end of the review cycle for that patch, joelpittet requested that to be moved to system_page_attachments() (in #75.3 over there). It made total sense to me (and all reviewers), so we just did that.

   ---

   Hence I think it's best to go with #6 for now. But we need to make seven/maintenance-page declare a correct dependency still anyway, but that's for another issue then.

#22:

RE: your first 3 points: indeed, see #23.
RE: your last 2 points: no, that's impossible. I also can't reproduce that. I think you might've made a small caching or testing mistake that caused you to find these, because I can't reproduce this on simplytest.me with patches #6 and #14.

Re #23.1 the patch in #6 does not invoke hook_page_attachments - it just calls system_page_attachments. Should we invoke the hook?

Re #24 my last two points from #22 were comparisons of HEAD and pre #2352155: Remove HtmlFragment/HtmlPage - install-background looks unused so that is a good change. And on IRC @Wim Leers pointed out that this is not a maintenance page so that change is good too. So all that leaves is the one-sidebar sidebar-first - these don't look right to me.

It feels fishy to call only system_page_attachments(), but it's safer, since it doesn't allow other modules to break/complicate the maintenance/install pages. We're cleaning up BareHtmlPageRenderer anyway in #2373735: Simplify/clean up BareHtmlPageRenderer, so if we want to clean that up further, that'd be the perfect issue to do so.

@Wim Leers re #23.5 Does that mean we need to manually invoke these somewhere again for maintenance/install or will the library conversion negate the need to manual invoke those hooks?

// Modules and themes can alter page attachments.
\Drupal::moduleHandler()->alter('page_attachments', $attachments);
\Drupal::theme()->alter('page_attachments', $attachments);

FYI didn't write the particular comment (pair reviewing) but still stand by it:)

Also does this comment still ring true with the changes at the end of the review cycle?

+++ b/core/modules/system/theme.api.php
+ * 2. \Drupal\Core\Render\MainContent\HtmlRenderer::prepare() now is guaranteed
+ * to be working on a #type 'page' render array. hook_page_attachments() and
+ * hook_page_attachments_alter() are invoked.

Created the followups:

• #2377393: Seven seven_preprocess_html adds unused classes
• #2377397: Themes should use libraries, not individual stylesheets

Re-uploading patch in #6

Screenshot from #28

Thanks @alexpott, it's looking much better and lighter than invoking more than is needed, should we move that assert into a test or was that just temporary to avoid declaring seven styles as a hard dependency in the tests?

Edit: (adding context lines)

+++ b/core/modules/system/src/Tests/Installer/InstallerTest.php
@@ -32,4 +32,11 @@ public function testInstaller() {
+  /**
+   * Installer step: Select language.
+   */
+  protected function setUpLanguage() {
+    $this->assertRaw('core/themes/seven/css/components/buttons.css', 'Seven\'s css is attached.');
+    parent::setUpLanguage();
+  }

That assert is in a test - an InstallerTestBase test. The setupLanguage() method interacts with the first page of the installer - which is what we want to test. And we can ensure that we don't break the language character set again :)

'Seven\'s css is attached.' -> "Seven's css is attached."

Life is better without assertion messages since the default ones are more helpful anyway.

@alexpott oh I figured there was some reason behind that, looked strange to see asserts inside of a non test*() method. Thanks for explaining.

Unless anybody thinks otherwise or testbot explodes, I've manually tested this and it seems like the right approach and includes regression tests. RTBC!

Visually look great :)

Looks good, thanks :)

Don't love calls to system module in the installer but we haven't eliminated those from elsewhere in the installer yet, and better this than having things hard coded in _drupal_add_*() on every request.

Committed/pushed to 8.0.x, thanks!

Thanks all!