Allow for Layout Builder sections to be given administrative labels

First pass at adding CSS to style the label.

Removed an unrelated edit and rolled the patch again.

Disregard #11 -- the file I removed actually is relevant! The patch in #10 is the correct one.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -358,6 +358,10 @@ display_variant.plugin:
   +  mapping:
   +    label:
   +      type: label
   +      label: 'Label'
   

   I was thinking this might get easily confused with the label of the layout plugin itself, but on second thought, this is very clearly part of the plugin settings, so it makes sense where and how it is. 👍

2. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -22,12 +22,18 @@ class LayoutSectionItemList extends FieldItemList implements SectionListInterfac
   +  /**
   +   * Numerically indexed array of field items.
   +   *
   +   * @var \Drupal\layout_builder\Plugin\Field\FieldType\LayoutSectionItem[]
   +   */
   +  protected $list = [];
   +
   

   I was going to ask why this was needed, since we get it from FieldItemList, but I see it's because of the type hint. So, 👍

3. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -60,6 +66,17 @@ public function getEntity() {
   +    // Loop through each section and make sure it is up-to-date.
   +    foreach ($this->list as $delta => $item) {
   +      $item->section = Section::fromArray($item->section->toArray());
   +    }
   

   I think this comment needs to be expanded a bit. Why would the sections be out of date?

4. +++ b/core/modules/layout_builder/src/Plugin/Layout/MultiWidthLayoutBase.php
   @@ -35,19 +36,14 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -    return $form;
   -  }
   -
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
   

   Not exactly sure what's going on here, but I assume it's just diff weirdness. 🤷‍♂️

5. +++ b/core/modules/layout_builder/src/Section.php
   @@ -98,7 +98,7 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
   -    return $this->layoutPluginManager()->createInstance($this->getLayoutId(), $this->getLayoutSettings());
   +    return $this->layoutPluginManager()->createInstance($this->getLayoutId(), $this->layoutSettings);
   

   This could use a comment. Why can't we call $this->getLayoutSettings() anymore? Will it not return the same data it previously did? If not, isn't that a backwards compatibility issue?

6. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -1219,10 +1257,10 @@ protected function assertCorrectLayouts() {
   -      'layout_builder/add/section/overrides/node.1/0/layout_onecol',
   +      'layout_builder/configure/section/overrides/node.1/0/layout_onecol',
          'layout_builder/configure/section/overrides/node.1/0/layout_twocol_section',
          'layout_builder/configure/section/overrides/node.1/0/layout_threecol_section',
   -      'layout_builder/add/section/overrides/node.1/0/layout_fourcol_section',
   +      'layout_builder/configure/section/overrides/node.1/0/layout_fourcol_section',
   

   Why did these change?

7. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -1236,6 +1274,7 @@ protected function assertCorrectLayouts() {
   +      $assert_session->linkByHrefNotExists("layout_builder/configure/section/overrides/node.1/0/$unexpected_layout");
   

   And here? I must be missing something.

8. +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
   @@ -120,7 +120,7 @@ public function testAccess($expected, $is_enabled, array $section_data, array $p
   +      new Section('layout_onecol', [], [
   

   How about this? It's not clear why this changed...

9. +++ b/core/modules/layout_builder/tests/src/Kernel/SectionListTraitTest.php
   @@ -47,7 +48,9 @@ class TestSectionList implements SectionListInterface {
   +    foreach ($sections as $section) {
   +      $this->sections[] = Section::fromArray($section->toArray());
   +    }
   

   This could also use a comment, maybe.

I gave this a quick manual test. It worked beautifully; the sections can indeed be assigned administrative labels, which are exposed as the title of the off-canvas area when configuring or deleting the sections. Well done! Code is clean and well-tested (par for the course with a @tim.plunkett patch), and we are accounting for the update path. I don't think this needs anything else.

Screenshots attached from my testing (on a clean new install).

Forgot to move to RTBC.

Ok, running this through WebchickTestCase™!

First question: Can we smoosh the layout selection + the admin label into the same form so it's one step instead of two? We ideally want adding a section to be as quick and painless as possible; this new second step adds friction. (Oh, I see... this happens only for one-col; the other ones already have an extra configuration step. Well then.)

Second question: Why do I not see the section label I just created on the page anywhere? This left me fairly disoriented, especially with a one-column section where it's lots of equally-sized boxes stacked on top of each other. Would expect to see the "Configure" link say "Configure Top Header Thingy" or whatever. This would also help with targeting drag-and-drop.

(Oh wait. After dragging a block in, the labels suddenly appeared? Some caching issue? It's visible in @phenaproxima's screenshot at https://www.drupal.org/files/issues/2019-08-16/3073872-label-new-section... nevertheless, so don't think it's just me.)

At least in Firefox, there is also some weird styling on optgroups on the move drop-down. Probably a pre-existing condition, tho:

Elsewise the patch itself looks very straight-forward, has ample test coverage, and so on. A nice accessibility and usability improvement both, so \o/ Feels like we need to address that weird label disappearing issue, however.

Ok, spun off #3076153: Off-canvas dialog <optgroup> styling in Firefox is illegible for the third point.

On the second point, which is the only thing we need to fix here, I figured out through talking with @tim.plunkett that this is not a caching issue; rather, those labels are only visible when in keyboard navigation mode (which I also was testing).

We discussed and agreed that showing them is better though, because otherwise it gets confusing, esp. if you are creating multiple similar-looking sections at a time, as to which is which.

Since this is a new feature that not all sites will be using right away, what we discussed was conditional logic like (blatantly stolen from @tim.plunkett :D):

$text = "Configure";
if (there is an admin label) {
 $text .= " $label";
}
else {
  $text .= " <span class="visually-hidden">section $delta</span>"; 
}

This way the labels are exposed to screen readers/keyboard users no matter what, but folks who don't need/want to use section labels aren't inundated with a bunch of "Section 1, 2, 3 [...]" in the UI.

Moar feedback from the UX meeting today. :)

HEAD:

This patch:

This won't fly, because without the extra input as to what you're configuring, it's not clear what that configure link actually refers to. Additionally, the "X" icon seems to be associated with Configure, vs. its own operation. :O

This is particularly apparent when a section has blocks in it:

Absent of the word "section," a sensible person would think that these were links to configure / delete the content within the section, since that's what's actually visible.

All of that makes sense, but then seems to make the proposed resolution not quite right... instead we probably ought to instead just always display the section name even if it's an auto-generated delta (which has the side benefit of making the UX consistent between screen reader users and visual users).

In other words:

$text = "Configure section $label";

...across the board.

(Obviously with translation and escaping and such :P)

Yeah, I think the first one is sufficient. As @yoroy always used to say, use fewer words. :) The previous concern was the standalone "Configure" getting confused with blocks, but I don't see that happening with the labels exposed.

Seems legit.

Great stuff! Thanks a lot for the extra tweaking!

Committed and pushed to 8.8.x, woohoo! Also crediting folks on the UX call, who contributed to the solution outlined here.

Also, probably something to mention in the release highlights reel. This is a nice feature for accessibility and usability both.

Now that we've added the ability to add Administrative labels - how do I get rid of them :/ I find them super belittling to the end user.