Remove breadcrumb from page template

The breadcrumb blocks are already defined for bartik and seven but aren't assigned a region. All that needs to be done is remove one line from the page.html.twig files in core and assign a region for the blocks in the block.block.bartik/seven.breadcrumbs.yml files. Apparently that hasn't been decided yet. The same problem exists for the primary and secondary menu blocks, when that issue is finally done. And since stark may have to run without the block modules enabled it may have to stay as is.
Some regions need to be defined to contain these elements that were formerly hard coded and are now blocks. Some have been suggested in other issues but have usually been very specific to the blocks being created(ie. main_menu region). I think they should be a little more generic and be layout related(eg, top_of_content, top_of_header, etc.). I'm sure someone can think of something better.

Tagging per discussion with @Crell.

Issue https://www.drupal.org/node/1256994 has been postponed pending the decision on this.

Just saying, in some themes it can be useful to have the breadcrumb as a fixed part of the page.tpl.php page template, and not have it floating in a region. Would it be easy enough for a theme developer to re-add it to the page variables?

Nope its a PITA to have some elements put inside a template file and other elements put into a config. So from a theme persective, we are totally OK with having everything inside of blocks, cause it make sense - lets not make exceptions just cause we used to do things back in D4.7

Menus & submenus are moved out of the page.html.twig file, so lets fix the same with breadcrumbs (and title, action links etc - but thats for another issue)

I am working on this.

Created a patch

Rerolled & I guess I fixed tests

testet it & it works
added to screenshots of bartik, seven & classy

+++ b/core/themes/bartik/templates/page.html.twig
@@ -130,7 +128,9 @@
+    {% if page.breadcrumb %}
+      {{ page.breadcrumb }}
+    {% endif %}

+++ b/core/themes/seven/templates/page.html.twig
@@ -77,7 +74,9 @@
+    {% if page.breadcrumb %}
+      {{ page.breadcrumb }}
+    {% endif %}

If there's no wrapper, there's no need for an if.

good catch wasnt thinking about that (was looking at some of the old crap we have in the page with if statements on menus etc)
cleaned up & added interdiff

dunno whats happening with the patch.. Its 0 bytes.

Great, one more thing is we should document the page.breadcrumb region in Seven's page.html.twig, Bartik has these docs. But while reviewing this more closely I don't think this is quite ready to fly. It really only implements part of the changes, and only for Seven and Bartik.

I am adding an initial beta evaluation table to the issue summary and updating the issue summary to outline some proposed remaining tasks, namely:

1. Is it possible to update the system page.html.twig to use the breadcrumb block? Seems like we were able to do that for primary and secondary menu. This also means adding a breadcrumb region to the default system list of regions, which I think is in \Drupal\Core\Extension\ThemeHandler::rebuildThemeData().

2. I think we actually want to remove the 'breadcrumb' variable from the page template rather than 'deprecating' it. This would be consistent with how the primary and secondary menu block conversions were handled. That means removing the code from template_preprocess_page(), and comes back to #1. If we can get these two sorted out we should end up with a green patch again :)

I think this a task, not a bug.

@Cottser, why do we need a region just for the breadcrumbs? Isn't that a bit too specific? RE point 2, yes, completely. We aren't in markup freeze, so we shouldn't need to worry about any backwards compatibility.

Also, we should probably include a change record?

Whether we include a region or not is bikesheddable. We could maybe stick it in the content region with a negative weight. I'm just basing that on the latest patch here.

I agree, there is enough regions already, maybe this should just be placed into an well-known region.

in the case of the need for bikesheed and the need for a region:

the "old breadcrumb" is in a complete seperate place, so translating that into being a part of part of page.content wont fly, its not the same place.

post patch: bartik

  <div id="main-wrapper" class="clearfix"><div id="main" class="clearfix">
    {{ breadcrumb }}

    <main id="content" class="column" role="main"><section class="section">
      {% if page.highlighted %}<div id="highlighted">{{ page.highlighted }}</div>{% endif %}
...
   {{ page.content }}

and in system:

  </header>

  {{ page.primary_menu }}
  {{ page.secondary_menu }}

  {{ breadcrumb }}

  {{ messages }}

  {{ page.help }}

  <main role="main">
    <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}

    <div class="layout-content">
........
      {{ page.content }}
    </div>{# /.layout-content #}

The number of regions we have in a template is not a part of this patch, its only role is to fix the hardcoded elements.
If theres a need for a discussion aboout regions that should be taken in the disucssion for the themes: seven, bartik & classy.
Else post is as a followup issues.

Yep, that makes sense. Let's add the region.

I don't really care so much how the markup in Bartik is/was. It is the way it is just because that's where things fell in line. If we move the breadcrumbs, we can fix Bartik's presentation. But, I'm in anti bikeshed mode, so add the region. It doesn't hurt anything.

Lets see what testbot says

This also affects performance; we currently build the breadcrumbs on every page, even when they're not used at all! This patch will solve that.

1. This should also update core/modules/system/templates/page.html.twig. I also suspect this is the main reason for the 56 fails: that template is used by Classy, and most tests use Classy; hence tests verifying breadcrumbs will fail, because it will simply never show up.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -460,6 +460,7 @@ public function rebuildThemeData() {
   +        'breadcrumb' => 'Breadcrumb',
   

   Should be plural?

I guess it shouldn't be plural if the name of the region is breadcrumb. I think it would be also confusing if the name of the block is breadcrumb and region would be breadcrumb. Thats why I chose to use singular. I fixed point 1.

One more fail, not one less :(

But I think I know why. All those current tests assume that the breadcrumb trail is just there, automatically. But now, you'll have to place a block to get it there.

I remember encountering similar problems in #1869476: Convert global menus (primary links, secondary links) into blocks. You'll need to do add some $this->drupalPlaceBlock() calls.

Lets see if it fixes some of the tests.. I ran some of them and they seemed to be green!

Progress! But many of them now have exceptions. You'll want to add the block module to the list of modules that these tests should install.

Lets see if some of the tests get fixed now :P

$modules is currently declared with 'comment'. I think you want to add to the array not replace it?

Ahh I missed it because normally its been placed top of setup..

+++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
@@ -110,6 +113,7 @@ function testPageCacheTags() {
diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php

diff --git a/core/modules/system/src/Tests/Theme/EngineTwigTest.php b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
index acfd1d1..ecd8d12 100644

index acfd1d1..ecd8d12 100644
--- a/core/modules/system/src/Tests/Theme/EngineTwigTest.php

--- a/core/modules/system/src/Tests/Theme/EngineTwigTest.php
+++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php

+++ b/core/modules/system/templates/page.html.twig
@@ -96,7 +94,7 @@
diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
index f657c4d..c309a76 100644

index f657c4d..c309a76 100644
--- a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

--- a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
+++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php

+++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
@@ -49,4 +49,13 @@ public function linkGeneratorRender() {
diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
index b754ffa..4a415cd 100644

index b754ffa..4a415cd 100644
--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -33,6 +33,10 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml
index 695645b..d2abf91 100644

index 695645b..d2abf91 100644
--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml

These hunks are from a different patch.

They also contain the single remaining test failure :) Remove those hunks, and this is green!

Removed!

here's interdiff also

Patch looks great. Great work, great progress :)

Still needed:

1. CSS fixes in Bartik (see attached screenshots); it looks fine in Seven
2. The label is 'breadcrumb' (lower case, bad) in the Seven block layout form, but 'Breadcrumb' in Bartik & Classy's block layout forms (good)
3. CR

And the screenshots I forgot.

Rerolled latest patch

Fixed #40.2, and the failing test.

Still pending the styling in Bartik and the Change Record. When fixing #40.1, we must take into account that the block title could be there (although I guess no-one would use it in this block, it should not look misplaced as default).

Uploading screenshot with title.

NW for #40.1 and #40.3.

Rerolled the patch

Should fix #40.1

Before:

After:

Did you upload the right screenshots? The "after" in #48 still doesn't look quite right to me.

I'll try to figure out what's happenin and fix the styles issue.

I tested the correct visualization (see the screenshot below) and fixed the PageCacheTagsIntegrationTest error.

Steps taken:

1. Applied the patch.
2. Ran a clean Drupal installation.
3. Verified that the breadcrumb block is there for Bartik, Classy and Seven.
4. Verified that there are no visual regressions in any theme.
5. Browsed around and the breadcrumb looks and works just like before.

As far as I can tell, this is RTBC, good work everyone!

The edge case of displaying the title of the block still doesn't look aligned in bartik, but it is on seven.

Compare screenshoots:

Create article using admin theme Seven:

Create article with Bartik:

Thinking again about this... As showing a title for the breadcrumbs is a nice new feature of converting breadcrumbs to a block and was not supported in D7, it shouldn't stop this patch if you don't want to, but please create a follow-up if we don't fix it here.

However, leaving this as Needs work because we still need a change record.

Thanks for working on this!

What I do with the label/title is set it inline with the list and make it 1em/bold. Could be an idea for core.

It really makes no sense to have a title for this block, perhaps we could just set the title not display by default for now?

It is already hidden by default. Is not *usual*, but I wouldn't say that it is a no sense ;-)

ow hehe ok -=]

Change record created: https://www.drupal.org/node/2410773

Back to needs review and go for RTBC. I'm looking forward to see this and watch breadcrumbs variable burn in hell!

changenotice looks good - may i suggest that @penyaskito creates the follow up to the breadcrumb block to take on the markup discussion there.

Follow-up created: https://www.drupal.org/node/2412951

Let's move this on!

Looks great. Removes a performance hit when you don't display breadcrumbs.

Committed/pushed to 8.0.x, thanks!

This change causes contrib tests using FieldUiTestTrait to fail.

Two things are needed I think:

- a separate change record to state that tests using FieldUiTestTrait need to be updated to enable the breadcrumb block
- documentation needs to be added to FieldUiTestTrait stating this too.

Eg, CommentNonNodeTest uses the trait, and does this:

  protected function setUp() {
    parent::setUp();
    $this->drupalPlaceBlock('system_breadcrumb_block');

Reclosing this and filing a follow-up: #2417517: removal of breadcrumb from theme causes FieldUiTestTrait to fail