Layout Builder should make it easier to modify the default layout for an entity type when viewing an entity

Assigning to @DyanneNova for input

reassigning to work on MWDS2018 :D

It think it would great if we could solve this is way where it works for bundles that don't have the layout builder enabled.

In general this been problem for a while where it is hard to find what controls the "display" on fieldable entities. If a site has articles using layout builder and basic page using manage display for fields if use has access to both you should be able to easily find the way to effect the display in block cases.

once #2914486: Add granular permissions to the Layout Builder is committed we could easily have user that can only change defaults and not overrides.

Worked on the first part of this today - moving the code to create local tasks from the Derivative into the SectionStorage classes.

After LOTS of pair programming with @sugaroverflow, we decided to move the (awesome) refactoring above to a new issue.
This new patch skips over that refactor for now.
No interdiff as it is a completely new patch.

The current "Layout" local task takes the user into the flow for overriding the layout for that entity.
This has been relabeled "Customize this page" for now.

The new "Layout" local task will take you to one of two places:
If the entity has no overrides yet, it will take you to the "Manage Layout" page for that entity view display.
If the entity has overrides, it will take you to the Layout Builder for that override.

This is what we came up with at the sprint, but is still rather confusing. More work must be done here to determine what the actual desired flow should be.

Docker hub was down due to maintenance so the patch failed. We retested and it passed ✌️

This need some work from @DyanneNova, not ready for wider review.

I like the general idea here.

I agree with #6 that maybe first we should look to solve the more generic issue of making it easier to get to the “manage display” (or even manage fields) for any Entity, not just within the context of Layout Builder.

I’m also not a fan of the naming proposed in the original summary, #8 makes some good progress on that but we really need to prioritise the experience of the frontend content editor who will only ever manage the “override layout”, as coming from the perspective of a large organisation many more people will manage overrides than will ever use the Manage Display Layout Builder. The people using the Manage Display Layout Builder experience will typically be more advanced users who understand the difference. So I’d be very hesitant with any terms such as “override” that might make the workflow of content editors more confusing.

Maybe the ideas in this issue are something to think about.

Another related issue, maybe there’s some cross-over here.

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -198,12 +198,12 @@ public function testLayoutBuilderUi() {
-    $assert_session->pageTextContains('Placeholder for the "Extra label" field');
+    $assert_session->pageTextContains('Extra, Extra read all about it.');

I couldn't figure out this test change at first. But I see that this patch is also changing where the user is redirected when the user reverts the override.

This seem unrelated to the current issue.

That change is needed for the new flow of things. However we're not set on the given proposal yet...

Reroll now that #2995071: Refactor LayoutBuilderLocalTaskDeriver to delegate local tasks to plugins went in.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -163,13 +173,20 @@ public function buildRoutes(RouteCollection $collection) {
+      $local_tasks["layout_builder.overrides.$entity_type_id.revert"] = $base_plugin_definition + [
+        'route_name' => "layout_builder.overrides.$entity_type_id.revert",

I think "revert" here in both these lines should actually be "redirect". if this is suppose to use the route that is added in this patch.

Right now this is actually just be overwritten by last assignment to $local_tasks array in this block.

The key layout_builder.overrides.$entity_type_id.revert is being used twice.

Right now there is not test for local task and where it redirects so that is why the tests pass.

Fixed #22

re #8

The new "Layout" local task will take you to one of two places:
If the entity has no overrides yet, it will take you to the "Manage Layout" page for that entity view display.
If the entity has overrides, it will take you to the Layout Builder for that override.

I think for people having the ability to see both the defaults and the overrides once you go to an existing entity that has an override will be confusing because "Layout" and "Customize this page" this will take them to the same place. But I could see how that might be confusing. and they think it is not the same place and trying to figure out the difference

It seems like for people with both permissions
entity with no override.
Layout -> "[bundle_label] layout]"

entity with override
remove "[bundle_label] layout]" because the bundle level has not effect.
or I guess a page that explains this with the option of revering to defaults.

1. \Drupal\Tests\layout_builder\Functional\LayoutDisplayTest::testMultipleViewModes() needed to be updated to use "Customize this page" link. It was only passing because the redirect was wrong.
2. Added new test to confirm the redirect works correctly with and without an override.

Leaning towards agreeing with @tedbow in #24 that having the "Layout" local task do different things depending on the context could be very confusing and possibly result the user making changes to a layout where the user thought they were editing the customised layout but were actually editing the bundle layout (and probably vies versa).

#27 seemed to be drupalci error. Retested and passed

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -228,7 +245,8 @@ protected function getEntityTypes() {
-    return LayoutBuilderEntityViewDisplay::collectRenderDisplay($this->getEntity(), 'full');
+    $section_list = LayoutBuilderEntityViewDisplay::collectRenderDisplay($this->getEntity(), 'full');
+    return $this->sectionStorageManager->loadEmpty('defaults')->setSectionList($section_list);

Whoa, this change makes sense but is kinda worrisome. How were we getting away with this in HEAD?

Also curious how this would look once #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides lands

Needs reroll and or rethinking of approach.

Re #31

Needs reroll and or rethinking of approach.

I'm going with the "and" option - I'm going to look into alternate approaches and will summarize them here. I also think a working patch will aid that process, so here's a reroll of #26

I created visualizations that map out some possible approaches, including comments on some of the choices.
I intentionally included options unlikely to be implemented. If it seemed like something that might aid the decision process, I put it in there. I'm particularly interested in which options strike people as being not-at-all feasible, as that can curtail some of the choice paralysis in selecting an approach.

One Layout-related link option:

Two Layout-related links option:

The draw.io files are also attached if there is interest in expanding on what I've already provided. The .txt file extension needs to be changed to .xml to be opened in draw.io as I can't upload an .xml file.

On the ux call on Jan 22, we discussed a plan for how this can be handled. Here are the mockups that were reviewed, with the updates based on feedback from the call:

Just to note, implementing this will be blocked by #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features. I am actively working on that issue.

The blocker landed.

Here's a rough first attempt on this patch.
One thing we need is the SVGs for those two icons that accompany the message. Leaving assigned to @DyanneNova for that.

I've attached the icon files in a zip because svgs can't be uploaded here.

Thanks, @DyanneNova! Unassigning.

Still needs tests and styling, but here is a patch which includes the SVGs as well.

Fixed a couple of small bugs and added a couple of test assertions.

However, I don't know how to approach the styling for this; will need to discuss with @DyanneNova.

+++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
@@ -64,10 +77,43 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
+      '#type' => 'html_tag',
+      '#tag' => 'div',
...
+      '#value' => $this->t('You are editing the layout template for all @bundle @plural_label.', [

Use '#type' => 'container' and s/'#value'/'message'

Added some styling after quick discussion with @DyanneNova. Screenshots attached of how it looks in Bartik.

One thing I noticed is that, with the RTL styles, the period in the sentence comes at the left, not the right. That seems really weird, but I guess it's expected in RTL displays? I'm not sure. Anyway, let the screenshots be your guide...

Forgot to set it to NR!

Oh, and I also forgot the interdiff.

This one fully addresses @tim.plunkett's feedback in #44. (Patch #46 only partially addresses it.)

1. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -64,10 +77,44 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +  private function buildMessage(LayoutEntityDisplayInterface $entity) {
   
   +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -107,9 +109,46 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +  private function buildMessage(EntityInterface $entity, OverridesSectionStorageInterface $section_storage) {
   

   Nit: These don't actually need to be private, might as well switch it back to protected :\

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -107,9 +109,46 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +          'layout-builder__message',
   +          'layout-builder__message--overrides',
   

   These look correct. Yay BEM, I guess!

No real substantive feedback, but then again, I wrote the original version.
Assigning to Emilie to weigh in on the CSS and any other styling tweaks that might be needed.

Also making sure those of us who discussed at MWDS get credit for their input.

Changed the methods to protected.

Added a missing </a>.

On @bnjmnm's advice, I tried adding layout_builder_test_css_transitions to the modules enabled for TestMultiWidthLayoutTest. Let's see if that helps.

The lighter green of the icon will be more discernible with a lighter yellow, there isn't enough contrast right now. Here's a color value that will work better: #f7efcf

Fixed. Adding screenshots to the IS too.

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -78,8 +78,8 @@
+  background-color: #f7efcf;
...
-  background-color: rgb(248, 236, 185);

For the record, the new value is equivalent to rgb(247, 239, 207). Not sure if you wanted to keep it that way.

This looks good to me!

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -75,6 +75,35 @@
+  border: 1px solid rgb(180, 160, 46);

I didn't realize that this would become only the 3rd usage of rgb() in core, let's switch this one to hex too. #b4a02e, looks like?

Fixed!

Thanks!

Unrelated (known random) test failure in Media Library.

Are all the UX meeting contributors credited here as well?

They are now! Crediting from UX 1/22 meeting: https://www.youtube.com/watch?v=QbrxHvjQOCU

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -107,9 +109,46 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +      '#type' => 'container',
   

   any reason not to use '#theme' => 'messages' here so it looks consistent with any front-end theme?

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -107,9 +109,46 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +        '#markup' => $this->t('You are editing the layout for this @bundle @singular_label. <a href=":link">Edit the template for all @bundle @plural_label instead.</a>', [
   ...
   +          ':link' => $section_storage->getDefaultSectionStorage()->getLayoutBuilderUrl()->toString(),
   

   should we be checking they have access to edit the default before showing the link?

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/TestMultiWidthLayoutsTest.php
   @@ -20,6 +20,7 @@ class TestMultiWidthLayoutsTest extends WebDriverTestBase {
   +    'layout_builder_test_css_transitions',
   

   what is this change for? nevermind saw #57

I was reviewing this and I found out #72.1 as well. The current styles are also specific to Umami and don't look great in Bartik for example.

#72.2

Without #2914486: Add granular permissions to the Layout Builder, any user that can see this message has permissions to edit the defaults. However, until #3003610: Remove block.module dependency from Layout Builder is resolved, the user also needs to have permission to manage the entity view display.

@phenaproxima suggested in chat that we check access to the URL directly, instead of permission checks, in order to future-proof

Let's see how the URL access check works. I'm not sure why the interdiff shows CSS changes; maybe I moved stuff around. Whatevs!

This should fix the tests. Looks like the data provider pattern will not work properly in this case, so I just added a new test method to LayoutBuilderTest.

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -107,9 +109,57 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +    if ($defaults_link->access($this->currentUser())) {
   +      $message = 'You are editing the layout for this @bundle @singular_label. <a href=":link">Edit the template for all @bundle @plural_label instead.</a>';
   +    }
   +    else {
   +      $message = 'You are editing the layout for this @bundle @singular_label.';
   +    }
   ...
   +        '#markup' => $this->t($message, [
   +          '@bundle' => $bundle_info[$entity->bundle()]['label'],
   +          '@singular_label' => $entity_type->getSingularLabel(),
   +          '@plural_label' => $entity_type->getPluralLabel(),
   +          ':link' => $section_storage->getDefaultSectionStorage()->getLayoutBuilderUrl()->toString(),
   +        ]),
   

   AFAIK this does not work.
   The string needs to be within the t() for the parsing.

   I'd suggest making a local variable for the args, and then doing the t() directly in the if/else.
   Also you can add :link to the $args in the if, since it's not needed in the else.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -63,6 +64,30 @@ protected function setUp() {
   +    $this->assertInstanceOf(LayoutBuilderEntityViewDisplay::class, $display);
   ...
   +    $this->assertSame(SAVED_UPDATED, $display->save());
   

   These assertion seems superfluous to me. We need to be able to assume this works here, IMO. Just create and save it all at once

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -63,6 +64,30 @@ protected function setUp() {
   +    // @todo This should not be necessary.
   +    $this->container->get('entity_field.manager')->clearCachedFieldDefinitions();
   

   Are we sure this is necessary? Can we try it without?

Can you post a FAIL patch as well with the next iteration? Thanks!

All fixed!

Sorry, forgot the interdiff!

@bnjmnm explained the test failure to me. To quote him (and I added this comment in the test):

The test checks coordinates before and after the mouse pointer is used and compares the Y position. Because`.layout-builder__message` wraps (and changes the Y position in the test) when the off canvas dialog opens, the "before" coordinates must be collected _after_ off canvas opens.

So that's why I'm changing LayoutBuilderDisableInteractionsTest in this patch.

I discussed the styling issue from #72 and #73 with lauriii and we agreed that the current approach is fine. This message should not inherit theme styling for current messages because themes frequently use their own icons to represent message status. In our case we are using the icons as a visual affordance for the user to more easily distinguish between the default and the override. The current styling isn't using Umami colors, it's intended for any of the core themes, and hopefully will work well enough generically for other themes.

Great, thanks everyone! Looking forward to getting this one in

It seems like the current colors don't have sufficient contrast to meet our accessibility core gates. Umami default link color with the current background doesn't fulfill WCAG AA. Bartik default link color with the current background color fulfills WCAG AA, but the color used for :hover doesn't fulfill WCAG AA.

I'm also wondering if we should add a hidden title describing that the element is an info message like we have on status messages. Adding the the accessibility tag since we are adding a new status message like element.

Thank you @andrewmacpherson for pointing out in Slack that in WCAG 2.1, the contrast should be applied not only to the text but to all non-text elements (reference). Therefore since we are going to have to make some changes anyway, we might want to increase the contrast between the border and the background, as well as between the icon and the background.

@lauriii asked in Slack if I would look at this..

I could use your feedback whether we should include visually hidden title for the message that is being added here:

Can you clarify:

1. What message it is for, and undee what circumstances it appears.
2. Where it appears. Does the position vary?
3. Whether the message is in the HTML source, or added dynamically after page load.
4. What visually-hidden text you propose.
5. Crucially, what do you think the message is lacking, to warrant extra visually-hidden text?

Edit: also, by title, do you mean heading?

@andrewmacpherson

I hope this provides more context to my request:

1. The message appears always when layout builder is rendered. There's a different message for the edit default layout page and the edit layout override page.
2. It is always rendered right before the layout builder. It doesn't vary.
3. It is in the HTML source.
4. I didn't have any specific text in mind.
5. I was only considering this because we chose not to use the pre-existing message component and this is something that is currently missing from that component. The problem could be the fact that it looks like a message, but when you are unable to see the page visually, you would think it's just a block of text on the page.
6. Yes, I meant heading.

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -83,6 +83,35 @@
+}
+[dir="rtl"] .layout-builder__message:before {

Another missing CSS newline. :)

#92: According to our CSS formatting guidelines, by default, we are not supposed to add newlines between CSS rules. In this case, these selectors are related to each other since they are modifying the same element. Instead of adding newline between these rules, we probably should remove the newlines between all the other selectors we are adding as well.

Yeah I'm wrong about #92; disregard.

To get sufficient contrast, we would have to change the background color roughly to #fbf8e9, the border color to #7a6c1f and the icon color #4d7b37. We could consider standardizing the link styles inside the message rather than relying on custom styles. If we want to rely on custom styles, maybe it would be safest for us to use a very light background instead.

#05840B for the icon and link on a #fffae7 background has enough contrast if we agree to standardize the link colors for this message (which I do think would be better overall). The border shouldn't need higher contrast because it isn't an affordance, just a design element.

Thank you @DyanneNova! Before we can start implementing this, we still need colors/designs for hover, focus, and active states.

Let's use underline for hover and focus states. We can use #0476CA for the active state, which has appropriate contrast against the background and is clearly distinguishable from the green.

Implemented the color changes and it looks following:

Just a reroll. I think there were some uneeded changes in LayoutBuilderDisableInteractionsTest. That file changes recently. That test still passes.

I check to make the messages show up. they look good.

Can we get Bartik screenshots as well?

The message for User layouts is kind of strange "You are editing the layout for this User user" . Perhaps if the bundle and singular label match, then only one needs to be shown?

Will continue reviewing, but thought this may be something someone wants to work on without waiting for the whole review.

Added Bartik screenshots to IS.

#102
I agree 100%. We did this for permissions too, by checking $entity_type->hasKey('bundle')

#103
Thanks!

Realized I was mixing plural and singular strings without aa pattern. nvmd.

+1 to #102, but still a little awkward when the bundle contains the entity type, like "Default comments". What if we just ditched the entity label altogether?

Implemented #102, with test coverage

Reviewed the code and I can RTBC this once the CSS from @lauriii gets +1'd

+1 to the CSS changes.

The key point of the accessibility review was the colors and they have been fixed so I don't think we should expect any further input from the accessibility team on this.

+1 for RTBC, looks all good 🧚‍♂️✨

Spent some time reviewing this with @xjm. Our initial impression is that the extra message styling here looks (to a layperson) as a mistakenly styled standard warning message. Here's a screenshot with both, for comparison. You can see that the default message is using a sans-serif font, different yellow, etc. The Layout Builder message kinda looks like it's coming from Umami. (I realize it's not, and the colours were actually carefully chosen to be WCAG compliant when including the link.)

@DyanneNova and @tim.plunkett explained the rationale for this, which is:

1. We don't want to use standard MessengerInterface stuff, because this important info might get lost in the shuffle if there's other info/warnings being displayed. See https://www.drupal.org/node/add/project-issue/drupal for a great example of "drupal_set_message() overload." :P Hence, it needs to be located in the area that it's in, because it's important info we want to make sure people actually see (makes sense).
2. We don't want to use stock "warning" style, because it's not actually a warning, per se. The user hasn't done anything wrong, nothing needs to be corrected. (Makes sense, also. However, the "similar enough" yellow colour still makes it look like a warning, at least to my non-designer eye).
3. We also don't want to use "status" type, because once again the user hasn't done anything that we need to signal is "Ok / Succeeded." (Though could we borrow the styling and just change the icon?)
4. An ideal situation would be an "info" kind of box, because that's exactly what this is. Info. However, somewhere along the way we lost the "info" type that I swear we used to have in like Drupal 6 or something? And adding it would be an API change, which we can't do during beta. Touché.
5. Complicating matters, because Layout Builder isn't stable yet, we can't adjust message styling in other themes. So getting the same message to look good in both Bartik/Seven and Umami is going to be... tricky (recommendation from @xjm was to worry about Bartik for now, since Umami is "perma-experimental").

TL;DR, we need that warning to look like a standard Bartik/Seven message w/ a different icon (this is what the UX team signed off on). Colours are negotiable, and happy to defer that decision to @DyanneNova's "design eye."

Not yet addressing #111, but this fixes a constructor change that xjm requested.
Leaving NW.

For reference: #375928: Add type "info" to Drupal Messenger service (Wow, 6-digit node iD!)

#111

1) Thankfully we can use #theme => status_messages directly (as opposed to the messenger service itself or even #type => status_messages, this sidesteps this problem

2) Agreed on not using warning. Plus, there is an aria-label for that which literally says "Warning" and we definitely don't want it.

This attempts to address #111 in the least controversial way: By using the status message rendering but with our own icons (#111.3)
This allows room for future improvement with more targeted CSS (#111.5) as well as potentially switching from status to info if #375928: Add type "info" to Drupal Messenger service lands (#111.4)

The screenshots in #114 look great to me, FWIW.

Looks good in Bartik. Let's make this work in Umami as well. ✌️

Classy (and therefore every other stable core theme) uses .messages for the background image.
Specifically in this case .messages--status
Umami uses .messages--status .messages__content
That's a bug in Umami, no?

Added a workaround to fix #116 for now, #3043228: Add Umami-specific styling for Layout Builder messages can fix it correctly once LB is stable.

Nope, wrong patch and wrong interdiff. NOW its right

This looks good to me. I think this is ready to go!

+1 to RTBC. I think it's reasonable to move fixing the Umami design to time after layout builder has been marked stable because there isn't any good way to fix it prior to that.

Great! I ran into the lack of this bug while testing the Quick Edit one, so nice to see this one land.

This meets the UX team requirements for the message, gets rid of the inconsistent styling noted in #111, and also fixes the blatant hilarity in #116. :D

Committed and pushed to 8.8.x, cherry-picked to 8.7.x. Thanks, all!

Is it possible to disable this message? I could hide it with CSS but if there is a legit way via some GUI setting that would be preferred.

I'd also like to point out that the message is a bit unclear. Currently it says (Layout Page is the name of my content type):

"You are editing the layout for this Layout Page content item. Edit the template for all Layout Page content items instead."

I think it should say:

"You are editing the layout for this Layout Page content item. Edit the default layout for new Layout Page content items instead."

This is more clear about what effect modifying the default layout will have on existing content. Which is nothing. But you wouldn't know that until you try it, which can be scary when you already have layouts configured for many nodes. I realize this ticket is closed so I may open another ticket for this.

This is more clear about what effect modifying the default layout will have on existing content. Which is nothing.

This isn't correct, modifying the default layout will impact all existing content that is not overriding that layout, and this is a use case covered in automated tests. If this is not what you're experiencing, that would warrant an issue, and the issue can be best addressed if it includes steps of how to reproduce what you're encountering on a fresh Drupal install.