[Follow-up] Add a "Visually-hidden" field label formatter

Embedded mockup

I'll test!

I've tested the patch in #5 on the latest drupal-8.x HEAD (via Simplytest.me) and I can confirm it presents a - Visually Hidden - option for field labels on node, user, taxonomy term and comment "Manage display" forms.

I've confirmed that selecting the new Visually Hidden label option gives the field label the same CSS styles as the visually-hidden CSS class.

Aside: I've confirmed these styles effectively hide the field in at least Safari 7.0.2, Firefox 28, Chrome 33, Opera 20, IE8 (Win7), IE9 (Win7), IE10 (Win7) and IE11 (Win8), and make it visible to screenreaders (or at least, VoiceOver on OS/X 10.9.2) — but note that any problems with screenreader / browser compatibility are a separate issue.

***

I've reviewed the patch: it follows Drupal coding standards.

While I personally would have tried adding the visually-hidden CSS class to the label, I can see now that would have required much-more-complex changes to the code. I'm all for Keeping It Simple, so I'm happy to mark this as Reviewed and Tested By the Community.

LOL :D

#8 and #10 ask for the same thing — I'll figure out how to do that. And add tests.

This will be useful for writing tests https://drupal.org/contributor-tasks/write-tests

Current behaviour

Default (Stark)

The default template (in core/modules/system/templates/field.html.twig) looks like...

  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</div>
  {% endif %}

... note that hidden field labels will not be included in the HTML, the class is not included in the $title_attributes variable, and there is currently no way to modify the classes on the label.

Bartik

Bartik, on the other hand, has a template (at core/themes/bartik/templates/field--taxonomy-term-reference.html.twig) that looks like...

  <h3{{ label_attributes }}>{{ label }}: </h3>

... the preprocess function (core/themes/bartik/bartik.theme::bartik_preprocess_field()) looks like...

  if ($element['#field_type'] == 'taxonomy_term_reference') {
    $label_attributes['class'] = array('field-label');
    if ($variables['label_hidden']) {
      $label_attributes['class'][] = 'visually-hidden';
    }
    if ($variables['element']['#label_display'] == 'inline') {
      $label_attributes['class'][] = 'inline';
    }
    $variables['label_attributes'] = new Attribute($label_attributes);
  }

... note that, for hidden fields, instead of simply not outputting the field label, it applies the class we want to use in order to hide it from sighted users.

Analysis

Default (Stark)

I'm going to guess that the reason core/modules/system/templates/field.html.twig doesn't lump the class variable in the template is so that it's easy for novice themers to modify. This is a great way to make theming discoverable, but, IMO there should at least be a way to add classes.

If we took Bartik's lead and added the class attribute to the $title_attributes variable, we could |without() filter should allow us to continue to provide more-discoverable theming. Using this filter will expose more of Twig's powerful features to novice themers, but that could be considered A Good Thing.

Bartik

When there was only one Hidden option, it's unclear whether Bartik's approach was correct... in theory, it was more accessible; but it also exposes unnecessary (perhaps even sensitive) information about the data structure of the site to screenreaders or people who examine the HTML directly.

If there are to be two options (Hidden and Visually hidden), then Bartik's approach is now incorrect.

Proposed resolution

1. Modify Bartik so Hidden labels are not output at all. Leave the default theme as-is.
   Alternately, add the hidden class so the label stays in the HTML but is invisible to BOTH sighted and assistive-technology users (it will be visible in the markup).
2. Modify core/includes/theme.inc::template_preprocess_field() and add the class attribute to the $label_attributes variable. Modify core/modules/system/templates/field.html.twig so the class attribute is not output twice (using the |without() filter). Modify bartik_preprocess_field() so it doesn't completely stomp the $label_attributes variable.
3. In template_preprocess_field(), add the visually-hidden class when the label is set to the Visually hidden option.

Here's a patch that implements the resolution proposed in #14.

I've confirmed on my local copy that the label visibility settings Above, Visually-hidden and Hidden work as proposed in #14 in Bartik and Stark.

Tests still need to be written, but it's progress.

Actually, let's see if that breaks any tests...

Testbot?

#2214241: Field default markup - removing the divitis modifies the same files and will probably cause this issue's patch(es) to stop applying.

This was assigned but hasn't been touched in a while. Unassigning.

@mgifford you were testing in the wrong place. The setting does not affect the node-edit form: it affects the node-view form.

This issue still needs tests. Updating the issue summary.

@lanchez, thanks for adding tests!

For the most part, the code looks good, and it appears to function correctly (except if you manually test with taxonomy term reference fields in Bartik, it doesn't work because of #2298923: Bartik's field--taxonomy-term-reference.html.twig doesn't output content_attributes, has out-of-date documentation).

However, that shouldn't postpone this issue because that bug only applies to certain types of fields in a certain theme. I'm updating the issue summary to mention manually testing that particular thing will fail.

I'm marking this as RTBC, but I do have some minor nit-picks, which you may change if you want, but aren't necessary:

1. In the future, adding an interdiff when you add a patch would be ideal... it helps reviewers see what you changed. I've attached one for the differences between #29 and #38.

2. -    <div class="field-label {{ title_attributes.class }}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>
   +    <div class="field-label{% if title_attributes.class %} {{ title_attributes.class }}{% endif %}"{{ title_attributes|without('class') }}>{{ label }}:&nbsp;</div>
   

   Because leading and trailing whitespace is ignored for the HTML class attribute, I'd rather leave this the way it was (short and clear, but with a trailing space).

3. +    $this->assertText('field_test_entity_display_build_alter', 'Alter fired, display passed.');
   +    $this->assertText('entity language is ' . LanguageInterface::LANGCODE_NOT_SPECIFIED, 'Language is placed onto the context.');
   +    $array = array();
   +    foreach ($this->values as $delta => $value) {
   +      $array[] = $delta . ':' . $value['value'];
   +    }
   +    $this->assertText($setting . '|' . implode('|', $array), 'Values were displayed with expected setting.');
   +
   

   I don't think duplicating this part of the test is necessary: other code in the same method already tests this.

Note this patch supersedes the changes made in #1106344: Taxonomy term reference field headers always should be rendered with a HTML header (invisible or not), i.e.: if this patch gets in, Bartik shouldn't visually hide "hidden" taxonomy term field labels anymore (because someone could choose "visually hidden" instead). This has been part of the patch since #15 but I thought I'd document it anyway.

Adding related issue.

Added a follow-up task: #2299477: Don't display taxonomy term reference field headers when field label display is set to "hidden". Edit: never mind, that was done in this patch. :P

Need to update the site building guide and other documentation pages. I'll post links to the changed pages once I find the appropriate place to do so.

Hide content properly has been updated to mention this feature.

The documentation on specifying how fields are displayed has been updated to mention this feature.

@yched I would be fine with that.

Here's a patch to do that.

Note this is a follow-up to something already committed, so there is no interdiff.

Whoops, change status.

Umm... I just noticed the original patch introduced a problem in Bartik: the bartik-taxonomy-term-reference-field-only $variables['label_attributes'] variable was replaced with the standard $variables['title_attributes'] but the corresponding line in the field--taxonomy-term-reference.html.twig was not changed. And there's an array wrapper that wasn't supposed to be there: $variables['title_attributes']['class'][] = array('field-label');.

Should I file a follow-up ticket, add it to the follow-up patch from #48 or wait until #48 gets in and make another follow-up?

EDIT: @alexpott suggested this new issue should be filed as a follow-up ticket, so I've filed #2301599: Regression: Bartik's field--taxonomy-term-reference.html.twig doesn't properly output label attributes.