Rather than having block visibility be toggled by leaving an empty Block title, which creates a large number of issues (blocks page with seemingly "emtpy" blocks, no name to match machine name, a inconsistent pattern with other places) - we should allow its visibility to be toggled.

A simple checkbox that says "Display block title".

Files: 
CommentFileSizeAuthor
#96 drupal-block_title_hide_option-2511754-3_0.patch4.49 KBbgilhome
FAILED: [[SimpleTest]]: [MySQL] 41,349 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#87 block-1875260-86.patch10.64 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 53,321 pass(es).
[ View ]
#87 interdiff-86.txt2.64 KBxjm
#83 block-1875260-83.patch10.12 KBbrantwynn
PASSED: [[SimpleTest]]: [MySQL] 53,497 pass(es).
[ View ]
#83 interdiff.txt783 bytesbrantwynn
#81 block-1875260-80.patch10.12 KBbrantwynn
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#72 block-1875260-72.patch10.23 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#72 interdiff-1875260-72.txt780 bytesxjm
#71 block-1875260-71.patch10.24 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#66 1875260-65-no-title.png14.23 KBandymartha
#66 1875260-65-yes-title.png15.96 KBandymartha
#66 1875260-65-before.png38.26 KBandymartha
#66 1875260-65-after.png65.22 KBandymartha
#65 1875260-65.patch10.24 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 52,503 pass(es).
[ View ]
#61 1875260-61.patch5.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 52,227 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#61 interdiff.txt723 bytesGábor Hojtsy
#56 1875260-56.patch5.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 52,243 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#53 1875260-53.patch5.26 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 52,161 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#46 1875260-46.patch4.4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 52,284 pass(es).
[ View ]
#46 Screenshot_2_26_13_1_33_PM-3.png25.77 KBGábor Hojtsy
#42 1875260-42.patch2.22 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,232 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#40 block-title-1875260-40.patch2.27 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 52,219 pass(es), 34 fail(s), and 0 exception(s).
[ View ]
#33 block-title-1875260-33.patch1.98 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 52,178 pass(es), 35 fail(s), and 0 exception(s).
[ View ]
#16 block-title-1875260-15.patch3.16 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es).
[ View ]
#16 interdiff.txt1.17 KBxjm
#12 block-display-title-1875260-12.patch2.9 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 50,557 pass(es).
[ View ]
#12 DisplayTitle.png32.99 KBGábor Hojtsy
#8 block-hide-title-1875260-8.patch2.87 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,551 pass(es).
[ View ]
#8 hide_title.png17.27 KBxjm

Comments

tim.plunkett’s picture

+1, this is closer to how Panels handles it, and I think it's a great idea.

xjm’s picture

Status:Active» Postponed
Issue tags:+Blocks-Layouts

Agreed. Also see #1875016: Automatically generate machine names for block plugins. This issue makes that one a lot simpler.

Postponing on #1535868: Convert all blocks into plugins.

xjm’s picture

Status:Postponed» Active
Gábor Hojtsy’s picture

Not sure what does this relate to. I tried to have steps to reproduce but could not:

- went to edit the "Tools" (menu exposed) block
- removed the title altogether in the textfield and saved the block
- the title does not show on the frontend anymore
- the backend block list still has the "Tools" title

Also when the block is retitled, the backend shows "Tools" regardless of the frontend title.

So not sure what this empty title refers to in the report.

Bojhan’s picture

@Gabor I dont think you are following, the whole idea is not having to delete the title altogether but instead toggle its visibility

Gábor Hojtsy’s picture

@Bojhan: I've been trying to reproduce the "blocks page with seemingly "emtpy" blocks" mentioned in the OP but was unable to. The block system has separate descriptions and titles for blocks. The descriptions for blocks originate from the block provider for menu provided blocks (eg. the menu title for menus) and cannot be changed/edited/provided by the admin except for custom blocks created by the user.

Starting from the non-reproducible problem description of "blocks page with seemingly "emtpy" blocks", I sense you maybe suggesting the block title become the block description instead, so the front end and backend "titles" are not different but there is a toggle as whether to display the title or not on the frontend.

xjm’s picture

Assigned:Unassigned» xjm

I'll work on this to clarify things with code. :)

xjm’s picture

Assigned:xjm» Unassigned
Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new17.27 KB
new2.87 KB
PASSED: [[SimpleTest]]: [MySQL] 50,551 pass(es).
[ View ]

Something like this.

hide_title.png

Edit: Eventually, of course, we'll make the machine name like:

tim.plunkett’s picture

With the ConfigEntity issue, we could add a method that wraps Block::label(), I'd think.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

@xjm: I'd suggest *not* to use "Hide" to describe a checkbox (a positive action). Usually if you do not check a checkbox, it means something is not turned *on*. We have been burned by this and confirmed with user testing that even if people read the description, they interpret the checkbox as if it would mean "show". So better make it to "Show" instead :) See #1853720: Hide language selection option is backwards

Also, does this deprecate the block description for the custom blocks and/or deprecate the block provider set block description that is currently used on the block admin UI? Seems like if the block title is required, the block description (AKA admin title) should not be a separate thing?

Bojhan’s picture

We could totally do, "Display block title". And have it toggle off, to not display it. Thinking out loud, we could pretty much remove description as it has nu purpose anymore.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new32.99 KB
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 50,557 pass(es).
[ View ]

Here is a reroll with the "display" terminology. Note that this hides all block titles by default, I could not figure out immediately where this default would need to be set proper. Pointers would be helpful :)

DisplayTitle.png

xjm’s picture

Assigned:Unassigned» xjm

With the ConfigEntity issue, we could add a method that wraps Block::label(), I'd think.

I dunno; I can see usecases for having the hidden title value available throughout the render process. In fact I'm considering moving it into a different template variable in the preprocess in case themers wanted to do something with it.

I'll work on #12.

xjm’s picture

Issue tags:+Needs upgrade path

I just saw Bojhan's idea about removing the block description. +1. This also got me thinking about the upgrade path for this -- not that we have an upgrade path for blocks at all yet -- but we could use the block description as a hidden title for upgraded blocks that have an empty string or <none> as the title.

Gábor Hojtsy’s picture

@xjm: since blocks don't have any upgrade path yet, I don't think we could code that in any way in this issue, so I'd rather we add that task in the upgrade path issue if/when we remove descriptions.

xjm’s picture

StatusFileSize
new1.17 KB
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es).
[ View ]

Actually it looks like only custom blocks have descriptions. (Was this the case in D7?) So removing it would be best as a followup to #1871772: Convert custom blocks to content entities. Edit: looking at @larowlan's video, it's already gone there as well.

Attached makes the Display title field checked by default and adds a "hidden_title" block property to the preprocess for theming. Next I'll write some tests for it.

Gábor Hojtsy’s picture

@xjm: yes, non-custom blocks use the module provided description (from the info hook). The runtime title is provided in the view hook, so that is how those two are different for non-custom blocks. I assume we'd want to have them the same here now that we make title required :) So if you change the title it would be reflected in the admin block overview too.

xjm’s picture

Gábor Hojtsy’s picture

Issue tags:+Usability

Definitely did nto want to remove the Usability tag. Meh.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Definitely did nto want to remove the Usability tag. Meh.

+++ b/core/modules/block/block.moduleundefined
@@ -568,6 +568,11 @@ function block_rebuild() {
+  // If the block title is configured to be hidden, set it to an empty string.
+  if (empty($variables['block']->display_title)) {
+    $variables['block']->hidden_title = $variables['block']->subject;
+    $variables['block']->subject = '';
+  }

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -261,7 +259,13 @@ public function form($form, &$form_state) {
+      '#default_value' => isset($config['subject']) ? $config['subject'] : '',
+      '#required' => TRUE,
+    );
+    $form['settings']['display_title'] = array(
+      '#type' => 'checkbox',
+      '#title' => t('Display title'),
+      '#default_value' => isset($this->configuration['display_title']) ? $this->configuration['display_title'] : TRUE,

I think this title/subject terminology back and forth is not very ideal....

Both in the theming code and in the form. Eg. the config for 'title' is coming from 'subject' but for 'display_title' it comes from 'display_title' (not 'display_subject'). Why do we still use 'subject' and can we standardise on that or on title?

sun’s picture

For accessibility reasons, I think we actually need to support more than a Boolean flag — whenever we're "hiding" titles from the UI, we most often do not actually omit them; they get an .element-invisible class on their wrapper instead, so screen-readers can make sense of the content that follows. This should likely be the default effect of "not showing" the title.

However, there can also be legit use-cases in which you might want to actually remove a block's title altogether (e.g., the site logo block doesn't need to state what content follows, it's part of the content already), so I'm certain that we want to have there or even four options:

- visible
- invisible (.element-invisible)
- hidden (.element-hidden; useful for JS)
- none (no output at all)

Bojhan’s picture

@sun No. Seriously, we cannot make every piece of functionality incredibly complex - this is frustrating, we always opt for the most difficult solution for the end-user. No user knows what the hell those options are. If we need to do that, I'd rather just won't fix this.

sun’s picture

In that case, we need to retain accessibility. Removing the title altogether is bad for accessibility. Menu blocks and others need to have a heading; this is actually hard-coded for some stuff into page.tpl.php right now (as well as template.php of core themes).

So we want to make the default effect of "not showing" the title .element-invisible, which means that the title is actually still output, just not visible for sighted users.

tim.plunkett’s picture

Then we'll make sure we don't cause regressions in those use cases, but this is not an option that should be exposed in the UI. The checkbox will just serve as a replacement for the <none> functionality.

Gábor Hojtsy’s picture

@sun: just as tim said, this is an existing functionality. You can remove the title altogether or not. We are not removing that or expanding on that, merely expose the existing functionality with a different (simpler) UI in this patch. Proposals to expand on the feature set would fit in other issues I think. Field labels have the same problem, where the field label is sometimes important to understand the field value, but is still also only possible to hide entirely or display, so if you consider this a reasonably big problem, a more widespread "display mode" solution would be useful elsewhere.

xjm’s picture

Title:Allow block title visibility to be toggled» Make the block title required and allow it to be hidden

I actually think wrapping it in .element-invisible is a fine suggestion, though maybe out of scope for this issue. The notion that some sites might want to do this is actually the usecase I was considering when I added it to the template variables in #16, and why I don't want to fix it at the entity level.

Both in the theming code and in the form. Eg. the config for 'title' is coming from 'subject' but for 'display_title' it comes from 'display_title' (not 'display_subject'). Why do we still use 'subject' and can we standardise on that or on title?

I think this is out of scope here. See #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer and #1871696: Convert block instances to configuration entities to resolve architectural issues.

Gábor Hojtsy’s picture

Yeah, well, I thought we want a straight new UX for the existing functionality to hide the title (necessitated by making the title required as internal name). If people agree instead of the existing functionality we should do that and that is not out of scope (I think it is out of scope), then do that :)

xjm’s picture

Issue tags:+Block plugins
Gábor Hojtsy’s picture

#1875016: Automatically generate machine names for block plugins was actually fixed without this :) Looks like we still favor a straight port of the feature?!

xjm’s picture

mgifford’s picture

I was thinking it would just be a checkbox that would just switch it from the default (visible) to invisible (.element-invisible). That's a pretty simple UX change.

Maybe once that checkbox is selected there is a link to allow someone to change it to hidden or none. I can see some real advantages to providing access to hidden/none from the UI. Although it's really not for the 80% I don't think.

Although a select list that's defaulted on (visible, invisible, hidden, none) doesn't sound like a whole lot more than a checkbox.

I tried to apply the patch from Jan 8th, then tried to update it, but BlockBase.php has changed too much.

It's definitely important for #1869476: Convert global menus (primary links, secondary links) into blocks

Gábor Hojtsy’s picture

I was thinking it would just be a checkbox that would just switch it from the default (visible) to invisible (.element-invisible). That's a pretty simple UX change.

You can make the title hidden *now* by removing the title. That was possible in Drupal 7 by specifying <none> as the block title. That is existing functionality. First and foremost I think we want to make the existing functionality less obscure. Adding other variants of how the title is hidden would be added functionality on top of that. While I don't believe that is bound to feature freeze that passed already, I don't think we should endlessly argue in this issue about that :)

Gábor Hojtsy’s picture

StatusFileSize
new1.98 KB
FAILED: [[SimpleTest]]: [MySQL] 52,178 pass(es), 35 fail(s), and 0 exception(s).
[ View ]

Looked at rerolling the above patch. The page title logic for the configuration page went to block.admin.inc and does not need changes anymore. So the patch became a little smaller. I'm still very confused myself by this labeling issue. We have a form item called 'label' that will end up in the block's 'subject' and then a 'display_title' checkbox decides whether the text that was entered as 'label' but then is displayed as 'subject' is actually displayed. That is pretty mind-blowing. Not really sure what terminology to side with so I just left it as-is it was used/suggested by @xjm, but I'd not say I understand it.

Gábor Hojtsy’s picture

Issue tags:+Spark

Also add Spark tracking tag. We are interested in this from a usability perspective (so people don't need to be explained they need to empty out their title for example).

Gábor Hojtsy’s picture

Also as for the terminology problem, I looked at and rerolled #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer. That does change 'subject' to 'admin_title', where 'subject' used to mean 'what the subject value would be by default', but then the live label value is still called 'subject' even with that patch, so not sure that is the right one to point to to clean up the terminology problem.

Also, seems like no other place internally calls the title a 'title', eg. the block template has this snippet (note standard $title_* vars and the subject property):

  <?php print render($title_prefix); ?>
<?php if ($block->subject): ?>
  <h2<?php print $title_attributes; ?>><?php print $block->subject ?></h2>
<?php endif;?>
  <?php print render($title_suffix); ?>

I'm not sure where the block label/title/subject is actually called a 'title' in the code, so using display_title might not be a good name for this value.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
Gábor Hojtsy’s picture

Category:task» bug

As per http://drupal.org/node/1884762#comment-6981884 the title not being required but the required but invisible machine name depending on it causes a bug where if you don't provide a title for a new block, the form will be impossible to submit. Every other object where the required machine name is derived from the title/label as the title/label required, so we should make it so here too. Not only for the UX improvement mentioned above, but also for consistency. Refiling as bug report due to those findings.

Status:Needs review» Needs work

The last submitted patch, block-title-1875260-33.patch, failed testing.

Gábor Hojtsy’s picture

So the problem seems to be that the default of display_title will not take from where I wanted to put it in BlockBase::getConfig(). I tried overriding PluginBase::getDefinition() in BlockBase with similar code (to ensure display_title is set by default to TRUE). Anybody knows a good place to do this?

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [MySQL] 52,219 pass(es), 34 fail(s), and 0 exception(s).
[ View ]

Discussed this with @EclipseGC on Friday, he jumped into trying to solve this but did not succeed I believe. He found unrelated(?) facts that disturbed him about the block system. I figured in the meantime that the config entity would surely need the display_title property added (and exported), so the config is properly maintained.

This does not yet make the title displaying work, still needs to be investigated. Moving to needs review for testbot.

Status:Needs review» Needs work

The last submitted patch, block-title-1875260-40.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] 52,232 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Let's try something like this.

Eclipse

Gábor Hojtsy’s picture

+++ b/core/modules/block/block.moduleundefined
@@ -531,6 +531,11 @@ function template_preprocess_block(&$variables) {
   $variables['block'] = (object) $variables['elements']['#block_config'];
+  // If the block title is configured to be hidden, set it to an empty string.
+  if (empty($variables['elements']['#block']->display_title)) {
+    $variables['block']->hidden_title = $variables['block']->subject;
+    $variables['block']->subject = '';
+  }

I think this is pretty puzzling that the #block_config is only some of the block stuff and not really the whole entity configuration?! Also that it has an anonymous object and then the real one is in #block. The use of that anonymous object predates this patch, so I don't think it needs to be solved here, but the resulting code is pretty puzzling :/ Hm.

Status:Needs review» Needs work

The last submitted patch, 1875260-42.patch, failed testing.

tim.plunkett’s picture

Yes, I added that to directly mimic the existing code, so that the block.tpl.php wouldn't have to change. It should absolutely be rearchitected.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new25.77 KB
new4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 52,284 pass(es).
[ View ]

Fixed the block storage test and added a specific test for the title visibility setting. It works. :)

Finally I'm still not convinced that display_title should be used as terminology. The form item for what it controls is 'label' and the theme display element is 'subject'. Sure, there are other title_* elements in the template but not the main block title. Are we making that a 'title', so this will fit into a future terminology setup?

Minus the terminology question, this looks like it would be good to go? The visual look of it (again) is:
Screenshot_2_26_13_1_33_PM-3.png

Gábor Hojtsy’s picture

Also no upgrade path yet (#1871700: Provide an upgrade path to the new Block architecture from Drupal 7), so no way to integrate this change in any upgrade.

sun’s picture

  1. We have a #title_display property in Form API already. I'd recommend to rename the property to title_display for DX and consistency.
  2. In case we do not support hidden vs. invisible in core, then we should at least allow contrib to enhance the title_display setting easily.
  3. To do so, I'd recommend to use a #return_value of 'hidden' for the checkbox element.
  4. That way, a contrib module is able to replace the form element widget with a select or radios, and thus, an alternative value of 'invisible'.
  5. The existing processing and behavior for a 'hidden' value in core remains to kick in, and the contrib module only has to cater for its additional 'invisible' value.
Gábor Hojtsy’s picture

How would that said contrib module then alter the output of the template?

Wim Leers’s picture

#49: no template output altering would be necessary; it'd just be setting a different value in the render array, according to http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc... ? (Though it seems #title_display === 'hidden' is not yet supported — but maybe that reference page just isn't up-to-date yet.)

Gábor Hojtsy’s picture

I did not understand @sun actually suggesting we use the #title_display property. The block template is not really render array driven. So the best that seems like possible from the template is contrib would be able to fiddle with $title_attributes? http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

sun’s picture

Yes, such a module would then simply futz with the $title_attributes variable, which is as simple as:

<?php
if ($block->title_display == 'invisible') {
 
$variables['title_attributes']['class'][] = 'element-invisible';
}
?>

Since the value is no longer 'hidden', the built-in logic in core to hide the title is no longer triggered.

The rename from $display_title to $title_display is merely for DX/consistency, because we have that concept in Form API already. Thus, it is familiar and behaves similarly but it is OK if it's not identical.

Gábor Hojtsy’s picture

StatusFileSize
new5.26 KB
FAILED: [[SimpleTest]]: [MySQL] 52,161 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Ok, made it use title_display. Also changes hidden_title to subject_hidden to conform to other values in the block object. This can be further fixed when the block template is improved. Added docs for subject_hidden.

I'm not entirely sure that title_display as empty string to make a title display is a good choice or that its empty string or 'hidden' values make it appear anywhere close to #title_display in formapi. I don't think #empty_value works for checkboxes so I could set something like 'visible' if not set. I can add even more workaround code to the submission handler to set it like that if that is preferred.

sun’s picture

A value of 'visible' sounds sensible to me.

FWIW, theme_form_element() uses 'before' and 'after' as more fine-grained values instead of 'visible', but that concept does not exist here.

In essence, I think the core checkbox would flip between the values 'visible' (checked) and 'hidden' (unchecked).

Status:Needs review» Needs work

The last submitted patch, 1875260-53.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new5.78 KB
FAILED: [[SimpleTest]]: [MySQL] 52,243 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

@sun: ok, the problem I mentioned is you can provide the return value for a checkbox to return when set, but not to return when not set right? Am I missing something? So we need to add lots of obscure workaround code to make sure it ends up saying "hidden" when the checkbox is not checked. What and I missing? This workaround stack seems to be too much for me to support theoretic contribs.

Status:Needs review» Needs work

The last submitted patch, 1875260-56.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review

Fails were unrelated. Drupal\Core\Config\StorageException' with message 'Failed to write configuration file ....

Gábor Hojtsy’s picture

#56: 1875260-56.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Needs tests, +Blocks-Layouts, +Spark, +Block plugins

The last submitted patch, 1875260-56.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new723 bytes
new5.78 KB
FAILED: [[SimpleTest]]: [MySQL] 52,227 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Now that we have the real fail, turns out I forgot to rename in the test. However this will *still* fail due to the checkbox not actually being unchecked. Is there some special thing I should do for checkboxes with #return_values? Have not found anything for this.... Setting it to FALSE worked well above before we made this #return_value magic... @sun, any tips?

Status:Needs review» Needs work

The last submitted patch, 1875260-61.patch, failed testing.

Wim Leers’s picture

#51: I dug into this and … the test is failing because even if you do it manually, it is impossible to uncheck "Display title"…

I also discovered a new bug, that is seemingly not covered by tests: the "Delete" link/button at admin/structure/block/manage/bartik.powered_by_drupal/configure also doesn't work.

Gábor Hojtsy’s picture

@sun: any golden insights or should I abandon the #return_value magic and alter values in the form submission handler instead?

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new10.24 KB
PASSED: [[SimpleTest]]: [MySQL] 52,503 pass(es).
[ View ]

Ok, I took way more time again to try and figure this out but was unable to. So I opted to make it understand empty as hidden and anything else as visible. This still provides contribs plenty of opportunity to fiddle with this. They can set it to anything non-empty and react to that. I also changed it all around to label_display instead of title_display. While title_display is an existing form API property, blocks don't have anything called a title. In fact they don't even have subject anymore. They have two things. label and admin_label. No title. Even in the template. None. So naming this label_display made most sense to me. Introducing yet another version of confusing terminology now that blocks finally settled on label and admin_label would be bad DX. I don't think the resemblance to the unrelated and differently behaving form API property could be a plus. It could just as well be distracting and misleading.

andymartha’s picture

StatusFileSize
new65.22 KB
new38.26 KB
new15.96 KB
new14.23 KB

Before the patch, I could not toggle the block title.

After applying patch 1875260-65.patch from #65 by Gábor Hojtsy, I can confirm that the checkbox displays the block title with a name of "label_display" and a value of "visible". A block title, when displayed, shows up in an h2 tag.

However, unchecking the checkbox seems to remove the block title from the html source on page render, which works. I thought the discussion was about giving the h2 title a "hide from view, display in code" attribute.

Gábor Hojtsy’s picture

@andymartha: hiding the block title from the markup is an existing feature. If without the patch you empty out the title, it is not displayed. The new behaviour keeps the existing functionality making it more intuitive with the checkbox.

andymartha’s picture

thanks for the pointer, just trying to help out clearing issues before weekend of sprints

xjm’s picture

Assigned:xjm» Unassigned
Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

@andymartha, for the "display in code" functionality, one could override template_preprocess_block() and change the output. That change is out of scope here though.

I think this is finally RTBC? :)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Sorry all, this no longer applies.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new10.24 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Rerolled. The bit that didn't apply was in context lines in default blocks; the langcode was recently changed from LANGUAGE_NOT_SPECIFIED to English for several blocks, e.g.:

diff --cc core/profiles/standard/config/block.block.bartik.content.yml
index fd53c61,2dcbe6b..0000000
--- a/core/profiles/standard/config/block.block.bartik.content.yml
+++ b/core/profiles/standard/config/block.block.bartik.content.yml
@@@ -18,4 -18,5 +18,9 @@@ label: '
  module: system
  region: content
  weight: '0'
++<<<<<<< HEAD
+langcode: en
++=======
+ langcode: und
+ label_display: visible
++>>>>>>> 65
xjm’s picture

StatusFileSize
new780 bytes
new10.23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Oops, I got a little hyper with the enter key when resolving the merge conflicts.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Looks good. RTBC assuming it passes :)

xjm’s picture

#72: block-1875260-72.patch queued for re-testing.

brantwynn’s picture

Tested #72 manually and it applies cleanly to latest HEAD. I was able to disable the 'Tools' block title without any problems. Also made a custom block and was able to show/hide the title using the checkbox toggle on the block's configuration.

sun’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -238,6 +238,13 @@ public function form($form, &$form_state) {
       '#title' => t('Title'),
...
+      '#required' => TRUE,

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -53,6 +53,16 @@ class Block extends ConfigEntityBase {
+   * Drupal core hides the title altogether if the value is
+   * empty. Otherwise the title is displayed.

1) Why is the title required now?

2) If it remains to be required, then the phpDoc of the $label_display property doesn't make much sense?

+++ b/core/profiles/minimal/config/block.block.stark.login.yml
@@ -16,3 +16,4 @@ visibility:
+label_display: visible

Hm. Isn't this the default value?

If it's the default value, then we do not have to add it to all default configuration files.

sun’s picture

xjm’s picture

#72: block-1875260-72.patch queued for re-testing.

brantwynn’s picture

Status:Reviewed & tested by the community» Needs work

@sun

1) per Gabor in #27

...we want a straight new UX for the existing functionality to hide the title (necessitated by making the title required as internal name).

2) The phpDoc needs updated as it reflects the old behavior.

I also agree the extra config for the themes is extra - won't that value get written anyway once a user saves the block?

xjm’s picture

@sun, yes, we are, but this simple change has been outstanding for three months now. We can change it everywhere when we change it. No reason to block this on that.

@brantwynn I don't understand your comment or what you think is NW here.

brantwynn’s picture

Status:Needs work» Needs review
StatusFileSize
new10.12 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Updated the phpDoc of the $label_display property.

brantwynn’s picture

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -53,6 +53,13 @@ class Block extends ConfigEntityBase {
   public $label;

   /**
+   * The label is displayed by default.
+   *
+   * @var string
+   */
+  public $label_display = 'visible';
+

Changed the phpDoc in patch 80 above.

brantwynn’s picture

StatusFileSize
new783 bytes
new10.12 KB
PASSED: [[SimpleTest]]: [MySQL] 53,497 pass(es).
[ View ]

Trying this again the *right* way with proper interdiff. Sorry @xjm

Gábor Hojtsy’s picture

@sun: on adding the default value to shipped config, I believe our express goal is that when config is re-saved, only real changes are seen (eg. when staging new config and diffing). If we don't include default values in the shipped config files, then diffs will show non-changes as changes, and then the staging/diff system for config is that much harder to use. So we should include the exported config as-is it would be saved with the UI. See eg. #1938570: Make views active config save format match the default yml file (order and quotes).

xjm’s picture

So, @brantwynn's patch makes me realize we probably should have a constant for the label display visibility, so that it's not a magic string.

Gábor Hojtsy’s picture

Where would that constant sit?

xjm’s picture

StatusFileSize
new2.64 KB
new10.64 KB
PASSED: [[SimpleTest]]: [MySQL] 53,321 pass(es).
[ View ]

Like this.

brantwynn’s picture

Just looking at the interdiff from #87 and this makes sense. Also @xjm's comments are much more explanatory than the one I wrote.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Superb, thanks! Let's get this in now. Compared to the complexity of the effect of this, we definitely spend way too much time already on this.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committing it while it's hot! ;)

Committed and pushed to 8.x. Thanks!

YesCT’s picture

I can hide the title of a default block, like search.

I cannot show the title of a custom block. I'll open a follow up for hiding and showing titles for custom blocks.

Gábor Hojtsy’s picture

@YesCT: custom blocks have three things:

- the custom block *type* has a name; that is like a node type name, article, book, etc :) just for blocks; you likely don't want to see this on the UI
- the custom block *itself* has an admin title (description), which shows only on the backend admin interface; this is not currently supposed to show on the frontend; non-custom blocks have this provided by their respective plugins/modules (and this admin label is not editable for non-custom blocks)
- the placement of the block has a title, which can be shown or hidden; while this initially copies the admin label to be its initial value, then it is totally independent, so you can set it to whatever; this will NOT show on the backend but will show on the frontend (if the checkbox is checked)

So each block has a backend only title which is initially copied to the frontend only title, but then the two are independent. This applies whether custom block or not. This is current behaviour. If it does not work for you like that, then you found a functionality bug. If you expect different behaviour, then you probably found a UX bug :)

Hope this helps.

brantwynn’s picture

^^ I was about to say - this is exactly how Bean works in D7. There's block type (bundle), admin title (label), and the actual rendered block title (title).

The only difference is the label and title being copied in D8 blocks - in Bean you have to enter both values from the get-go.

So each block has a backend only title which is initially copied to the frontend only title, but then the two are independent.

We found the UX in Bean is confusing with label exposed but it is necessary. Thanks, Gabor for clarifying that the architecture here.

Perhaps we can get a UX discussion going about block labels (backend only titles) in a followup?

YesCT’s picture

Re #91: what I thought was a bug was the default for bartik to always hide block instance titles in the header region (where I was putting my test blocks). So I thought the show title check box was not working.
If I found any bug, then it's a ux bug with bartik. It's probably works as designed.

For what to call the things: block type - name, block - admin title/admin label/description, block instance - block/placement title
See (I think):
#1956496: Custom block contextual links should differentiate between editing/deleting the block instance and the custom block entity
And
#1956448: Use the block instance title on the block admin listing

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

bgilhome’s picture

Version:8.0.x-dev» 7.x-dev
Issue summary:View changes
Status:Closed (fixed)» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new4.49 KB
FAILED: [[SimpleTest]]: [MySQL] 41,349 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Adding patch backported to D7 for review. from https://www.drupal.org/node/2511754#comment-10056572

Status:Needs review» Needs work

The last submitted patch, 96: drupal-block_title_hide_option-2511754-3_0.patch, failed testing.

mgifford’s picture

Version:7.x-dev» 8.0.x-dev

Ok, just setting it back to D8 so it can be fixed there first.

David_Rothstein’s picture

This was fixed in Drupal 8 above.

Not entirely sure the Drupal 7 patch in #96 is a backport though? It looks like a different feature, to add support for screenreader-only block labels. Does Drupal 8 even do that? Sounds like it should probably be a separate issue...

David_Rothstein’s picture

Version:8.0.x-dev» 7.x-dev

This was fixed in Drupal 8 above.

I meant to change the version when I wrote that :)

mgifford’s picture

Oops, sorry. I missed that in #90.