Add option for visually-hidden block titles

This would be a great addition to Core!

In a sense a ternary version of the label_dispaly variable is implemented in the block twig.html files. Currently the status of 'label_display' is tested 2 ways in drupal twig templates. These are 8 templates that use {% if label %}. These are relying on "template_preprocess_block" to actually test the config setting of label_display and simply skip rendering of the title if it is empty. 3 menu twig template files use {% if not configuration.label_display %} and add the "visually-hidden" class to the title element if it is not set.
It seems to me any one can have local copies of block templates and make their own decisions about how to handle the label. The existing block templates actually do show the 3 possible options identified in this issue, ie. the block label can be omitted, visually hidden, or visible.
I do think some existing blocks should be reviewed(eg. site branding block) as to whether they should omit the label or visually hide it, for ada conformance.

I created a new issue that I think is relevant to this one, #2715687: make templates use 'display_label' in consistent manner. I am hoping to spur discussion there that could affect the eventual result of this issue.

This would give more control to the users editors.

Related issue fixed, so new constants should live inside plugin interface #2829680: BLOCK_LABEL_VISIBLE is defined on the wrong interface

Patch rerolled against 8.4.x including changes from #18
It still needs the tests to be fixed and new tests to be added, though.

I added an upgrade path and fixed core templates that used the old way to hide block title.
I'm afraid we should use a "0" value instead of the "hidden" we introduced in this patch to preserve BC. It's up to discussion.
We still need tests here, though.

I discussed about it offline at dev days Seville with Artusamak and we thought it was possible to introduce that new option without breaking BC.
Here is a patch that removes BC breaks from the previous one by reverting the \Drupal\Core\Block\BlockPluginInterface::BLOCK_LABEL_HIDDEN value to "0", which is it's current value, and reverting the code that were changed in templates to make it work when it was "hidden".

Next step is to fix the existing tests, then write new ones.

I'm still working on this one.

Tests should be fixed and completed.
Give me a green, dear testbot!

---
I had to fix an existing test (\Drupal\block\Tests\BlockTest::testHideBlockTitle) that were doing strange things.
In fact, that test was testing that the block title was not shown by default. But, given the existing UI, the block title IS shown by default. That's just because of the internal \Drupal\simpletest\WebTestBase::drupalPostForm() mecanisms that mess with checkboxes that the tests didn't fail right now (not sending a value is like sending an unchecked checkbox, which is different from its default value). It would have failed when converted to BrowserTestBase anyway.

Fixing the last tests

I think you don't need my phpunit.xml file :/
Sorry again

We could use screenshots here and there is some manual testing to do.
Novices are welcome!

After applying the patch #30, I checked that is worked on:

Chrome

58.0.3029
57.0.2987

Firefox

52.0.1
52.0
51.0.1
51.0

Also screenshots are added (PRE and POST patch).

I also updated the summary with the tasks done.

@agomezmoron want to mark it RTBC?

@mgifford sure!

Nice!

AFAICT this needs usability review. The IS (issue summary) should also have a before vs after screenshot. Those screenshots already exist in #32, but should be added to the IS so that they're easy to find for a core committer and a usability maintainer.

Should it be moved it into Needs Review? I think it doesn't make sense keeping it in Needs Work.

Thanks for a solid issue summary and task list! I think adding this is a good idea. Some UI considerations:

3 options in a dropdown is not really nice :-\ I know this is similar to label visibility in Field UI, but that's consistency with a not so good user interface so we can allow some room to experiment here.

Accessibility-wise, is it even a good idea to have the option to exclude the title from the html output? Could we make this a boolean that toggles between show title and visually hidden? Put differently: is visually-hidden preferable over hidden?

I tried to come up with another way to go about this that at least initially looks quite compact, at least more compact than 3 radio buttons. Not sure I like it: very wordy and not sure it's that much clearer. My goal was to make this work more like a single choice about how to change the default of "show title". If you choose to check the box you get a second option to refine what "do not show" should mean.

*Edit* initially showed an earlier version of this design, updated the link to show the latest.

What if we used the working version, which is totally standard and looks like any other similar thing in core, THEN in a follow-up, discuss about improving it? I'd like to get things done for accessibility first. Usability is great, but we are overthinking the problem this time.

I updated IS to add the screenshots

See the previous comment

This sounds like a good way to move forward to me. I've re-queued the patch in #30 and am fine with the idea of moving this new pattern into a separate issue.

Rerolled #30

It's better with the file :)

Totally Agree @DuaelFr - you're totally teasing us though.. :)

Sorry, I don't know what happened twice :/

Yes, it's ok to start with the already existing pattern for how core handles this. I wasn't too happy with the other ideas yet either :)

So, according to #38 and #56, can we RTBC this issue ? Is there yet remaining tasks ? Writing a change record is it really necessary ?

Let's try to move it forward.
See #32, #38, #48 and #56.

Rerolled after #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page got commited (conflict solved interdiff attached)
I still hope it can go into 8.4 as the issue was RTBC before the alpha

Here's the straight reroll of the patch.

Here's the correct patch with correct status

Fixed the block module failures. Still having failures with SettingsTrayBlockFormTest

Finalized the fix for 8.5 (I can't run the tests locally so I hope the testbot is gonna be happy)

Fixed the test failure
+ Fixed an issue where the block title field in the settings tray was not shown when needed

Straight reroll of #74

From an WCAG accessibility viewpoint, I would also be looking for the 'completely remove title from output' configuration per block.

Having a heading rendered in some cases causes both heading structure issues (see image of H2 above H1), and generate superfluous headings which aren't helpful for users of assistive technologies (screen readers etc).

In some cases ARIA landmarks and HTML5 elements cover the need for menu/nav and search structure.

Any further dev/thoughts on this appreciated.

#80 the thing is that we currently only have an option to hide the title which is supposed to entirely remove it from the markup. But, some themes decided that for some blocks, like menu blocks, this could also mean to hide them visually to help users to navigate to these blocks by the heading jumping technique.
Be completing this issue, we could allow site builders to choose wisely which option they need.

Thanks @duaelfr and the others working on this! That would be a massive improvement!

this issue is really about giving authors finer control over all block titles

+1!

Re-rolled against 9.1.x. Kindly review a patch.

Rerolling

This should resolve the majority of failed test cases

Applied patch #96 and it works fine.There will be visually hidden option available in the block title.
Adding screenshots

Block configuration:

markup:

Dreditor goes "green" and I cannot see any drawbacks here ATM. Testing was without flaws but I would love to hear an opinion from somebody who is more into core block module, looking at it to make sure we do not create any bad follow ups. +1 for the work on this!

Resolving deprecation notice

@raman.b tested #101 and i confirm that all 3 options are working as expected, follow the screenshots.

UI - Select with the 3 options

Front end: Visually hidden

Front end: Hidden

One detail i just saw: Do we really need the - (dash + space) before the labels for Hidden and Visually hidden?

I tried both #89 and #93 on my 9.1 Drupal installation but they both cannot be applied. Since 9.2 is still in dev mode I prefer using 9.1.

"patches": {
    "drupal/core":{
        "Block Title Visually Hidden 9.1": "https://www.drupal.org/files/issues/2020-08-11/reroll-2614950-93.patch"
    }

Any idea what the problem could be?

1. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
   @@ -28,6 +28,19 @@
   +   * Indicates the block label (title) should hidden from end users.
   

   There's a missing "be" between should and hidden.

2. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
   @@ -28,6 +28,19 @@
   +   * @todo This value should be changed to "hidden" in Drupal 9.0.0 for
   +   *   consistency. See https://www.drupal.org/node/2863313.
   

   This @todo is referring to a Drupal version that has already released.

3. +++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
   @@ -28,6 +28,19 @@
   +   * Indicates the block label (title) should be visually-hidden from end users.
   

   Change "visually-hidden" to "visually hidden" and remove "from end users"

4. +++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
   @@ -171,10 +171,11 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
        $form['label_display'] = [
   -      '#type' => 'checkbox',
   +      '#type' => 'select',
          '#title' => $this->t('Display title'),
   -      '#default_value' => ($this->configuration['label_display'] === BlockPluginInterface::BLOCK_LABEL_VISIBLE),
   -      '#return_value' => BlockPluginInterface::BLOCK_LABEL_VISIBLE,
   +      '#description' => $this->t('How to display the block title.'),
   +      '#options' => $this->getLabelDisplayOptions(),
   +      '#default_value' => $this->configuration['label_display'],
        ];
   

   This presents a backwards compatibility issue for contrib/custom that may be altering this form element, it would be difficult to get a change such as this through all the core gates. If there were a way to accomplish this by *adding* a form element instead (even if the UX isn't as good), it's more likely to be accepted as a change to core. Otherwise, this would probably need to wait until D10

   An additional "make visually hidden" checkbox field that is only availavle when "visible" is selected may work. The value of that could conditionally add a .visually-hidden class. Not as elegant, but more likely to land.

5. +++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
   @@ -185,6 +186,20 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +      BlockPluginInterface::BLOCK_LABEL_HIDDEN => '- ' . $this->t('Hidden') . ' -',
   +      BlockPluginInterface::BLOCK_LABEL_VISUALLY_HIDDEN => '- ' . $this->t('Visually Hidden') . ' -',
   

   No dashes/spaces should be added here, particularly since these characters are used elsewhere to represent hierarchy, and there's no hierarchy to these options.

Addressed #105.1,2,3,5

Here is a re-roll of #78 for Drupal 8.9.x

Re-roll of the #107 patch for Drupal 9.3.x.
Comments in #105 still need to be addressed.

Re-roll of the patch in #109. Removed no longer necessary diff for

core/modules/block/tests/modules/block_test/config/install/block.block.test_block.yml

Rerolled patch in order to make it apply to 9.4.x version of Core.

There were (luckily) no conflicts to solve.

(edit: Forgot to attach the patch to this comment! as a result, the patch number is #114 but the comment will be #115, sorry)

Tried to apply #114 patch but the given 9.5.x-dev version is not compatible with 10.1.x-dev so i reroll the patch and updated also attaching the reroll file.

Try to fix patch #117 CCF errors.

I'm not sure why #107 attempted to reroll patch #78, despite later patches adding essential changes. It looks like there have since been 5 attempts to re-reroll the failed reroll of patch #78, even though it omits several desirable changes that have been made since.

#106 would have been the best choice as it addressed several concerns / problems that were spotted after #78. At this point, however, I recommend no more rerolls and instead creating a new patch entirely that is inspired by but not a reroll of 106. As I mentioned in #105.4, the solution is changing the configuration form in a way that breaks backwards compatibility in a manner that prevents it from being committable.

+++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
@@ -171,10 +171,11 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
     $form['label_display'] = [
-      '#type' => 'checkbox',
+      '#type' => 'select',
       '#title' => $this->t('Display title'),
-      '#default_value' => ($this->configuration['label_display'] === BlockPluginInterface::BLOCK_LABEL_VISIBLE),
-      '#return_value' => BlockPluginInterface::BLOCK_LABEL_VISIBLE,
+      '#description' => $this->t('How to display the block title.'),
+      '#options' => $this->getLabelDisplayOptions(),
+      '#default_value' => $this->configuration['label_display'],
     ];

Changing this from a checkbox to a select will break form_alters for any contrib/custom that uses it. If you instead add an element to the form to set visually hidden (AJAX could be used to only make this field available when 'Display Title' is true). The UX would be slightly worse, but this would not break form API usages AND many of the tests that had to be changed to support this new feature won't have to be because the 'label_display' would still work the same, just augmented by a newly introduced 'label_visually_hidden' option

I'd love to have this feature for our current client and I like @bnjmnm's suggestions in comment #120. Is this something being actively worked on?

Here's a reworked #106 based on comments from #105. Added a new configuration field label_display_type separate from the existing label_display checkbox that lets one select how one wants to hide the title. The hope would be that in a future version, label_display will just roll into the select box as the "visible" option, just like the patches prior. Name, label, and description could be better, open to suggestions.

Tests removed in this patch, I wasn't sure if they'd still be applicable (I also don't have a test-capable setup at the moment). Hopefully someone can pick up the baton from here.

Marking this as WCAG 2.4.6 https://www.w3.org/WAI/WCAG21/Understanding/headings-and-labels

Reroll for 10.2.x

Updated #125 for 10.3.x

Updated #125 for 10.3.x, take 2.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Here is a new patch updated for Drupal 11
This update is based on the latest patch (#127), which I have rebased and adapted for compatibility with the 11.x branch.

This looks like it needs review @guietc.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Tag cleanup

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The changes have been added as a patch file which is implemented in merge request 8596. The changes are working correctly on Drupal 11.3

I have rebased the merge request.

Currently, when label_display is falsy, label is empty. It should be that label always contains the label. This would allow a template to have code like this:

{% if label and not label_display %}
  {% do attributes.setAttribute('aria-label', label) %}
{% endif %}

Haven’t fully reviewed but tests are failing and CR is missing.

I have created a draft of a different approach in merge request 15613. What this change does is put the block title in an aria-label attribute when a label exists but it would not otherwise be displayed. I only did this change in the default template. If this approach is accepted, then the merge request can be updated to change this template in all themes and add tests.

Think summary should be updated if 2 approaches are being proposed.

That said if a new one is done then remaining tasks will have to be updated as this will have to go through the same checks.

The next step is to decide which approach to use.