Remove the div around block content - and make sure this doesn't break any CSS defined in core modules.

Current block code:

<div class="block {{ attributes.class }}"{{ attributes }}>
  {{ title_prefix }}
  {% if label %}
    <h2{{ title_attributes }}>{{ label }}</h2>
  {% endif %}
  {{ title_suffix }}

  <div class="content {{ content_attributes.class }}"{{ content_attributes }}>
   {{ content }}
  </div>
</div>

Proposed change:

<div class="block {{ attributes.class }}"{{ attributes }}>
  {{ title_prefix }}
  {% if label %}
    <h2{{ title_attributes }}>{{ label }}</h2>
  {% endif %}
  {{ title_suffix }}

  {{ content }}
</div>
Files: 
CommentFileSizeAuthor
#47 1972122-47.patch8.36 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,616 pass(es). View
#47 bartik-after.png59.89 KBakalata
#47 bartik-before.png59.89 KBakalata
#41 interdiff-1972122-32-41.txt9.64 KBakalata
#41 1972122-block-content-div-41.patch5.65 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,617 pass(es). View
#41 stark-html-after.png49.96 KBakalata
#41 stark-html-before.png61.58 KBakalata
#41 stark-after.png48.23 KBakalata
#41 stark-before.png48.23 KBakalata
#32 1972122-block-content-div-32.patch10.9 KBJamesLefrère
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,805 pass(es). View
#30 1972122-block-content-div-30.patch10.63 KBJamesLefrère
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,803 pass(es), 1 fail(s), and 0 exception(s). View
#25 interdiff-block-18-25.txt1.06 KBmortendk
#25 1972122-block-content-div-25.patch10.1 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,320 pass(es), 1 fail(s), and 0 exception(s). View
#22 1972122-block-content-div-18.patch8.88 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,903 pass(es). View
#17 drupal8.1972122-block-content-div-17.patch6.19 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,758 pass(es), 71 fail(s), and 15 exception(s). View
#16 drupal8.block-content-div-16.patch5.4 KBpakmanlh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,780 pass(es). View
#14 drupal8.block-content-div-14.patch4.49 KBpakmanlh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es), 1 fail(s), and 0 exception(s). View
#9 drupal8.block-content-div-9.patch3.26 KBckrina
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,718 pass(es), 4 fail(s), and 0 exception(s). View
#6 drupal8.block-content-div.6.patch3.28 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,836 pass(es), 4 fail(s), and 0 exception(s). View

Comments

webchick’s picture

Hm. Don't we need those attributes/classes for things like RDF / contextual links modules?

jenlampton’s picture

Issue tags: +markup

I don't think so, contextual links uses title prefix / title siffux, and RDF (i think?) should be adding attributes to the content, not the wrapper around the content.

Also, if RDF really needs a div here, it can add it later. One of our goals with the new theme system is to start from nothing. We shouldn't add anything "just in case" and certainly not in a place that will add an extra level of divs almost everywhere. less is more. add only when absolutely necessary.

Also, tagging.

chippper’s picture

We are working on modifying this in this dream markup issue: https://drupal.org/node/1982244

heddn’s picture

Status: Active » Closed (duplicate)
sun’s picture

Status: Closed (duplicate) » Needs review
FileSize
3.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,836 pass(es), 4 fail(s), and 0 exception(s). View

#1982244: Markup for: block module suffers from massive scope-creep.

Attached patch just performs the removal of the inner div.content wrapper, as agreed on over there.

Status: Needs review » Needs work

The last submitted patch, 6: drupal8.block-content-div.6.patch, failed testing.

ckrina’s picture

Assigned: Unassigned » ckrina
ckrina’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,718 pass(es), 4 fail(s), and 0 exception(s). View

Rerrolled #6 patch.

ckrina’s picture

Assigned: ckrina » Unassigned

Status: Needs review » Needs work

The last submitted patch, 9: drupal8.block-content-div-9.patch, failed testing.

jjcarrion’s picture

Assigned: Unassigned » jjcarrion
ckrina’s picture

Assigned: jjcarrion » ckrina
pakmanlh’s picture

Assigned: ckrina » Unassigned
Status: Needs work » Needs review
FileSize
4.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es), 1 fail(s), and 0 exception(s). View

Here the patch again modifying the markup value to pass the Render Block test, this way is quite tricky because twig add some extra and invisible «\n» characters.

Status: Needs review » Needs work

The last submitted patch, 14: drupal8.block-content-div-14.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,780 pass(es). View

Test fixed.

mortendk’s picture

FileSize
6.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,758 pass(es), 71 fail(s), and 15 exception(s). View

Added a block.html.twig file for bartik, so it still haves the .content wrapper class.
Bartik can then keep its css until somebody wanna clean that up.

Status: Needs review » Needs work

The last submitted patch, 17: drupal8.1972122-block-content-div-17.patch, failed testing.

sun’s picture

Not sure I understand — @mortendk, do we have to retain the .content wrapper for Bartik?

I.e., does #16 (which apparently passed tests) break Bartik's layout/design?

mortendk’s picture

@sun - jeps theres a log if .content references in bartik. else we have to rewrite the css, which offcourse can be done as well ;)
dunno why the test breaks btw ?

akalata’s picture

Assigned: Unassigned » akalata

Since #16 passes, I'm going to work from there to fix Bartik's CSS.

akalata’s picture

FileSize
8.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,903 pass(es). View

Issue found when removing the content_attributes div: the Search block form has an additional class that is added this way, container-inline. Would it be acceptable to put that class into the block wrapper? Does this become a Bartik task?

I've also found some unused CSS in Bartik (#block-user-login vs #block-bartik-login, #block-search-form vs #search-block-form) but I'm guessing those should be specifically created as Bartik tasks?

This patch add CSS fixes for sidebar and footer selectors that existed using .content.

akalata’s picture

Status: Needs work » Needs review
akalata’s picture

Assigned: akalata » Unassigned
mortendk’s picture

FileSize
10.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,320 pass(es), 1 fail(s), and 0 exception(s). View
1.06 KB

@akalata: the container-inline ive added to the outer wrapper in the patch here

afaik there isnt any known use for {{ attributes_content }} unless we want that for consistency reasons, then we cant do anything here :( - which i personally think is kinda stupid to have wrappers in the if if if if if situation that you need it, lets stop that.

I think we should have a "clean away all of bartiks crap" issue when we have corrected the blogs name scheme + id

Status: Needs review » Needs work

The last submitted patch, 25: 1972122-block-content-div-25.patch, failed testing.

LewisNyman’s picture

Issue tags: +frontend, +html
emma.maria’s picture

Issue tags: +Needs reroll
JamesLefrère’s picture

Assigned: Unassigned » JamesLefrère
JamesLefrère’s picture

Assigned: JamesLefrère » Unassigned
Status: Needs work » Needs review
FileSize
10.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,803 pass(es), 1 fail(s), and 0 exception(s). View

Right, I think this has rebased correctly... we'll see.

Status: Needs review » Needs work

The last submitted patch, 30: 1972122-block-content-div-30.patch, failed testing.

JamesLefrère’s picture

Status: Needs work » Needs review
FileSize
10.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,805 pass(es). View

Amended BlockPreprocessUnitTest to look for classes on the block itself rather than the missing content div.

JacobSanford’s picture

Issue tags: -Needs reroll

It doesn't look like this needs a reroll. Removing tag.

Jeff Burnz’s picture

I'll only comment on the Bartik stuff, either do this or what morten said previously and bash Bartik into shape later on and leave with the content wrapper and new template, mainly because all we are really doing here is juggling specificity around - trading one over specified selector for another, which is not really getting the root of Bartiks issues to be frank, the patch is probably better than what we have now, but its not a complete fix and really Bartik needs a big rewrite to do this properly (such as its rather mad font sizing). Either way, go for it, Bartik is going to get rewritten eventually so this will change later on any ways.

scor’s picture

This patch is fine from an RDF standpoint, since we don't rely on the content_attributes... only on the attributes (that wraps the entity output) and the title_attributes.

mortendk’s picture

shoudnt we use a data-attribute to test on instead of a css class?

yup were fixing bartik so it "works" one step at a time

joelpittet’s picture

Need a bit more interdiff between the patches, it's hard to tell who is adding what and the scope of this issue may be getting bigger than need be to get in. For how to create an interdiff: @see https://drupal.org/documentation/git/interdiff

The CSS changes are the only worry otherwise I'd like to RTBC.

We may not need the search container-inline moved to the block's container as now it is inline?

  1. +++ b/core/themes/bartik/css/style.css
    @@ -893,18 +886,21 @@ ul.links {
    +.sidebar .block .contextual-links {
    +  font-size: 1.010;
    +}
    

    This is a bit strange can whoever wrote this some context to why this is needed?

  2. +++ b/core/themes/bartik/css/style.css
    @@ -991,6 +982,18 @@ ul.links {
    +  color: #c0c0c0;
    +  color: rgba(255, 255, 255, 0.65);
    +  font-size: 0.857em;
    +}
    +#footer-wrapper .block .contextual .contextual-links a {
    +  color: #333333;
    +  font-size: 1em;
    +  line-height: 1;
    +}
    +#footer-columns .block > h2 {
    +  color: #3B3B3B;
    +  font-size: 1.167em;
     }
    

    Is this needed?

  3. +++ b/core/themes/bartik/css/style.css
    @@ -1023,26 +1023,26 @@ ul.links {
    -#footer-columns .content li a {
    +#footer-columns .contextual-links li {
    +  line-height: 1;
    +}
    

    Is this needed?

joelpittet’s picture

Issue tags: +Twig

@akalata What's your take about those styles you added I think in #22? Are they needed?

akalata’s picture

@joelpittet that CSS was added in so that the visual presentation on Bartik would be changed as little as possible -- unfortunately, Bartik's CSS at the time relied on the div that this patch is removing, and since Bartik is the default theme (and how I was testing the patch), I thought it made sense to include those changes here. If not, feel free to ignore and we can open a separate issue for the impact on Bartik.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +needs screenshots

@akalata Thanks for letting me know. I bet you likely got them working dead on. Most of them I can see the pattern of before and after and I understand the reason behind the contextual links because I've broken enough in my own themes to know they can be a pain.

These two things are my biggest concerns:

  1. +++ b/core/themes/bartik/css/style.css
    @@ -893,18 +886,21 @@ ul.links {
    +.sidebar .block .contextual-links {
    +  font-size: 1.010;
    +}
    

    This one is my biggest concern, it's a font-size without any units and dang is it hard to tell how big that is, any chance we are using rems in Drupal yet? Rems would save a bunch of pain.

  2. +++ b/core/themes/bartik/css/style.css
    @@ -991,6 +982,18 @@ ul.links {
    +  font-size: 1em;
    

    This shouldn't do anything as 1em = same as parent. I may be wrong here, it's hard to tell.

Also tagging with needs screenshots so we can see this change in action.

akalata’s picture

Status: Needs work » Needs review
FileSize
48.23 KB
48.23 KB
61.58 KB
49.96 KB
5.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,617 pass(es). View
9.64 KB

Rerolling #32, since it wasn't applying cleanly for me. I've also split the Bartik changes into its own issue: #2342005: Bartik theme style cleanup from removal of div in base block template.

joelpittet’s picture

Good idea @akalata! Makes good sense to remove this from stark from a foundation perspective and themes or their suggestions can add the wrappers that make sense.

joelpittet’s picture

@akalata with this approach(I'm torn on this myself). Do we need to extend block.html.twig in bartik to keep it from busting still as a stop gap?

akalata’s picture

@joelpittet that's the approach I took at #2342005: Bartik theme style cleanup from removal of div in base block template. Figured it would be an easier review than changing the CSS, since that's on the list to get an overhaul. Or did you want to move that adjustment back into this issue?

joelpittet’s picture

@akalata Oh sorry miss-understood the separatation. I was thinking that that template would end up in this patch, and that issue would deal with CSS changes and if possible removing that template too.

Granted it's a chicken and egg thing, but I think if we move the template and preprocess stuff we had in core to bartik, it then becomes bartik's repsonsibility to do further CSS cleanup and markup improvements.

And we can still keep the CSS changes out of this issue:)

+++ b/core/modules/search/search.module
@@ -115,7 +115,7 @@ function search_theme() {
-    $variables['content_attributes']['class'][] = 'container-inline';
+    $variables['attributes']['class'][] = 'container-inline';

I believe we may need to redo this preprocess change for bartik too btw.

joelpittet’s picture

We likely don't need to address the search inline container, but it would be good to get a screenshot of that though, just in case:)

akalata’s picture

FileSize
59.89 KB
59.89 KB
8.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,616 pass(es). View

Ah, I got you. Cool with following your lead, happy to have the feedback! :)

Here's a patch adding the block.html.twig to Bartik. I've added in the container-inline class via the preprocess (more for the practice), but the margin between the block title and input field isn't exact. Is it a big enough change worth playing with?

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs screenshots +dreammarkup

@akalata nope that's not a big deal, I don't think that can be considered a serious regression of any sorts. Let's get this in, and deal with trying to remove need for the extra template in that followup issue after this is in.

This cleans up/simplifies the base (less divs!). And for the cases where those divs are needed they can be provided by the theme's suggestions.

Thank you for your patience and patches!

Wim Leers’s picture

Manually tested, contextual links also still work (I just wanted to make sure).

RTBC +1

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 517c332 on 8.0.x
    Issue #1972122 by akalata, mortendk, JamesLefrère, pakmanlh, ckrina, sun...

Status: Fixed » Closed (fixed)

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