Field default markup - removing the divitis

Should multivalue fields be <li>s inside a <ul> container? They are a list of items, after all.

I don't know if <label> is the correct tag to use in this situation. I think semantically they're probably more suited to form elements. In the HTML5 spec, labels (4.10.4) are specifically a subsection of forms (4.10), however there doesn't seem to be any explicit prohibition on their use outside forms. I would however argue that a strict interpretation of the spec, would indicate the above use incorrect.

The <label> element should only be used to reference labelable elements. You can create custom labelable elements via JS, but the default elements are:

• button
• input
• keygen
• meter
• output
• progress
• select
• textarea

My preference would be for a div to wrap the multiple value fields, rather than a section tag. From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

Do not use the element as a generic container; this is what

is for, especially when the sectioning is only for styling purposes. A rule of thumb is that a section should logically appear in the outline of a document.

Specifically:

A rule of thumb is that a section should logically appear in the outline of a document.

I was about to write precisely what @baronmunchowsen wrote, I +1 that :)

As far as the semantics go, I love this SO much better than what's in D7.

I agree with the comments around use of the label tag. The solution in #10 is semantic and general enough for out of the box use. In some cases I would override the label in this template and use heading tags, but that's not often enough that we should try to account for it here.

I haven't been following a lot of the work yet and hoping to get more involved soon, but if this what's going into D8 so far, I'm very happy indeed.

On one hand I like the simplicity here. I'm a bit concerned that the markup pattern is going to vary based on how many items a field has.

For example let's say I have a node reference field to show related pages. If I reference two pages, I get a list. If there's just one, no list.

If I want to style the individual page reference, I could use .field-item when there is a list. But if there is only one item, then I need to target the parent div. But if I do that, then it may mess up the styles when there is a list.

I'm beginning to understand why all the nested divs now.

Maybe wrapping item in span class=field-item would help.

I think this is all about providing sensible defaults, and making them super-easy to change. Seems like #10 does exactly that. A list for a list of field items in a multiple value field makes much more sense than a bunch of divs, but considering all the different ways a field can be used, using a section and/or a heading tag has the potential of breaking document outlines in all sorts of interesting ways.

mdrummond, valid point. My gut reaction would be to add the field-item class to the wrapper div in case of there being only one item in a list, but if there are more items, add it to individual list items. Feel free to poke holes in that idea.

+1 to field-item on wrapper for one item

That would require CSS to have two selectors:

.field-name .field-item,
.field-name.field-item {
}

Could we have a .field-name-item class?

Here's how we can solve this.

In field.module, within template_preprocess_field, we add this:

$variables['multiple'] = $element['#cardinality_multiple'];

That checks to see if a field is capable of having multiple field items or not.

Then we can change the template like so:

<div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}" {{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% if multiple %}
    {% for delta, item in items %}
      <ul class="field-items" {{ content_attributes }}>
        <li class="field-item">{{ item }}</li>
      </ul>
    {% endfor %}
  {% else %}
    {# print only the data #}
    {{ item }}
  {% endif %}  
</div>

With that, if a field can have multiple items, it's in a list, regardless of how many items are in that field. That's nice and consistent.

If you want to use CSS to style every .field-item, regardless of which field it is a part of, you can do so. If you want to target a .field-item for a particular field, you should know if it allows multiple values or not, and then use .field-name .field-item or .field-name.field-item as necessary.

Seems fairly clean and simple. Thoughts?

I find @mdrummond solution acceptable.

First of all:
The <ul> is only necesary once: (Should be outside the loop:)

<div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}" {{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>´
    {% for delta, item in items %}
        <li class="field-item">{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {# print only the data #}
    {{ item }}
  {% endif %}
</div>

And why do we need the outer <div>?
I always delete it … never need it.

Oops, sorry on putting the ul tags inside the for loop.

If the field allows multiple items, and it's outputting a list, and there's no label, then yes, any field classes could probably go on the ul.

If there is a label, I could see there being a use for having a wrapper around the label and the list, so that you can target the bundle.

If it is a single item, with no list involved, I'd think in most cases you'd want some sort of wrapper around the field data. Otherwise it's just going to clump up next to whatever is before or after it.

In most cases, if there's a single element, I'd probably have something like:

<p>{{ item }}</p>

If there was a label, I'd probably have:

<p><strong>{{ label }}:</strong> {{ item }}</p>

But this is supposed to be flexible, so I'm fine if we're using divs instead of p and strong.

I'd really only want that mess of classes unless I had a real reason to target those individual fields, otherwise I'd prefer removing all of them.

If I can do that easily with template files, ok.

I've write the patch following the idea of mdrummond, I've added a variable for multiples fileds to use it later in twig template.

I've tested it for text and image values (I've attached two screenshots) and works good.

reroll because the template_preprocess_field function has been moved to /core/includes/theme.inc

Let's make testbot work a little :P

Do we really need   here? Can we get rid of it? Ideas?

+++ b/core/modules/system/templates/field.html.twig
@@ -31,11 +31,16 @@
+    <h3 class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</h3>

I’ve been trying to follow the Drupal 8 CSS architecture guidelines for the past 8 months or so and have ended up having "field" be a SMACSS component. Thus my markup looks like this:

<div class="field field--name/type/etc">
  <div class="field__label"></div>
  <ul class="field__items">
    <li class="field__item"></li>
    <li class="field__item"></li>
  </ul>
</div>

I’m not a fan of using <h3> for the label since it’s highly dependent on the heading structure of the page.

I really like the idea of removing the items and item wrappers for non-multivalue fields. Also, since we’re using <ul> for the field items, we could probably get away with removing the field item class.

Here is the new patch and the interdiff changing the h3 to a div.

Taking a look at fixing the failing tests.

Right, the tests are ok, I think - the fails were down to an extra space before the </li> in the patch:

-      <div class="field-item"{{ item_attributes[delta] }}>{{ item }}</div>
+        <li class="field-item"{{ item_attributes[delta] }}>{{ item }} </li>

The tests look for text enclosed between > and <, eg $this->assertRaw('>' . $newterm1 . '<', 'First new term displayed.');

I've removed the extra space and run some of the failing tests locally and they pass ok.

Lets see what the testbot thinks.

Some of the other tests still fail locally, so I'll investigate further.

Fixed the failing Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() tests by changing the xpath to match the new markup.

-      $elements = $this->xpath('//div[text()="' . $output . '"]');
+      $elements = $this->xpath('//div/div/div/following-sibling::text()[contains(.,"' . $output . '")]');

Moving onto the next failing tests.

Started to look at the failing RDF tests and the patch actually removes some of the RDF markup. Looking at the Drupal\rdf\Tests\CommentAttributesTest->_testBasicCommentRdfaMarkup() test, the original markup was:

<div class="content">
  <span rel="sioc:reply_of" resource="/node/1" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <div class="field-items">
      <div class="field-item" property="content:encoded"><p>vT3AygB4</p>
      </div>
    </div>
  </div>
</div>

and the markup after the patch is:

<div class="content">
  <span rel="sioc:reply_of" resource="/node/1" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <p>o76HBbbM</p>
  </div>
</div>

so we lose the property="content:encoded", causing the test to fail.

With the patched field.html.twig template, we're losing the content_attributes and item_attributes information when a field doesn't have multiple set.

  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>
    {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {{ items }}
  {% endif %}

This is causing some of the RDF tests to fail, see #40 above. How can we include the attributes for non-multiple fields without bloating the markup? If someone can point me in the right direction, I'm happy to rework the tests to suit.

Updated the patch of the work so far from #40 to tidy up the xpath statement with a placeholder.

-      $elements = $this->xpath('//div[text()="' . $output . '"]');
+      $elements = $this->xpath('//div/div/div/following-sibling::text()[contains(.,:output)]', array(':output' => $output));

After discussing this with @ jjcarrion, I tried reintroducing the div around fields that don't have multiple set.

The field.html.twig template now looks like

  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>
    {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {% for delta, item in items %}
        <div class="field-item"{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>
    {% endfor %}
  {% endif %}

and with this change, all the failed tests pass ok locally.

Running the patch through the testbot to see if it works.

I wonder if it's simpler to just do this:

{% if multiple %}
  <div class="{{ attributes.class }}"{{ attributes }}>
     {% if not label_hidden %}
       <div class="field-label"{{ title_attributes }}>{{ label }}</div>
     {% endif %}
     <ul class="field-items" {{ content_attributes }}>
     {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
      {% endfor %}
      </ul>
  </div>
  {% else %}
  {% for delta, item in items %}
    <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>
     {% if not label_hidden %}
       <div class="field-label"{{ title_attributes }}>{{ label }}</div>
     {% endif %}
     {{ item }}
    </div>
  {% endfor %}
{% endif %}

An alternative would be:

  <div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}"{{ attributes }}{% if multiple == false %}{{ content_attributes }}{% for delta, item in items %}{{ item_attributes[delta] }}{% endfor %}{% endif %}>
    {% if not label_hidden %}
      <div class="field-label"{{ title_attributes }}>{{ label }}</div>
    {% endif %}
    {% if multiple %}
    <ul class="field-items" {{ content_attributes }}>
      {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
      {% endfor %}
    </ul>
    {% else %}
      {{ item }}
    {% endif %}
  </div>

The latter has two additional if multiple statements, but is a bit more DRY, as the field-label if block only appears once.

This would allow us to have just one div if there's only one item... and a whole mess of classes and attributes on the wrapper div.

Doh, the Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() tests failed now that the markup had changed again. Still, 2 fails is better than 24 :-)

Here's the patch without the OptionsFieldUITest xpath change together with the first option from #47.

@galooph the RDF tests should pass as long as you keep "item_attributes[delta]" in the HTML element wrapping the field value, whether it's a multiple value or single value. The actual HTML element doesn't matter for RDFa, it can be div, span, ul or li, the only thing that matters in RDFa is the attributes.

Thanks @scor, that's what we figured. The changes suggested in #47 add the item_attributes back in.

Checking out the test fails from the patch in #49.

The Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() test can be fixed with the xpath tweak, as before.

The Drupal\rdf\Tests\CommentAttributesTest->_testBasicCommentRdfaMarkup() test seems to fail due to some extra whitespace

The test sets up an expected value array:

    // Comment body.
    $expected_value = array(
      'type' => 'literal',
      'value' => $comment->comment_body->value . "\n",
      'lang' => 'en',
    );

but checking the output of debug($graph) in the test, we're actually getting:

        array (
          'type' => 'literal',
          'value' => '
          cB74xflH

    ',
          'lang' => 'en',
        ),

Applying #49 results in double printed field label, i assume this is not meant to work like this:

Which version should stay? I am not a big fan of having ": " after every field label by default.

The change in #49 fixes the Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() test so i added it. I also found that the field label was printed twice in field.html.twig, fixed that. Unfortunately it still fails some RDFa and other tests even after applying the final patch from https://drupal.org/node/2226233. Still needs work but i am not sure i can fix the tests, making it unassigned.

Some notes to have a look at from #7. Thanks for the patch @aboros!

1. +++ b/core/includes/theme.inc
   @@ -2418,6 +2418,12 @@ function template_preprocess_field(&$variables, $hook) {
   +  // We have to know if the field has multiple values to create a simple value
   +  // or a list.
   +  if (count($variables['items']) > 1 ) {
   +    $variables['multiple'] = TRUE;
   +  }
   +
   
   +++ b/core/modules/system/templates/field.html.twig
   @@ -30,12 +30,27 @@
   +{% if multiple %}
   

   This can be done in the twig template with items.length > 1.

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -30,12 +30,27 @@
   +     <ul class="field-items" {{ content_attributes }}>
   

   Remove the space before the print tags {{}}. Each attribute prints a space before it automagically.

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -30,12 +30,27 @@
   +  {% else %}
   

   This else should be pulled back a tab.

4. +++ b/core/modules/system/templates/field.html.twig
   @@ -30,12 +30,27 @@
   +    <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>
   

   This is asking for duplicate attributes, you'd likely want to merge them before hand in the preprocess onto something like item.attributes and then just have item.content/item.value? That's just my preference because I'd rather not use [delta]

Picking this back up to investigate the failing RDF tests.

I've finally got my head around the EasyRdf library that's used in the RDF tests and I've got the 'RDFa markup of comments' test group to pass now.

I'll carry on with the next failing RDF test and post a patch later on.

After lots of advice from @scor, we realised that the RDF tests that were failing were down to the new markup structure. @mdrummond came up with a revised twig template that maintained the RDF validation.

I've run all the failing tests locally and they all pass. The only test I've modified in the end is the OptionsFieldUITest, and then only to use a placeholder.

Changing the status back to Needs Review to trigger the testbot.

And the tests are passing! The only requirement for RDF is to have the {{ item_attributes[delta] }} in the direct wrapping HTML element of the field item output, so yeah, this works:

<whatever ...{{ item_attributes[delta] }}>{{ item }}</whatever>

Thanks for fixing #58.2 @galooph could you have a look at addressing the other issues brought up in #58?

Also mind posting an interdiff with the changes? Here's a git workflow I use that may help you: http://pittet.ca/drupal/sprint/patch

Otherwise the official docs are at https://drupal.org/documentation/git/interdiff

An example of what I'm talking about in #58.4
@see #1939062-120: Convert theme_item_list() to Twig

Let me raise a potential pitfalls for themers with the markup outlined in the OP. Imagine you have set a field allowing multiple values. You create a new node so you can start theming. You create 3 values for that field so you have a good scenario to theme against. Your CSS could look like this:

field-items field-item.li {color: red}

It will work, until a user submits a node with only one value for that field, in which case the CSS path will be different: field-items will be missing, and there will be no li. One could argue that the CSS could be designed for both use cases, but come on, that's no intuitive and people will forget. Maybe if the logic to switch was based on the field settings, and not the actual number of items being rendered, this problem would go away.

Completely agree with @scor. Field settings would be a better way to determine the element changes but with element changes on |length would be unintuitive. Though for anybody who would like to do that in their own site, it would be super trivial, by overwriting this template with {% if items|length > 1 %} ul list {% else %} div {% endif %}

I think there might be some potential issues with this structure, but I don't think that's a particularly good example.

If we're going to go through all of the effort to have a billion classes in pursuit of SMACSS nirvana, then it's not unreasonable to expect people to use those classes.

If what you want to do is to make each field item red, great, do this:

.field-item {
  color: red;
}

If you want each field item to be red whether it is in a list or not, then great, don't make your selector dependent on the existence of .field-items.

If you want to handle the padding around the list as a whole, then using the .field-items is a good and necessary choice.

It's possible the .field-label could be an issue, as it is outside .field-items when there are multiple field items, but inside .field-item if there is just one.

But again, if you want to change the styles of .field-label, then just target that class as a selector. And if you don't want .field-item to pollute the styles of .field-label, then don't use generic descendant selectors with .field-item.

Sorry to get all ranty, but I've never been thrilled with the divitis and classitis inherent in Drupal. That said, classitis is a potential cure for divitis if we take advantage of OOCSS principles. That at least lets us kill a good amount of the divitis. And then if people want to kill off the abundance of classes, particularly if they're developing their CSS with Sass, then more power to them.

Here's a revised patch addressing #58.1 and #58.3

I'm not too sure about #58.4, @joelpittet, but happy to take a look if you can give me a few more pointers.

I'm really concerned about the problem described in #65.

The idea that the actual structure of the markup could be one way today, and another way tomorrow depending on whether there are one or two values available is a problem. It's going to cause shocks and swearing at the system - by the themers - during testing.

The themers I work with look at the markup being produced, figure out a pattern and then build CSS to match it.
They can built a perfectly working result based on perfectly working site markup. This patch makes it possible and likely that that theme will blow up in the clients face as soon as they enter only one value, because all the mockups had more than one.

During the birth of CCK, at a technical level, all our node fields were sometimes singles, sometimes multiples. The attributes attached to the $node were sometimes strings, and sometimes arrays. You could never be sure what to expect, and all field handler code was layered with is_array() checks to figure out what to do with the input.
And then this was fixed. Multiple field values became supported properly as the only way to do it, each single value became an array of 1, but always an array, and vast amounts of logic were simplified.
Yes, it's a little annoying to have to go $node->fieldname[0]['value'] instead of just $node->fieldname['value'] but it's 100% consistent and doesn't switch around based on what the content is doing today.
There is now much less if-then-but work done in managing field rendering.

What this patch does is bring back the inconsistency and unpredictability that was seen in the early-cck days, and places new if-then work into the CSS.

I predict pain. Pain at the themers end, as this makes their life less predictable.

( sorry I'm late, and I see this was already brought up earlier. I only noticed this when RDF was brought up (good work there!). I'm just thinking of the new training we have to do down the road, and the landmines this lays. )

1. +++ b/core/modules/system/templates/field.html.twig
   @@ -43,12 +43,12 @@
   +      <div class="field-item{{ attributes.class }}"{{ attributes }}>
   

   +++ b/core/modules/system/templates/field.html.twig
   @@ -43,12 +43,12 @@
   +      <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>
   

   Ok so here's the thing, the attribute's "values" aren't smart like the name/value attribute pairs when it comes to putting spaces in-front. So you don't want to do this for the class pull outs. Though if you'd like to avoid the possibility of a space in the attribute at the end you can do this trick {{ attributes.class|merge(['field-items']) }}

I'm still on the side of scor's point in #65 and echo what @dman said in #71.
Though nice work on the demo page @mortendk: http://mortendk.github.io/drupal8-fields/

Re: #70 this line in the preprocess:

  foreach ($variables['items'] as $delta => $item) {
    $variables['item_attributes'][$delta] = !empty($element['#items'][$delta]->_attributes) ? new Attribute($element['#items'][$delta]->_attributes) : clone($default_attributes);
  }

Can become:

  foreach ($variables['items'] as $delta => $item) {
    $variables['items'][$delta]['value'] = $item;
    $variables['items'][$delta]['attributes'] = !empty($element['#items'][$delta]->_attributes) ? new Attribute($element['#items'][$delta]->_attributes) : clone($default_attributes);
  }

Then you can remove the delta and array access in Twig.

      {% for item in items %}
        <li class="field-item {{ item.attributes.class }}"{{ item.attributes }}>{{ item.value }}</li>
      {% endfor %}

I could deal with switching markup if it was business rule - field cardinality is 1.
But that's not what is being checked here - it's switching due to there being just one of possibly many today.

Thanks for the help, @joelpittet - I think I've got the changes from #72 applied correctly now, but please double check!

I tried the twig merge you mentioned, but that threw an error - "Twig_Error_Runtime: The merge filter only works with arrays or hashes in "core/themes/bartik/templates/node.html.twig" at line 92. in twig_array_merge() (line 673 of core/vendor/twig/twig/lib/Twig/Extension/Core.php)."

Consistency is good. It just gets difficult to balance that with markup that ends up looking somewhat ridiculous, purely for the sake of consistency.

We do have a few different scenarios that are possible here.

We could have multiple field items, and a label for those field items. In that case, it's entirely logical that we'd have a structure that looks like this:

<div class="field-wrapper {{ attributes.class }}"{{ attributes }}>
  <div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:&nbsp;</div>
  <ul class="field-items {{ content_attributes.class }}" {{ content_attributes }}>
  {% for delta, item in items %}
    <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
  {% endfor %}
  </ul>
</div>

That's a very sensible HTML structure. There are a couple divs, but they make sense.

If there's just one item in a field, and there is a label, then it feels silly to have field-item wrapped in field-items.

There is a difference, however, between there being one item in a field that always has one item in a field, and one item in a field that sometimes has one item in a field.

I think it's a point well taken that maybe what we should be testing is if the field is capable of having multiple items rather than that plus if a field actually has multiple items. While it's a bit of extra HTML, it would be more consistent to keep the ul.field-items for fields that can have multiple items, even when there is only one.

The other scenario is a field that only ever has one item. Sensible markup for that would be:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
  <div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:&nbsp;</div>
  <div class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
</div>
{% endfor %}

If it's possible, it might be nice to merge the attributes and content_attributes in pre-process.

Anyhow, putting a field-wrapper around field-item makes sense here. What doesn't make sense would be to include field-items around field-item, particularly if this is only for fields that can only ever have one item.

The last scenario is that we have a field item, but it doesn't have a label. That's where it gets tricky. The consistent thing to do would be:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
  <div class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
</div>
{% endfor %}

What feels frustrating here is that the field-wrapper seems extraneous and needless cruft. It's consistent cruft, but cruft all the same. You'd get something similar for field-items with no label.

<div class="field-wrapper {{ attributes.class }}"{{ attributes }}>
  <ul class="field-items {{ content_attributes.class }}"{{ content_attributes }}>
  {% for delta, item in items %}
    <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
  {% endfor %}
  </ul>
</div>

Again, the field-wrapper seems like it could probably be dropped. But in both scenarios, if you do so, then you have a different markup pattern based on whether or not a label is present.

In a world with no labels, it would be nice to see that markup as:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }} field-item {{ item.attributes.class }}"{{ attributes }}{{ content_attributes }} {{ item.attributes }}>
  {{ item.value }}
<div>
{% endfor %}

And:

<ul class="field-wrapper {{ attributes.class }} field-items {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
{% for delta, item in items %}
  <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
{% endfor %}
</ul>

The real question here is how often is there going to be a scenario where somebody is going to want to write a selector based on .field-wrapper .field-item. That is the scenario that would fail if we dropped a .field-wrapper div when there is no .field-label.

I like seeing nice and tidy HTML, but I also don't want to be yelling WTF when writing CSS. Not always easy to balance out those two.

Thoughts? Suggestions?

Sorry, galooph, apparently I was commenting at the same time you were posting your patch, and somehow my comment deleted your patch? It seems really silly that d.o is set up to work that way. Anyhow, my apologies.

No problem, @mdrummond!

Here's my patch and interdiff from #74 again, as I think we confused the testbot.

I guess that now the tests are passing, we need to agree on the precise markup before tweaking the template again.

Well, the testbot really doesn't like #78 - 576 fails! Mainly '"attributes" is an invalid render array key'.

Here's the patch from #70 but with the extra space replaced. field-item{{ attributes.class }} becomes field-item {{ attributes.class }}.

I'll just leave this link right here so everyone can enjoy:

http://www.heydonworks.com/article/confessions-of-a-css-expert

For the multiple with no label, there is an extra closing div tag.

For the single with label, I don't think WTF belongs in the template.

The multiples are missing the attributes variable.

I miss having a list for multiples, but this is more consistent. If I want a list, I can easily customize the template to make that happen.

So essentially we have a different wrapper class based on whether a field is single or multiple. Interested in hearing from some of the people that had objections before if that is helpful.

For the single value field, field-content is nested in field-single if there is a field-label, while there is no nesting if there is no label. That is nice and simple: interested in hearing from those with consistency objections if this would be a concern or not.

I've been thinking that what might be a helpful exercise is looking through the existing CSS to see how field markup is targeted. That might provide some examples that would clarify what structure would provide the most consistent base as styling hooks.

reroll against 8.x HEAD of patch in comment #83

@galooph re: #74 Thanks for trying that out, I was sure I had merge working somewhere before on attributes but I guess not because of that check in twig_array_merge() for is_array() and Attribute is an ArrayObject so those checks don't pass. May have to look into some solutions for that elsewhere. Also, for the removal of [delta], I'm going to move that over to a new follow-up so as to not distract from issue summary goals.

That will live over here: #2229435: Clean up the way attributes are printed in field.html.twig

Carry on:) Nothing to see here.

There is a reroll of #83 in #87, but bot was not triggered.

@mortendk the way I read your proposal at http://mortendk.github.io/drupal8-fields is that no matter what patterns is rendered, we can be sure that each field data item is wrapped by a div with the "field-item" class, like this:

<div class="field-item">data</div>

If that's the case, then I'm 100% on board.

@mortendk I believe that simplifies the way it works and makes easier to understand the markup or even theme the different patterns.

As scor said, the fact that we know that every data in a field has only one div and we know the class... I am so in :)

While the resulting html might look good, I feel like the template becomes a little overcomplicated with all those if, elseif and loops.
In my experience each field really wants to be treated differently and in complex cases you will end up using a template for each field if not printing the fields in the node template and using wrappers there. What I'm saying is that there are so many use cases that it might be challenging to find a solution for most of them.
In this sense, I think we might be better served by a super simple template:

  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% for delta, item in items %}
      <div class="field-item{{ content_attributes }}">{{ item }}</div>
  {% endfor %}

@dodorama et al What about doing something along the lines of what fences module does, or maybe that's overkill but I know in d7 I include and turn that on every site. @see https://drupal.org/project/fences

Also would be nice to get input from @JohnAlbin though I'm still cool with this plan or even just splitting out multiple into it's own template as @Cottser mentioned in the twig hangout.

What I briefly mentioned on the Twig call last night was that I wonder if a BEM syntax for class names could help resolve the concerns people have about consistency in markup.

Rather than worry about chaining together multiple selectors and using descendant selectors, we could use class names that allow people to target each element accurately no matter the markup structure.

Base component classes:

• field__wrapper
• field--single__wrapper
• field--multiple__wrapper

Each of these base component classes can be used for styles regarding how this field or group of fields relate to the surrounding fields or content. These would be good classes to use to handle margin around a field for example.

As an example, it might be worth putting both field__wrapper and field--single__wrapper classes on an element: that gives somebody the choice of whether they want to have the same styles for both single and multiple value fields or if they want to vary those styles based on whether the field can handle multiple values or not.

Label element classes:

• field__label
• field--single__label
• field--multiple__label

This class would have perhaps the most to gain from this class scoping. With this class, it would be simple to float a field label to the left or display above a list as appropriate.

Again a more generic field__label class allows you to handle styles that affect both styles of fields.

item element classes:

• field__item
• field--single__item
• field--multiple__item

These classes allow you to style how the actual data in a field appears. You can apply the same styles for all types of field items, or you can apply specific styles for items in a multiple field context: for example, margins between each field item.

items element class:

• field--items

There's no need for a the single/multiple variations here, because a multiple value field by its nature is the only one that could have items. This element serves as a wrapper for the items in a multiple context: probably the primary usage is for controlling the relationship between the grouping of field items and any accompanying label.

Other classes

• field-type--bar
• field--label-above
• field--label-inline

I think field--label-above is a more accurate way to describe these modifiers. This is a field where the label is above. The variation seems to be label-above and label-inline. Similarly bar is a variation of field-type, not a sub element of the field-type.

Personally I would recommend avoiding the class names field--label-above and field--label-inline. Those are presentational classes, so if you want to change your label style, you need to change not only your CSS, but also your HTML.

Example: single field item, no label

<div class="field__wrapper field--single__wrapper field__item field--single__item field-type--bar">single field data </div>

Example: single field item, with label inline

<div class="field__wrapper field--single__wrapper field-type--bar">
  <div class="field__label field--single__label">single field label inline</div>
  <div class="field__item field--single__item">single field data</div>
</div>

Example: multiple field item, no label

<div class="field__wrapper field--multiple__wrapper field-type--bar">
  <div class="field__items">
    <div class="field__item field--multiple__item">data one </div>
    <div class="field__item field--multiple__item">data two</div>
  </div>
</div>

Example: multiple field item, with label

<div class="field__wrapper field--multiple__wrapper field-type--bar">
  <div class="field__label field--multiple__wrapper">label above</div>
  <div class="field__items">
    <div class="field__item field--multiple__item">data one </div>
    <div class="field__item field--multiple__item">data two</div>
  </div>
</div>

Summary
There are a few extra classes with this method, but now you never need to guess when you're writing your CSS. You don't need to know the exact markup structure for a particular field or provide variations for all the markup structure possibilities. You have classes that let you write consistent CSS, applying specific purposes to each class.

This allows us to neatly solve the divitis, provide RDF as necessary and allow for consistent styling of fields. There are a few extra classes, but in my opinion, it's worth it.

Thoughts?

Talked this over with mortendk, and we're in agreement now on how to have consistent styling with a minimum number of classes while still keeping the number of divs to a minimum.

Here's our primary cast of classes:

• field__wrapper
• field--single
• field--multiple
• field__items
• field__item

Supporting players include:

• field-type--$type{}
• field-name--$name{}
• field-label--$position{}: field-label--above, field-label--inline, field-label--hidden

As our story open, there will always be at least one div. On that outermost div, you will usually want these classes:

• field__wrapper
• field--single or field--multiple
• field-type--$type{}
• field-name--$name{}

Our hero strides on stage, ready to take on all opponents. If there is only ever one item for a field, and it despises any labels that try to define it, then add field__item to that one sole wrapping div. That's it: success!

<div class="field__wrapper field--single field-type--hero field-name--steve field__item field-label--hidden">Steve, the heroic field item!</div>

Great, you have a nice simple field that you can consistently style. Use field__item to style the data and field__wrapper to style the relationship of this field with other fields.

What if our heroic field gets a label?

<div class="field__wrapper field--single field-type--hero field-name--steve field-label--inline">
  <div class="field__label">Steve</div>
  <div class="field__item">The heroic field item!</div>
</div>

The field__label will always be inside of a div with either field--single or field--multiple, so there is no need to provide variations within the class name itself.

Also, we're putting the label type on the wrapper div, because of the UI on the field display page, which controls this. There you can choose to display the label inline, above or to hide the label. Since a hidden label class would need to go on the wrapper div (because the label div doesn't exist), we put the other labels there too.

What if our trusty field item, Steve, gets some friends to join him, but they don't have a label yet?

<div class="field__wrapper field--multiple field-type--hero field-name--steve field-label--hidden">
  <div class="field__items">
    <div class="field__item">Steve</div>
    <div class="field__item">Tony</div>
    <div class="field__item">Bruce</div>
    <div class="field__item">Thor</div>
  </div>
</div>

In this case field__item will always appear inside field__items if there is even the possibility of more than one field item showing up to do battle. So there is no need for a variation based on field--multiple.

Finally, if our field gets a label:

<div class="field__wrapper field--multiple field-type--hero field-name--steve field-label--above">
  <div class="field__label">Avenge this!</div>
  <div class="field__items">
    <div class="field__item">Steve</div>
    <div class="field__item">Tony</div>
    <div class="field__item">Bruce</div>
    <div class="field__item">Thor</div>
  </div>
</div>

Again, the field__label will always appear within a wrapping div that tells it if this is a multiple field and the position of the field label. So no need to vary the field__label class.

The only possible tricky thing is that if you want to vary the styling of a field__item based on the field type, you would need to two selectors:

.field-type--hero .field__item,
.field-type--hero.field__item {
  font-weight: bold;
}

I don't think that is the end of the world. It may cause a little extra CSS, but it saves us HTML on every single page of the site.

I think this is a pretty good solution that offers consistency and conciseness. But I've been wrong about that before, so have at it!

Morten pointed out we could probably simplify this even more. For field items with no label we could have:

<div class="field__wrapper field--multiple field__items field-type--hero field-name--steve field-label--hidden">
  <div class="field__item">Steve</div>
  <div class="field__item">Tony</div>
  <div class="field__item">Bruce</div>
  <div class="field__item">Thor</div>
</div>

This is still consistent. field__item is still always inside field__items.

1. +++ b/core/includes/theme.inc
   @@ -2414,19 +2416,22 @@ function template_preprocess_field(&$variables, $hook) {
   +  if ( isset($element['#items']) && is_object($element['#items']) ){
   +    if( $element['#items']->getFieldDefinition()->getCardinality() != "1"){
   +      $variables['multiple'] = TRUE;
   +    }
   +  }
   

   All that code should be $element['#items']->getFieldDefinition()->isMultiple(); Maybe though this should be already part of core/lib/Drupal/Core/Field/FormatterBase.php::view

2. +++ b/core/modules/system/css/system.theme.css
   index a0d527a..d4a00c8 100644
   --- a/core/modules/system/templates/field.html.twig
   
   --- a/core/modules/system/templates/field.html.twig
   +++ b/core/modules/system/templates/field.html.twig
   

   WTF! A simple template got way more complex compared to before.

Yes, the template is slightly more complicated, but it's pretty straightforward in how it sets up the four primary variations on field markup: multiple and singular, and label or no label.

What's more important is that it keeps the actual HTML output much more sane, with sensible class names that can be consistently targeted with CSS no matter which variation of the field markup is used.

First of all, thanks a lot for kicking this, folks ! Field output is a hairy issue that always lacked the insight of real theme-oriented people, so yay !

I think I like where this is going (HTML-wise you guys should know better anyway), and the recent iterations seem to make good mood to address the concerns I had with earlier iterations (predictability on multiple values, subtle and not too telling differences between ".foo .bar" and ".foo.bar" in CSS selectors...). Double yay !

Regarding field.tpl's "increased complexity", I'm just wondering whether it should be split into separate tpls (two ? four ?), resolved at runtime (in preprocess ? in the code that sets #theme on the field render array ?).
There will still be use cases for overriding the tpl (e.g to output ul/lis, or comma-separated values), so what would be easiest for a themer to override ? One single tpl with lots of logic, or various smaller flavours ?

This is looking really good (except the failed test ;). The template seems easy to read and understand despite multple conditions.

One thing that I would change for sure is css-class set name to css-classes or css_classes. The set contains multiple classes so it should be named accordingly just like title_attibutes, item_attributes, content_attributes and attributes.

I've got to agree with @yched in #116 - having a single template with a huge overarching if/else doesn't sit right at all.

Yes, template suggestions are difficult for people to wrap their heads around but they are a very useful tool that themers should know how to use. Even if we did have a template suggestion for each, the 90% use cases (overriding templates and using template suggestions) would be pretty much the same.

I would prefer one file over one for multiple and one for single.

I myself don't have a strong opinion on the question of multiple tpl files vs one, I was just raising the question on what's easiest to override :-)

In the "separate tpls" scenario :
- I was rather thinking 2 tpls (field-single.tpl, field-multiple.tpl), each one with "if" logic for label display,
rather than 4 tpls for the combinations of single / multiple & label / no label
- Since the template would be determined by a property of the field definition ("does the field potentially accept multiple values ?"), they don't add up in template suggestions: a field is either single or multiple, so field-single.tpl & field-multiple.tpl are not eligible for the same field at the same time, and the number of actual "suggestions" (based on field name, field type...) is the same as it is today.

Just sayin'. I can totally live with "nah, one file that is basically two separate if / else branches is going to be simpler for overrides in the end".

I'd really rather have one template rather than two (much less four).

I have definitely run into situations where I have had a field that starts as accepting only one value, but after a while, I realize it needs multiple or unlimited values. Maybe bad planning on my part, but it happens.

In that case, if I had a custom template for that field, and we had separate templates for single and multiple fields, then the template stops working. I have to remember to create a new template then or wonder why my field no longer has the customizations that it previously (if I notice it all).

A few if-then statements with logically named variables are not that hard to understand. Certainly much easier than trying to sort out which template file to use and how to deal with that if the field cardinality changes.

I agree with one template, may look more complicated, but you read it a couple of times, with the documentation and you are golden!!

No extra divs, no extra suggestions.. life will be simpler.. I rather have to take some hard minutes understanding the template once! and not trying to find the right template or the right hook suggestion to use every time I have to theme.

Fine with 1 tpl then :-)

+++ b/core/modules/system/templates/field.html.twig
@@ -29,13 +29,78 @@
+{% set cssclass %}
+field field-name--{{ field_name }} field-type--{{ field_type }} field-label--{{ field_label }} {{ attributes.class }}
+    'field-' . $variables['entity_type_css'] . '--' . $variables['field_name_css'],

Nitpick :
- we tend to avoid smushing words in var names.
- the name might as well reflect that its a list of classes rather than a single class ?
- not even sure the 'css_' prefix is needed, we usually simply talk about "a class" / "classes"

Long story short: s/cssclass/classes/ ?

Morten asked me to look over the documentation.

1. +++ b/core/includes/theme.inc
   @@ -2439,24 +2441,15 @@ function template_preprocess_field(&$variables, $hook) {
   +  //we want to know if the field is single or multiple
   

   This is missing a space and a capital. I think this might be more clear just saying "Are there multiple field items"

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -29,13 +29,80 @@
   +We want to make it easy for the themer to remove or add css classes easy.
   +Instead of hiding these as a string in the preprocess, they are done inside of
   +the tempate file.
   

   Remove, this is useful information during implementation, but not when people are actually using the file.

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -29,13 +29,80 @@
   +{{ attributes.class }} is providing a placeholder for mddules that wants the
   +option to add classes to a field.
   

   This would be better: "{{ attributes.class }} provides a way for modules to add classes to fields dynamically"

4. +++ b/core/modules/system/templates/field.html.twig
   @@ -29,13 +29,80 @@
   +  First we check if its a multiple value field and if it have a visible label.
   ...
   +  We check if its a multiple value field and if it have a visible label.
   ...
   +  We Check if its a single field and it have its label visbile
   ...
   +  Now check if the field is a single field and dont have a label shown
   

   I get why these are here but the are basically repeating the if statement above them? Are they really that useful? I would rather make the variables more verbose than have these comments.

5. +++ b/core/modules/system/templates/field.html.twig
   @@ -29,13 +29,80 @@
   +  The holy grail of single field data!
   

   We don't need this either/

6. +++ b/core/modules/system/templates/field.html.twig
   @@ -29,13 +29,80 @@
   +    <div class="field field--multiple {{ cssclass }}">
   

   This would print the field class twice?
   I think we should move {{ cssclass }} to the start of the class attribute so field--multiple isn't the first class

I have made the changes from the documentation that said Lewis #134, a screenshot with all the posibilities and I have removed some spaces between classes.

hope it would be usefull!

The screenshot from #135 shows that we're missing some vertical spacing between successive fields ? (compare with HEAD)

Also, my remark from #128 ("{% set cssclass %}") still stands ?

I still believe that, as suggested in #97, we are overdoing a little.

The template is getting more complex. If I'm a themer and I'm looking at the template for the first time it doesn't really makes me want to mess with it.
The HTML output is going from divitis to classitis. Are we trying to serve too much use cases with all those classes? When do we really need a class to distinguish single and multiple fields? Wouldn't it be better to make those variables available for the themers to use in the template but don't actually print them on the default template?

We need to consider that the first approach for most themers would be to scan the html code. If I look at the screenshot right now, I perceive a complexity with all those long classes that gives me the wrong impression. We need to understand that the default template shouldn't be a template to rule them all, but a good example that serves 80% of use cases and makes it easy to build on top of it.

If a person has enough technical chops to understand HTML and CSS, I don't think we need to worry about simple if/else/elseif logic in templates.

At least, I've never heard complaints from the frontend people I've worked with on this. And I've worked with a lot of weird people, present company included ;)

It's important to bear in mind that the markup output is also an interface for understanding what's going on. If you have some styling added somewhere and you look in a mark up inspector to find out where it's being applied, div soup makes it very difficult to understand. Removing complexity in the template adds complexity somewhere else.

I guess my main concern is with the number of classes and I understand there's a different issue for it.
In terms of markup I wonder if we need the "field__items" wrapper for multiple with label.
And more in general, I'm not a big fan of generic wrappers. In my, obviously limited, personal experience when I'm dealing with complex fields, I go and override the template anyway. That's why I was suggesting to get rid of wrappers all together and get a simple straightforward template and markup.

But I understand there might be somebody happy with a generic wrapper. I'm probably too much of a markup purist :)

#141: Well, either we'll need the field name on both field value and field label, or we'll need a wrapper around both.

I don't think it's that unreasonable to have a wrapper, it'll make the out-of-the-box markup a lot more useful (ie. being able to float the value to the side of the label).

But is the field item wrapper a common use case? I don't know, you already have a wrapper around the entire field, that's more useful. Is it really easy to add in if you want it? It is.

I think we have the markup and classes in good shape. Let's move this forward to RTBC.

+++ b/core/modules/system/templates/field.html.twig
@@ -29,13 +29,72 @@
+      <div class="field__item"{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>

Unless you can merge ArrayObjects (attribute.class) or objects that act like arrays(attribute) in Twig... this will just cause duplicate attributes. Also if you do merge attributes you need to know which attribute wins out in the duplicate attribute name (id="field--1" vs id="field__content--1" vs id="field__item--1")for a contrived example).

For the merging you can do that in preprocess with little resistance ATM, I know that may not be ideal but at the moment that's the only feasible way. But you still have the who wins in the merge.

FTR, I'm completely pro having "more attributes in the template and less everything in the preprocess" as an end goal here, just want to bring some reality to this experiment.

So for a way forward: If we can solve the issue of merging and other array manipulations(slice,sort,first,last,etc) for twig templates with non-arrays (AttributeArray and Attribute) and we deal with the "who wins" issue in either case this issue can have legs. Otherwise it's this guy:

Also, side note, I was really close at getting AttributeArray aka {{ attributes.class }} to be just a normal array, though currently that is borked because printing {{ attributes.class }} knows it's an array and tries to think it's a renderable array. @see #2239945-7: Refactor Attribute class into one class

1. +++ b/core/modules/system/templates/field.html.twig
   @@ -17,25 +17,90 @@
   +  <div class="{{ classes }}field--multiple">
   

   leftover field--multiple ? (it's only within a comment)

2. +++ b/core/modules/system/templates/field.html.twig
   @@ -17,25 +17,90 @@
   +{% elseif not multiple and not label_hidden %}
   ...
        {% for delta, item in items %}
   

   Seems confusing to keep a foreach in the "not multiple" case ?
   deltas are supposed to be sequential ints starting at 0, so items[0] should be safe here ?

3. +++ b/core/modules/system/templates/field.html.twig
   @@ -17,25 +17,90 @@
   +{% elseif not multiple and label_hidden %}
   ...
   +  {% for delta, item in items %}
   

   Same

Your merging function seems to work like php's array_merge_recursive()

It also makes all attributes an array, but we need to allow for non-array attributes right?

I haven't had a chance to actually test your earlier version of the patch but what was wrong with the output from NestedArray::mergeDeep()?

There are currently classes for array, string, and boolean attributes.

Single value attributes like id would be strings.

I thought one of the intended uses of nestedArray::mergeDeepArray() was for attributes, so I'll test out the previous patch a bit later today and see if I can work out what's going on.

@rooby and @mortendk this isn't perfect by any means but should get you a bit closer and yes @rooby deepMerge is the way to go. Only trouble is some things are arrays and others are ArrayObjects with AttributeBoolean/AttributeArray/AttributeString values in it, so merging becomes tricky. Cross my fingers on this though.

160: 2214241-field-divitis-160.patch queued for re-testing.

Ok, I am trying to apply #162, and it was impossible, apparently is making the same change twice somewhere, I believe.... I'll try again and see what I can see on why the function is not working.

I think we need to move the attributes to the preprocss, and leave the markup of the divs to the template. IMO

Fixing the tests…

@mortendk and @joelpittet Working on the merge, MergeDeep is not gonna make it, mergedeep will actually only add value of the last array passed for each key, unless those values are arrays themselves (if that makes sense) Basically the reason is failing is because that is the intended use of this, in my understanding.

The proposal is: Keep the attributes separated and use them as variables in TWIG, this way you put your attributes as you want and have more control, just merge the classes.

This can be done with a simple array_merge() and some test to make sure we do not pass a NULL value, or it returns NULL.

Another consideration, and the reason I don't submit a path, would be about the decisions being made about the theming layer in general and maybe moving clases to the template and out of the preprocess.

Any comments? what are the use cases where we need attributes merged? what concerns on RDF? Please comment so we can move forward.

Thanks

167: 2214241-field-divitis-167-reroll.patch queued for re-testing.

The test run locally, so it seems to be some stupid test bot thing…

Okay, it looks like all the test failures are caused by missing classes on fields – for example, with the #167 patch, the user picture is output like this:

<div class="field-items">
  <div class="field-item">
    <a href="/user/1">
      <img class="image-style-thumbnail" src="http://d8.d7.hoegh.co:8080/sites/default/files/styles/thumbnail/public/pictures/Screen%20Shot%202014-05-26%20at%2011.23.38.png?itok=hESC4Hn2" alt="" typeof="foaf:Image" height="100" width="87">
    </a>
  </div>
</div>

Since there is no identifying class on the field, the test cannot verify that it's output. I guess there should be at least a `field--name-user-picture` or similar somewhere…

Did this stall for good ? Would be sad, this was really promising :-/

Also, pinging @mortendk about #2229435: Clean up the way attributes are printed in field.html.twig - not sure that conflicts with the approach taken here.

Marked #935878: Don't output unnecessary markup for fields with just a single value entered. as duplicate of this one.

Rerolled

So I synced css from 2214241-field-divitis-167-reroll.patch

1. removed the debug code:
* @todo: remove these test classes for attributes merging */
...

2. there's no inline classes in html. But the css will stay so fix it from backend:
.field--label-inline .field__label,
.field--label-inline > .field__item,
.field--label-inline .field__items {

3. So why do you overwrite only 2 styles for rtl here:
[dir="rtl"] .field--label-inline .field__label,
[dir="rtl"] .field--label-inline .field__items {

This is the code before rtl:
.field--label-inline .field__label,
.field--label-inline > .field__item,
.field--label-inline .field__items {

So in rtl one does not overwrite ".field--label-inline > .field__item,"

I didn't test .field__items. Where can I see that? Maby it's not in D8?

4. Where can I test this:
submitted .field--name-field-user-picture img {

5. Anyways, I didn't check anything in the core/themes/bartik/css/style.css
except;
.field--type-taxonomy-term-reference,

Here is the interdiff for the previous patch.
There is a trailing space error in the patch.

This should fix some notices

I am making screenshots.

I will recreate the patch.