Problem/Motivation

Install head with English and when installing modules:

When setting up configuration:

When installing in a foreign language:

After installing in a foreign language (until you clear the cache):

Proposed resolution

Figure out what causes the visual problems. Fix.

Remaining tasks

Figure it out. Fix it.

User interface changes


API changes

TBD.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#60 2614014-2-60.patch4.97 KBalexpott
#60 50-60-interdiff.txt670 bytesalexpott
#50 interdiff.txt5.31 KBdawehner
#50 2614014-50.patch4.97 KBdawehner
#44 interdiff.txt2.87 KBCottser
#44 regression_progress-2614014-44.patch4.86 KBCottser
#41 2614014-41.patch4.85 KBalexpott
#41 2614014.41.test-only.patch888 bytesalexpott
#34 regression_progress-2614014-34.patch4.88 KBjoelpittet
#34 regression_progress-2614014-32--tests-only.patch920 bytesjoelpittet
#33 regression_progress-2614014-33--tests-only-fail.patch1.78 KBjoelpittet
#33 regression_progress-2614014-33-pass.patch3.99 KBjoelpittet
#32 interdiff.txt920 bytesdawehner
#32 2614014-31.patch3.11 KBdawehner
#30 Webhely beállítása | Drupal 2015-11-16 20-05-40.png174.53 KBGábor Hojtsy
#13 Installing_Drupal___Drupal.png336.16 KBjoelpittet
#13 Database_configuration__.png237.27 KBjoelpittet
#12 progress_bar_looks-2614014-12.patch2.21 KBjoelpittet
#11 ThemeManager_php_-_html_-____Sites_d8_html_.png137.33 KBjoelpittet
#10 progress_bar_looks-2614014-10.patch980 bytesLewisNyman
#6 Database_configuration__.png84.7 KBjoelpittet
Installing Drupal | Drupal 2015-11-12 12-58-57.png823.35 KBGábor Hojtsy

Comments

Gábor Hojtsy created an issue. See original summary.

LewisNyman’s picture

It looks like Classy's progress.css isn't being loaded. The library should be attached in classy/progress-bar.html.twig

Gábor Hojtsy’s picture

Component: other » Classy theme

Assigning to classy then(?)

davidhernandez’s picture

We should check to see if it is loading Stable or core's progress-bar template since those don't load a library.

davidhernandez’s picture

Component: Classy theme » install system

Yup

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'progress_bar' -->
<!-- BEGIN OUTPUT from 'core/themes/stable/templates/misc/progress-bar.html.twig' -->
<div class="progress" data-drupal-progress>
    <div class="progress__track"><div class="progress__bar" style="width: 0%"></div></div>
  <div class="progress__percentage">0%</div>
  <div class="progress__description">Initializing.<br />&nbsp;</div>
</div>

<!-- END OUTPUT from 'core/themes/stable/templates/misc/progress-bar.html.twig' -->

All the installer's templates are coming from Stable, which does not have a progress bar library. I'm not sure why, but the installer is loading both Classy and Stable, but only acts like its template suggestions go to Stable.

It doesn't look to be a Classy issue. It certainly was unearthed when we added all the Stable stuff, which was a recent commit. I think though this is an installer special snowflake issue?

joelpittet’s picture

Issue tags: +rc target triage
FileSize
84.7 KB

Wondering if this is also related: The fieldset on installer looks a bit wrong and doesn't have the CSS to toggle it: EDIT: has JS toggling just doesn't look good or work.

joelpittet’s picture

Seems to be loading all the CSS on that page just for that details template it's loading stable instead of classy's template, bizarre.

dawehner’s picture

The same thing can be also observed when running a test via the UI.Rebuilding the cache helped, sorry

LittleCoding’s picture

the templates use of Stable theme templates may have to do with the changes made by Make Classy extend from the new Stable base theme

LewisNyman’s picture

Status: Active » Needs review
FileSize
980 bytes

@joelpittet It could be, because no templates are being loaded from Classy. Templates are being loaded from Seven though, so it could be that templates are only being loaded from the highest base theme on the installer only.

This patch fixed the reported symptom by copying the Classy template over to Seven. There's something else going on here but if we run out of time to fix this before release it would be great if the installer did ship looking like crap.

joelpittet’s picture

Priority: Normal » Major
Issue summary: View changes
FileSize
137.33 KB

Working through this:
core/includes/theme.maintenance.inc:96

Still trying to see what's at the root of it but it looks like it's only dealing with one of the base themes at that line.

Also this breakpoint is weird saying that classy is the baseTheme for Stable and Classy.

joelpittet’s picture

Still not perfect because Stable is the basetheme for stable for some reason but at least things seem to be loading correctly. It just looks like it was loading the base themes in reverse.

I could be completely wrong, please try it out. And bumping to major

joelpittet’s picture

Things look good here:

joelpittet’s picture

As this affects the product, I'm tagging for Angie to have a look as I really thing this needs to get in for release.

dawehner’s picture

  1. +++ b/core/includes/theme.maintenance.inc
    @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
    +    $base_themes[] = $themes[$themes[$ancestor]->base_theme];
         $ancestor = $themes[$ancestor]->base_theme;
    

    Do you mind making it a bit easier to read, so just use $ancestor = ...; $base_themes[] = $themes[$ancestor];

  2. +++ b/core/includes/theme.maintenance.inc
    @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
    -  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], array_reverse($base_theme)));
    +  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], $base_themes));
    

    This is the right fix, as its the same as we did in \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName itself.

xjm’s picture

+++ b/core/includes/theme.maintenance.inc
@@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
   // @todo This is just a workaround. Find a better way how to handle themes
   //   on maintenance pages, see https://www.drupal.org/node/2322619.
-  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], array_reverse($base_theme)));
+  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], $base_themes));

+++ b/core/lib/Drupal/Core/Theme/ThemeInitializationInterface.php
@@ -57,8 +57,8 @@ public function loadActiveTheme(ActiveTheme $active_theme);
-   *    An array of extension objects of base theme and its bases. It is ordered
-   *    by 'oldest first', meaning the top level of the chain will be first.
+   *   An array of extension objects of base theme and its bases. It is ordered
+   *   by 'next parent first', meaning the top level of the chain will be first.

So this is actually a pretty significant behavior change, no? When did this bug get introduced?

Defnitely would need tests. The inline comment also links this other issue:
#2322619: Remove workarounds in the theme system for the maintenance page and the installer

I agree that it would be extremely unsightly to ship 8.0.0 with this regression and we should fix it before release if possible. However, the patch points to a much deeper problem and who knows what side effects it would have, which makes me nervous. Tests would help.

xjm’s picture

+++ b/core/includes/theme.maintenance.inc
@@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
-  $base_theme = array();
+  $base_themes = array();
...
-    $base_theme[] = $themes[$themes[$ancestor]->base_theme];
+    $base_themes[] = $themes[$themes[$ancestor]->base_theme];

Also, technically these changes aren't in scope, but I'm okay with it.

dawehner’s picture

dawehner’s picture

So yeah IMHO the code should as similar as possible to \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName
and then maybe in 8.1 we could get rid of that sadness.

Wim Leers’s picture

#19++

Gábor Hojtsy’s picture

Title: Progress bar looks broken right in installer » Progress bar, fieldsets broken in the installer
Issue summary: View changes

Updated title / summary with fieldset problem.

Gábor Hojtsy’s picture

Issue summary: View changes
webchick’s picture

Title: Progress bar, fieldsets broken in the installer » [regression] Progress bar, fieldsets broken in the installer
Issue tags: +regression

Definitely would love to see this in before release. Not sure what other feedback you need from a PM point of view.

joelpittet’s picture

Feel free to work on tests , I won't be able to until later and don't want to hold this up.

It looks like it never worked quite right and the second base theme just exposed it

xjm’s picture

Title: [regression] Progress bar, fieldsets broken in the installer » [regression] Progress bar, fieldsets broken in the installer due to theme ordering bug
dawehner’s picture

I'll try to come up with some regression tests.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Talked to @dawehner and found some time so I'll take the tests on.

@xjm can you look at 'blessing' this with "rc target"?

catch’s picture

Issue tags: -rc target triage +rc target

I discussed this with xjm a bit, and I think it was enough discussion to tag it.

joelpittet’s picture

Thank you for discussing it @xjm and @catch.

I'm adding a new Test. The approach I'm planning is to check the ActiveTheme on the language page and Assert on the base themes if possible. As I don't think targeting specific CSS/markup is a decent test unless I write 2-3 installer sub themes and check their inheritance.

Hope that will suffice.

Gábor Hojtsy’s picture

Title: [regression] Progress bar, fieldsets broken in the installer due to theme ordering bug » [regression] Progress bar, fieldsets, messages broken in the installer due to theme ordering bug
Issue summary: View changes
FileSize
174.53 KB

Messages also look messed up (lacking styling):

Gábor Hojtsy’s picture

Priority: Major » Critical
Issue summary: View changes
FileSize
108.93 KB

I found yet one more bug that this patch fixes. If you install HEAD in a foreign language (in this case Hungarian), then your breadcrumbs and your seven shortcut star shows up all broken. (Until you clear your cache). I think the set of problems at this point is worthy of the critical label.

The proposed patch fixed the messages in the installer and this problem after a foreign language install as well.

dawehner’s picture

So we could do something like this ...

joelpittet’s picture

Here's my attempt, need to merge in #32

joelpittet’s picture

Added 32 tests and here is #32 tests only patch as well.

The last submitted patch, 32: 2614014-31.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33-pass.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33--tests-only-fail.patch, failed testing.

LittleCoding’s picture

The patch from #34is working for a clean install with RC4, PHP 5.5.16 and MySQL 5.6.24

The last submitted patch, 32: 2614014-31.patch, failed testing.

joelpittet’s picture

I've been trying to get the test only patch from #32 to fail but locally I can't get it to fail either. The idea was neat but I don't know how to ensure that condition will evaluate on the batch page. (Was testing through the UI)

alexpott’s picture

Here's a failing patch based on @dawehner's idea.

The last submitted patch, 41: 2614014.41.test-only.patch, failed testing.

joelpittet’s picture

Nice I'd say this looks like it has enough test coverage. Thanks for helping get both types of regression tests working @alexpott.

Cottser’s picture

Nits/documentation things, don't mind me. Everything else is looking good to me and I'd say this is ready to go.

  1. +++ b/core/includes/theme.maintenance.inc
    @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
    -  $base_theme = array();
    +  $base_themes = array();
    

    If we're going to chagne it might as well [].

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
    @@ -0,0 +1,45 @@
    +use Drupal\simpletest\KernelTestBase;
    +
    +
    +/**
    

    Nit: Extra blank line here.

  3. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
    @@ -0,0 +1,45 @@
    +   * Tests themes and base themes are correctly loaded.
    +   * Regression test for: https://www.drupal.org/node/2614014
    

    I think there needs to be a summary line and then a blank line per https://www.drupal.org/node/1354#general.

  4. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
    @@ -0,0 +1,45 @@
    +    // Do we have an active
    +    $this->assertTrue(\Drupal::theme()->hasActiveTheme());
    

    Do we have an active… theme?

  5. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
    @@ -0,0 +1,45 @@
    +    // Ensure base theme's have proper base themes and the right count.
    

    s/theme's/themes/, but this comment is hard to read IMO, I will try to reword it.

  6. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -41,6 +41,17 @@ protected function setUpSite() {
    +    // Ensure that we see the classy progress css on the page.
    

    Nit: s/css/CSS/.

Status: Needs review » Needs work

The last submitted patch, 44: regression_progress-2614014-44.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Testbot is ornery today

The last submitted patch, 10: progress_bar_looks-2614014-10.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33--tests-only-fail.patch, failed testing.

The last submitted patch, 41: 2614014.41.test-only.patch, failed testing.

dawehner’s picture

Fixed a couple of minor things here and there.

Status: Needs review » Needs work

The last submitted patch, 50: 2614014-50.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

DrupalCI you are lying.

The last submitted patch, 10: progress_bar_looks-2614014-10.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33-pass.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33--tests-only-fail.patch, failed testing.

The last submitted patch, 34: regression_progress-2614014-34.patch, failed testing.

The last submitted patch, 41: 2614014.41.test-only.patch, failed testing.

Wim Leers’s picture

  1. +++ b/core/includes/theme.maintenance.inc
    @@ -82,19 +82,21 @@ function _drupal_maintenance_theme() {
         if ($ancestor) {
    -      // Ensure that the base theme is added.
    +      // Ensure that the base theme is added and installed.
           $theme_handler->addTheme($themes[$ancestor]);
         }
    

    We state that this code block is basically a duplicate of \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName(). Yet we omit one important part that we see there:

            if ($ancestor == 'stable') {
              // Themes that depend on Stable will be fixed by system_update_8014().
              // There is no harm in not adding it as an ancestor since at worst
              // some people might experience slight visual regressions on
              // update.php.
              continue;
            }
    

    Shouldn't we then at least document why this part is different, why it's only a partial duplicate?

  2. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -41,6 +41,19 @@ protected function setUpSite() {
    +      $this->assertRaw('themes/classy/css/components/progress.css');
    

    This is the most ugly, the most hacky test I've ever seen I think… but I guess it's justified here.

    I'd almost say: let's add a @todo pointing to the visual regression testing issue, because once we have that, we don't need this hack anymore; we'll be able to test it much more elegantly/sanely.

    Also: why is the other test alone insufficient?

  3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
    @@ -0,0 +1,43 @@
    +   * Ensures that the maintenance theme initializes theme and their base themes.
    

    This reads kinda strange.

dawehner’s picture

We state that this code block is basically a duplicate of \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName(). Yet we omit one important part that we see there:

Well yeah, the thing is the block after that ... the exception when a theme is not installed yet ... at install time this is always the case, so the ThemeInitialization though we just have a workaround in place for some update path issue.

alexpott’s picture

Thanks @Wim Leers

  1. I agree with @dawehner I don't think we need a comment about that it is a pretty special situation that only occurs during update.php and for sites before rc-4.
  2. Yes it is ugly but also I don't think we should be adding @todo's without an issue to point to and visual regression testing is some way off afaik
  3. Fixed
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Given my contributions are all about test coverage and documentation I think it is okay if I RTBC this patch. It fixes the serious visual regression and adds complete test coverage.

dawehner’s picture

Title: [regression] Progress bar, fieldsets, messages broken in the installer due to theme ordering bug » Progress bar, fieldsets, messages broken in the installer due to theme ordering bug

Removing the regression tag given that all bugs are more or less a regression. There are better ways to add attention to an issue.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed f8ef22c on 8.0.x
    Issue #2614014 by joelpittet, alexpott, dawehner, Cottser, LewisNyman:...
Gábor Hojtsy’s picture

Yay, amazing, thanks all!

The last submitted patch, 12: progress_bar_looks-2614014-12.patch, failed testing.

The last submitted patch, 32: 2614014-31.patch, failed testing.

The last submitted patch, 33: regression_progress-2614014-33-pass.patch, failed testing.

The last submitted patch, 34: regression_progress-2614014-32--tests-only.patch, failed testing.

The last submitted patch, 34: regression_progress-2614014-34.patch, failed testing.

The last submitted patch, 41: 2614014.41.test-only.patch, failed testing.

The last submitted patch, 41: 2614014-41.patch, failed testing.

The last submitted patch, 44: regression_progress-2614014-44.patch, failed testing.

The last submitted patch, 50: 2614014-50.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 60: 2614014-2-60.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

  • catch committed f8ef22c on 8.1.x
    Issue #2614014 by joelpittet, alexpott, dawehner, Cottser, LewisNyman:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.