Problem/Motivation

The Layout Builder UI currently relies on mouse-driven drag-and-drop to move around blocks.
Once #2956204: Allow Layout Builder sections to be reordered is finished, it will also be used for sections.

The designs include another keyboard-driven mechanism for these reordering actions.

Proposed resolution

Current proposed solution:

  1. Add a "Move" contextual link
  2. Clicking this opens a form in the off-canvas dialog(same as "Configure" and other actions)
  3. The form has a "Region" dropdown which allows moving to another region
  4. The form also has a blocks draggable table
  5. The blocks in the table are update for when the region changes

Move block form
Demo video: https://youtu.be/z3TlnuVSFqQ
The demo video also has #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys and #2938132: Ship layouts that make sense with Layout Builder's concept of sections applied

Previous proposed solution:

Previous/Next Links

Each block would have 2 links "Previous" and "Next"(probably replaced with arrow icons)
Clicking these links would move a block through the regions and sections of the layout.

Latest patch for this version: https://www.drupal.org/files/issues/2018-10-19/2995689-13.patch in #13
Video demo: https://youtu.be/5dHSoEPOgq0

4 way directional links

Each block may have up to 4 direction links(up/down/left/right)
The links will only be shown if the block can move in that direction. For instance if the block is region with no regions to the left or right these links will not show up.
The direction links are determined by the currently available regions. For instance if the layout is on small screen and the regions that were left and right are now up and down the links will be updated to remove left and right links.

Latest patch: https://www.drupal.org/files/issues/2018-10-26/2995689-22.patch #22
video demo: https://youtu.be/d63oIeKpxZA

Target method(limit other links)

Each block will have a contextual link "Move block"
Once "Move block" is clicked every possible place the block to could be place will have a "Place Block" link.
When the user clicks "Place Block" the block is moved to that location and all "Place block" links are removed until next "Move block" link is clicked.
Other links like "add block" will removed until the block is placed

Latest patch: https://www.drupal.org/files/issues/2018-10-26/2995689-26.patch #26
Demo video: https://youtu.be/41OfUEz-4wY

Target method(don't limit link)

Same as above but other links are not removed. If the user clicks "Move block" but before clicking a "Place block" link does another action like "Configure block" or "Add block" the "Place block" links are removed.

Latest patch: https://www.drupal.org/files/issues/2018-12-04/2995689-46.patch
Demo video: @todo @tedbow will make demo video

Remaining tasks

TBD

User interface changes

Yes

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#132 2995689-132.patch31.92 KBbnjmnm
#132 interdiff_130-132.txt4.53 KBbnjmnm
#130 2995689-130.patch31.93 KBbnjmnm
#130 interdiff_128-130.txt3.61 KBbnjmnm
#128 2995689-128.patch31.82 KBbnjmnm
#128 interdiff_112-128.txt13.38 KBbnjmnm
#128 section-and-region-names.png58.57 KBbnjmnm
#116 Edit_layout_for_Article_1___Site-Install.png50.39 KBxjm
#112 2995689-112.patch24.95 KBbnjmnm
#112 interdiff_100-112.txt635 bytesbnjmnm
#110 2995689-110.patch25.19 KBbnjmnm
#110 interdiff_108-110.txt722 bytesbnjmnm
#109 106-11.png40.35 KBbnjmnm
#108 2995689-108.patch25.15 KBbnjmnm
#108 interdiff_104-108.txt21.27 KBbnjmnm
#107 Screen Shot 2019-03-19 at 11.36.42.png36.99 KBlauriii
#104 2995689-104.patch24.4 KBbnjmnm
#104 interdiff_97-104.txt2.53 KBbnjmnm
#104 move-block-IE.png101.24 KBbnjmnm
#97 Screen Shot 2019-03-07 at 16.17.13.png15.12 KBlauriii
#97 interdiff.txt2.3 KBlauriii
#97 2995689-96-reroll.patch24.93 KBlauriii
#97 2995689-97.patch24.4 KBlauriii
#96 interdiff-95-96.txt674 bytestedbow
#96 2995689-96.patch30.36 KBtedbow
#95 2995689-95.patch29.65 KBbendeguz.csirmaz
#95 interdiff-2995689-92-95.txt2.85 KBbendeguz.csirmaz
#94 Screen Shot 2019-03-06 at 16.41.51.png6.1 KBlauriii
#92 2995689-92.patch29.59 KBbnjmnm
#92 interdiff_88-92.txt28.13 KBbnjmnm
#88 interdiff_85-88.txt35.71 KBbnjmnm
#88 2995689-88.patch55.73 KBbnjmnm
#85 2995689-85.patch26.53 KBtedbow
#85 interdiff-83-85.txt1.62 KBtedbow
#83 2995689-83.patch26.39 KBtedbow
#83 interdiff-77-83.txt1.38 KBtedbow
#77 2995689-77-reroll.patch26.23 KBbnjmnm
#75 2995689-75.patch26.18 KBtedbow
#75 interdiff-71-75.txt3.19 KBtedbow
#74 no-blocks-label.png44.27 KBbnjmnm
#74 blocks-label-default.png47.84 KBbnjmnm
#74 drag-then-show-row-weights.png38.58 KBbnjmnm
#73 2995689-73-reroll.patch25.95 KBbnjmnm
#73 interdiff_71-73.txt862 bytesbnjmnm
#71 2995689-71.patch25.95 KBtedbow
#71 interdiff-70-71.txt3.28 KBtedbow
#70 2995689-70.patch26.83 KBtedbow
#70 interdiff-64-70.txt921 bytestedbow
#64 2995689-64.patch25.62 KBtedbow
#64 interdiff-63-64.txt1.8 KBtedbow
#63 2995689-63.patch25.44 KBtedbow
#63 interdiff-61-63.txt918 bytestedbow
#61 2995689-61.patch25.39 KBtedbow
#61 interdiff-59_reroll-61.txt5.08 KBtedbow
#59 2995689-59-with-test-2995689-fix.patch50.17 KBtedbow
#59 2995689-59-do-not-test.patch24.83 KBtedbow
#59 interdiff-58-59.txt2.19 KBtedbow
#58 2995689-58-with-test-2995689-fix.patch50.18 KBtedbow
#58 2995689-58-do-not-test.patch24.84 KBtedbow
#58 interdiff-57-58.txt9.8 KBtedbow
#57 2995689-57.patch20.47 KBtedbow
#57 interdiff-56-57.txt6.75 KBtedbow
#56 2995689-56-form.patch14.5 KBtedbow
#56 interdiff-52-56.txt3.85 KBtedbow
#56 highlighted_icon_map.png31.34 KBtedbow
#52 2995689-52-form.patch11.43 KBtedbow
#52 interdiff-50-52.txt1.18 KBtedbow
#51 2995689-51-form-2938132-2725259-do-not-test.patch37.87 KBtedbow
#51 move_block_form.png36.91 KBtedbow
#50 2995689-50-form.patch11.37 KBtedbow
#50 interdiff-49-50.txt6.81 KBtedbow
#49 2995689-49-form.patch10.53 KBtedbow
#48 layout-builder-move-block-region-and-weight-in-dialog.jpg685.55 KBandrewmacpherson
#46 2995689-46.patch19.25 KBtedbow
#46 interdiff-43-46.txt1.36 KBtedbow
#43 2995689-43.patch17.89 KBtedbow
#43 interdiff-42-43.txt1.07 KBtedbow
#42 2995689-42.patch18.06 KBtedbow
#42 interdiff-26-42.txt15.42 KBtedbow
#26 2995689-26.patch13.55 KBtedbow
#23 2995689-22.patch26.11 KBtedbow
#23 interdiff-20-22.txt11.67 KBtedbow
#20 2995689-20.patch23.91 KBtedbow
#20 interdiff-2-20.txt17.16 KBtedbow
#19 firefox-menu-cut-copy-paste-on-one-line-2995689.png20.48 KBandrewmacpherson
#13 2995689-13.patch31.05 KBtedbow
#13 interdiff-12-13.txt12.66 KBtedbow
#12 2995689-12.patch18.39 KBtedbow
#12 interdiff-8-12.txt8.97 KBtedbow
#8 2995689-8.patch15.25 KBtedbow
#8 interdiff-2-8.txt26.02 KBtedbow
#2 2995689-2.patch18.8 KBtedbow
Add - Page - Example Content - Toolbar - 1.2,2.png196.84 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tedbow’s picture

Here is what the current patch does

  1. Adds previous and next links to each block in the layout builder.
  2. These links move the block across regions and sections.
  3. They can be used via mouse or keyboard navigation
  4. Every move saves the position
  5. Changes the drag and drop position saving to use this same function as this to save position
  6. Focus is returned to the link when layout is rebuilt. Allowing quicking moving the block across regions and sections by continually hitting enter.

The javascript is probably rough right now but wanted to get the idea out there to see if this a good way to solve the problem.

Here is little video demo: https://youtu.be/5dHSoEPOgq0

A couple other things a noticed about keyboard navigation that aren't directly related to drag and drop:

  1. The navigation should be simpler after #2973921: Interactive controls inside preview block in the Layout Builder form should be disabled. because we would take forms out inside these blocks out of the tab order.
  2. When you add a new section keyboard focus should be set to the first "Add Block" of the section.
  3. When you configure or delete block we should but keyboard focus somewhere so you don't have to start from the top again.

I don't know if we should expand this issue to cover keyboard navigation in general or just stick to moving blocks.

I looked through #2920006: Research accessibility of drag-and-drop grid interfaces. I didn't see anything that was ready for us to right away.

I think doing the up, down, left and right links would very hard because it is very difficult or impossible to know if regions are next to each other or below. This will change at different breakpoints and themes could override this in the CSS. Also will it always be the case that blocks in a region are stacked vertically? Could they sometimes be next to each horizontally? Would this change at different breakpoints?

Also I think having just previous and next might actually be less clicks for keyboard navigation. With this patch clicking next continually will take a block all the way from the top to bottom across multiple sections with multiple regions.

If we up, down, left and right links if you wanted to navigate the whole page you have switch/tab between different links.

Status: Needs review » Needs work

The last submitted patch, 2: 2995689-2.patch, failed testing. View results

andrewmacpherson’s picture

Title: Add a way to interact with Layout Builder without drag-and-drop » Add a way to interact with Layout Builder without a pointer device.

Tweaking the title. It's not about a mouse as such, or even about the drag operation.

andrewmacpherson’s picture

Title: Add a way to interact with Layout Builder without a pointer device. » Add way to interact with Layout Builder without a pointer device.

It's also not just about keyboard controls, though they are a huge part of it. Speech control is also something we can address.

Feel free to scope keyboard and speech control as separate issues if you prefer.

andrewmacpherson’s picture

Title: Add way to interact with Layout Builder without a pointer device. » Add ways to interact with Layout Builder without a pointer device.
tedbow’s picture

Assigned: Unassigned » tedbow
  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -85,3 +85,7 @@
    +.block-layout-builder.updating {
    +  opacity: 0.5;
    +}
    

    @todo What visual clue should we give that block is being moved.

    Once the ajax request comes back this will be gone.

  2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
    @@ -210,6 +210,12 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
    +          if (isset($build[$region][$uuid]['content'])) {
    +            $build[$region][$uuid]['content'] = [
    +              'layout_builder_reorder' => $this->createLayoutBuilderReorderNavigation(),
    +              'block' => $build[$region][$uuid]['content'],
    

    Need to fix this because will interfere with block rendering. It causing the test fail because it stops the label for Views blocks.

    Probably will move the whole block render array down a level so they can on the same level as the navigation links.

  3. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
    @@ -337,4 +343,36 @@ public function cancelLayout(SectionStorageInterface $section_storage) {
    +        '#type' => 'html_tag',
    +        '#tag' => 'a',
    +        '#attributes' => [
    +          'href' => '#',
    +          'class' => ['layout-reorder-previous'],
    +          'data-layout-builder-reorder-direction' => 'previous',
    +        ],
    +        '#value' => $this->t('Previous'),
    

    Just have placeholder a tag for now. Need to make these into actually links to the layout_builder.move_block route that will move the block. This would make it work even if JS is turned off.

    Hmmm. maybe if this link simply was a ajax link we could remove all custom JS? Will try this.

tedbow’s picture

  1. Hmmm. maybe if this link simply was a ajax link we could remove all custom JS? Will try this.

    Ok, this is patch removes all JS. So it is possible but the UX is not as nice.

    On my local it is fine because the response is so fast but if I throttle to "slow 3g" you don't see the block move right away.

  2. Now the links will work with JS enabled.
tedbow’s picture

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -337,4 +345,185 @@ public function cancelLayout(SectionStorageInterface $section_storage) {
+            'section_storage' => $section_storage->getStorageId(),
+            'delta_from' => $delta_from,
+            'delta_to' => $previous_delta_to,
+            'direction_focus' => 'previous',
+            'block_uuid' => $uuid,
+            'preceding_block_uuid' => $previous_preceding_block_uuid,
+            'region_to' => $previous_region_to,
+            'section_storage_type' => $section_storage->getStorageType(),

So now that each link has all the information about where the block should be moved to we could probably add back the JS but the JS would not have to do any of the logic to determine where the block should go.

I will try this next.

tedbow’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2995689-8.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.97 KB
18.39 KB
  1. #8 failed because of undeclared var, fixed
  2. This patch adds all information about where the block should move for each link that figured out to make the route arguments for the links and embeds that data into the link using data- attributes.
  3. This allows moving the block directly with JS without the complicated logic that was in #2
  4. The block is moved instantaneously and then the ajax call is made. While the ajax call is made(if it is slow connection) the user will already see the block in the new location. The only weird thing is that link may different if they are at the end of the page because when updated there won't "next".
    @todo add a .updating class to the block. This would removed when the ajax replace is done. Then we could somehow hide the navigation links or turn into another icon when there is current ajax call.
  5. @todo we should probably stop the user from making another reorder action whether through link or drag/drop when they have already made a re-order action and the ajax has not returned.
tedbow’s picture

Assigned: tedbow » Unassigned
FileSize
12.66 KB
31.05 KB

Adding test.

This JS test places a bunch of block in different regions of 2 layouts and moves the top block from the top of the page to the bottom and then form the bottom back to top.

andrewmacpherson’s picture

@tedbow This issue is confusing to follow, because there isn't an explanation of what you're trying to build.

The proposed resolution is just a picture. What does it show, and how is it intended to behave?

I can see the word "move" followed by 4 separate arrow icons. But there's also a "4-arrows-combined-in-a-cross" icon in the blue area above them (the same icon as we use in draggable table rows). Why are there two sets of arrows? Are they for different operations?

I'm not clear if there is a design for the interactions, or whether you're experimenting code-first.

I think we should start with a list of the operations that a user nust be able to carry out, then figure out what UI elements will be needed to facillitate these with a variety of input devices. Do that before writing any code.

For example, a user needs to be able to indicate (select) which block they intend to move. How could we facillitate that for someone using....

  • A mouse? e.g. point then click, or point then click-and-hold.
  • A finger on a touchscreen? e.g. touch it and keep your finger on the screen. Are there any differences to consider when compared to mouse interaction?
  • Keyboard? e.g. press tab until focus reaches a control which is associated with the block you want.
  • Speech control?

The picture shows arrows which seem to be associated with one specific block. Going by this, I can't see any way that a user can indicate which block they want to move using speech control. They need to be able to say the name of the control they want to activate. How would they indicate that they wanted to move the Cassini/Voyager block?

The problem here is that "without drag and drop" has been misinterpreted as "using a keyboard". But that doesn't follow.

If you're trying to address one input device at a time (mouse, touchscreen, keyboard, speech, etc.), the UI and code will quickly get messy and confusing, and some groups of users won't be able to use it at all.

This is similar to how we end up with a building which has a grand looking front entrance with steps, and wheelchair users have to use the goods entrance at the rear of the building.

Re: #2

Adds previous and next links to each block in the layout builder.
These links move the block across regions and sections

That's not what links do. Do you mean buttons? Links point somewhere and change the user's context .

I don't know if we should expand this issue to cover keyboard navigation in general or just stick to moving blocks.

What are you referring to by in general?

andrewmacpherson’s picture

In #5, yesterday I said:

Feel free to scope keyboard and speech control as separate issues if you prefer.

Sorry, I take that back because it's misleading. What I should have said was:

Assuming there is an inclusive design, for how we intend to support a wide variety of input methods, than it's okay to treat "naming controls", "visible labels", "implement keyboard behaviours", markup, etc., as separate child issues.

tedbow’s picture

Title: Add ways to interact with Layout Builder without a pointer device. » Allow reordering blocks without a pointer device.
Parent issue: » #3007978: Accessibility Plan for Layout Builder

Adding #3007978: Accessibility Plan for Layout Builder as a parent meta issue.

Hoping that can be the plan issue and then we can move back to this as sub task if it still makes sense.

sorry I didn't provide more a description but I will try to update if no one else does.

andrewmacpherson’s picture

Yes, now we have the meta-plan issue, you can probably keep using this issue for implementing whatever keyboard behaviours we settle on.

We should still take time to plan the keyboard needs well first though.

I'm concerned that we could end up with a lot of tab stops, especially if there ends up being 4 seoarate arrow buttons per block. The WordPress a11y reviewers also noted this problem with the Gutenberg block UI.

I think we should explore using a single "grab-handle" button together with arrow keys. I think it would be reasonably discoverable, and might be enhanced with a tooltip. That could keep us at 2 tab stops per block (grab-handle, and settings cog-wheel).

I appreciate we'd need a way to compute the allowed dirctions for any given position, for movement in 4 directions instead of back/forward in a sequence.

Another issue with arrow key movements, is making sure it doesn't clash with the way a screen reader's existing use of arrow keys. I need to refresh my memory on this, but the ARIA Authoring Practices will help - I expect there's some usefull advice in the grid pattern there. Whatsock might have something helpful too.

tedbow’s picture

I'm concerned that we could end up with a lot of tab stops, especially if there ends up being 4 seoarate arrow buttons per block.

Yes this is was biggest concern with approach of my patch above.

One option would be to display the reorder links but don't put them in the tab order. Then have a new context link for each block, "Reorder block". That would automatically put focus on the reorder links. That way for those using a point device that could click directly but for those using tabbing we don't add extra tabs for every block.

I appreciate we'd need a way to compute the allowed dirctions for any given position, for movement in 4 directions instead of back/forward in a sequence.

is there a way we can do this? For instance if a page is opened at a larger width the layout may display some regions side-by-side but then if page is resized to be more narrow there may no longer be any side-by-side regions.

Also I don't think we can know for sure that regions or blocks that core intends to be side-by-side or otherwise will actually rendered side-by-side or other intended relationships in any give theme.

For instances give the core's two column layouts there is "bottom" region. In bartik the blocks in this region will be displayed vertically 1 after the other. So if we had 4 arrows for up/down/left/right then clicking down or using arrow key down should move this block in relationship to the other blocks in the region.

But if another theme's intention is for blocks in this region is to be displayed horizontally instead of vertically then the theme could simply add

[data-region="bottom"] [data-layout-block-uuid] {
  float: left;
}

Then responding to the down link or down arrow key if there is left and right also would not make sense. And of course if there are user style sheets that would also affect it.

So the only way to know if left/right vs up/down should move a block in relationship to other blocks in that region would have to be someway on the client side. Can this reliably be done via JS in cross-browser compatible way? There maybe away but I don't know of one(though not JS expert).

That is why I prefer previous/next links/keys vs 4 directions. Also that tab order now for contextual links in the layout builder follow this same order. Tabbing(next) takes you through the block within regions the same exact order the "next" link/key would take you.

andrewmacpherson’s picture

Interesting stuff in #18. I'm stlll digesting it all.

The layout builder UI uses the default (a.k.a. front-end) theme, but the block arrangements are saved in a theme-agnostic way? I hadn't thought about that. This is different to block placement in theme regions, which are saved per-theme.

What I meant by computing allowed movements, I originally thought it could be deduced either from the icon_map property in *.layouts.yml (but I've learned that info is optional), or else deduced by JS in the browser after rendering the layout HTML/CSS.

I kind-of assumed it applied to the theme being used by the layout builder at the time of editing. I didn't take responsive layouts into account, say if someone is using the layout builder on a portrait-oriented iPad and gets the middle breakpoint.

So with all that, I see how 2-dimensional movement is hard.

One option would be to display the reorder links but don't put them in the tab order. Then have a new context link for each block, "Reorder block". That would automatically put focus on the reorder links.

So in the mockup, the 4-arrows-in-a-cross icon might be the "reorder block" button, which reveals the 4 arrows. This would get us to 2 tab-stops per block (reorder and settings). I'd be happy with that.

We could even get it down to one tab-stop per block if the movements and settings were under the same menu button. Say, the movements formed the first row of the settings menu (a bit like how cut/copy/paste buttons are on one line in the Firefox menu). I don't know what a single combined reorder/settings button would have as a label/icon though:

Screenshot of Firefox main menu, shows cut, copy, paste controls on one row of menu.

tedbow’s picture

I'm not clear if there is a design for the interactions, or whether you're experimenting code-first.

Ok. experimenting code-first again. I just trying see what might be possible as far as 4 way navigation but based on our discussion here.
I haven't addressed anything about the tab order but we could take this out of the tab order and then make them available after you select to reorder.

The javascript is rough but I want to see if it is possible
I am not sure right now if it preferable to navigation with only 2 directions but it something to consider.

the patch starts from #2 because it needs clients side js logic.

re #19

So with all that, I see how 2-dimensional movement is hard.

Yeah that is a lot that could go into it.

from me in #18

But if another theme's intention is for blocks in this region is to be displayed horizontally instead of vertically then the theme could simply add

I am wondering if we assume this will not happen. I looking at how the Layout builder was made made that it may have been the intention that regions were used to stack block vertically and some regions would be next to each at certain wide enough widths.

I think could document this. It would probably make navigation for all devices easier if there were some known limits. Of course there is no telling what I theme might do but we have to make some guidelines and state one of the reasons is accessibility. Probably also site builder expectations across themes would be a another good reason.

So here is patch that tries to see if 4 way navigation is possible.

Here are the assumptions it makes:

  1. Blocks will stacked horizontally within a single region
  2. regions within a layout may appear side to side
  3. Those same regions may appear stacked horizontally at narrow widths

The patch should provide

  1. Four way navigation links
  2. If a block has no way to move in a given direction that direction's link will be hidden(maybe should be disabled).
  3. For instance if there is row with 3 regions blocks in the left most region will not have a "left" link shown.
  4. When a block is moved to another region its links should be updated
  5. For instance if the blocks in the leftmost region are moved to the middle region the left link should appear.
  6. resizing the page should adjust what links available.
  7. For instance if the block in the middle region and the page is resized to the regions being stacked the left/right links should be disappear.
  8. If they page is expanded again so the regions are horizontally next to each other the left/right links should reappear.
  9. The up/down links should move to the region that is above/below as the regions are currently displayed.
  10. This means that "up" would actually move to different regions based on how wide the page is because the region that is above at a narrow width is actually to the left at a wider width.
  11. When moving into a new row of regions the block would always be moved to the left most region whether is being moved from above or below.

here is demo https://youtu.be/d63oIeKpxZA
No CSS applied to links

Status: Needs review » Needs work

The last submitted patch, 20: 2995689-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

This patch adds a couple things

  1. Some JS clean up. Still rough don't want to worry about all the details while trying to figure which is the prefered way
  2. Ensures that link that used to move the block is not available after the block is move then the opposite direction receives focus. For example if you click "left" and there is not region to the left after moving "right" link will receive focus.
  3. Fix test failure.
tedbow’s picture

Status: Needs work » Needs review
FileSize
11.67 KB
26.11 KB

And ... the patch

AaronMcHale’s picture

So I posted a comment in the parent issue then noticed this one so linking to my comment here #3007978-14: Accessibility Plan for Layout Builder.

I think my idea that I linked could solve all the problems that occur when trying to implement effective movement controls and could be a lot simpler but just as - if not more - effective.

tedbow’s picture

@AaronMcHale I do like that idea. I might try to make up a rough working version of that on that.

I think another advantage is that we could render the possible destinations on the server and then just display them after you click "move block". possibly this could just a be a contextual link.

I think after you clicked "move block" your tabbing should be constrained to block destinations. So for keyboard navigation it would something like

  1. Click move block. either contextual link or icon.
  2. all destination locations are made visible.
  3. We use Drupal.announce for screen readers to say "Your tabbing is constrained to possible locations to move this block. Press enter to select a destination. Press esc to exit" or something like that. It would similar to who we constrain tabbing in the "edit" mode from the Contextual module. That way I think we have the added advantage of being something similar to what a keyboard user of drupal is already familiar with.
  4. As the user tabs to destinations we again use Drupal.announce to inform them of the current destination.
tedbow’s picture

Ok here is what I think @AaronMcHale was describing in the linked comment in #24

Demo video of this method: https://youtu.be/41OfUEz-4wY

Again this a rough patch just so people can try out the functionality. Once we pick a method we can work on refining it.

I do like this best.

Among other advantages mentioned

  1. Fewest clicks
  2. Fewest ajax calls, only 1 to final destination
  3. block doesn't get re-render to positions it won't be in along the way.

the keyboard navigation is not done for this patch but I described it before and in the video. I think it won't be hard technically.

AaronMcHale’s picture

@tedbow thanks for trying it out, I really like your demo, and your ideas in #25 sound like a good plan forward for it.

Thanks
-Aaron

andrewmacpherson’s picture

Various feedback about #24-27.
It's a useful ecperiment, but I don't think we can say it is the way forward. It leaves many many challenges unaddressd

I'm sceptical of using tabbing manager here. It kinda smacks of an "oh we have libraries!"

I think tabbingManager usn't very flexible. When D8.0.0 came out it was barely used, and I don't think we can just assume it's a tried-and-tested pattern we can stick on any UI. Maybe it was intended that way, but I don't think it is gonna be a good fit just anywhere.

The way it dumps long sentences into Drupal.announce() is a bit overdone IMO. It kinda sounds like it's trying to be like VoiceOver, which gives a highly verbose running commentary in it's default setting, probably to support new users. But other screen readers are much less verbose by default. And if you turn down VoiceOver's verbosity, the custom Drupal announcents (tabbingManager and others) start to sound very chatty by comparison. There isn't any way for us to know what the screen reader's settings are though.

Drupal.tabbingManager also suffers from being a bit "screenreader-centric", in a way that neglects other users. Sighted users can be frustrated by the fact that it leaves controls in a perceivable-but-not-operable state, and it's not clear how those can be activated.

For example, the video in #26 shows "add section" buttons thougout the process. Do these remain operable (and does it make a difference what input device is being used)?

Modality:
This is my main concern, and it affects several different input methods.

This introduces more modal behaviour. I don't mean modal in a dialog-box sense, I mean modal like the Vi text editor. There's a reason why modal text editors aren't mainstream: users get stuck in a mode and confused about what's possible.

After pressing move-block, can I abort, or must I complete the place-block action?

Meanwhile a mouse drag-and-drop isn't really modal, in the sense that it can be more easily aborted by letting go early, or letting go in a random meaningless/disallowed place.

So we're creating a division where mouse-users get non-modal behaviour, and everyone else gets a modal behaviour. That goes against an important inclusive design principal of creating an equivalent experience.

Drag and drop, or cut and paste?
The process of "click the move-block command, tab to destination, click the place-block button" - is there a precedent for this, in Desktop OS for instance?

  • It seems to have some qualities like a mouse drag-and-drop, e.g. permitted destinations are indicated. Howver desktop UIs usually do it the other way around, by indicating that a destination isn't allowed, rather than what destination is allowed.
  • On the other hand, it has some qualities in common with a cut-and-paste operation, e.g. there's no indication of what you currently have grabbed.
  • But unlike cut/paste or drag-and-drop, this has a modal quality: after clicking move-block, I have to drop it somewhere. What if I grabbed the wrong block?

Cognitive accessibility:
The "add block" buttons are temporarily replaced(?) with "place block". This is a tricky naming distinction - what's the difference? This impacts cognitive ability, reading level, and UI localization.

Speech control:
I think the visible + operable destination buttons could be a feasible approach for speech control users, which is a random-access input method.
It's not the only approach we can use though.

Screen reader:
The modal aspect of this could be confusing. Visually there's stuff you can do to convey a special mode, like highlighting or dimming certain controls. For screen readers we need to find another way to convey this. We could put in custom messages via Drupal.announce(), but I'm wary of relying on those because:

  • They're ephemeral, only uttered once.
  • They can be interupted.
  • They could be hard to manage. Like, we'd get the generic announcenent about tabbing constraints from tabbingManager, but that doesn't have much context, it's not clesr how that follows from "move block".We might need another message relevant to the layout builder operation.
  • For comparison, we got some feedback that the CKEditor toolbar configuration was hard to understand (TODO find the issue link).

Screen magnifiers:

  • The "move-block then place-block" approach could solve difficult long-distance mouse drag problems.
  • It won't help with orientation problems.
  • It might be better in combination with some visible signposts to say where you are. I'm going to propose that on the parent issue, with more detail, because they have several uses

.

Switch access scanning:
I have a hunch the place-block destination buttons may work really well for switch access scanning! (I could be totally wrong about that, I don't have enough experience of it.)

andrewmacpherson’s picture

I missed the title change in #16.

In #6, I said "add ways to..." - plural - because I don't expect to find a one-size-fits-all method.

I've elaborated on this in #3007978-17: Accessibility Plan for Layout Builder.

AaronMcHale’s picture

Thanks for the detailed input @andrewmacpherson, I need to digest your thoughts a little more but after reading over it from what I can tell it looks like most of your comments are specifically around how we should go about implementing this and not necessarily about the fundamental idea.

andrewmacpherson’s picture

No, to be clear, it's not just implementation details. I am sceptical of the overall idea. Let me take another go at explaining why.

Does the method you propose have a well known precedent? I'd be more easily convinced if this was commonly used in other applications, especially native desktop ones.

I'm not saying your idea isn't worth exploring further. However I don't think we can just make up a brand new method, and assume that users will understand it. That's the high-level accessibility problem that WordPress Gutenberg finds itself with (broadly speaking).

The main part which I think is troublesome is that it introduces a modal behaviour, for everyone except sighted pointer users:

  • Pointer users can move a block to another position with a single drag/drop operation.
  • By contrast, a keyboard user first has to nominate the block they want to move, then whilst the page is in the "place block" mode, they have to navigate to a different control to complete the action.
  • Likewise for speech control users.

It's not the extra physical actions (number of keypresses, say) that bother me, so much as the extra cognitive effort of looking for a second control. And we'd be imposing this cognitive effort from this extra step on screen reader and magnifier users, who are already missing the visual overview that pointer users with good vision have.

In the existing block layout UI for theme regions (/admin/structure/block), keyboard and speech control users don't have this two-step modal process. They can use the region drop-downs to move a block to any other region, by choosing the region name. This involves just a single control, without needing to navigate to a second control elsewhere on the page to complete the action. The same holds true for the weight field (reorder within a region).

I think multi-step modal actions are easier to comprehend when the actions are neatly contained within a dialog. That's what I was suggesting in the parent issue - a move block dialog. Alternatively, section and region controls might be included in the block's off-canvas settings dialog. Dialogs have clear affordances for aborting, and room for help text too.

Another idea would be to provide a list of section/region names in the contextual menu, so movement doesn't need a second control. This might be similar to desktop window managers, where a dropdown has options to maximise, minimize, close the window. In some cases this menu also includes options for tiling windows, or moving them to another desktop. I'll get some screenshots of these.

andrewmacpherson’s picture

Views UI uses dialogs to re-order fields, filters, sorts, etc. That might be worth looking at.

andrewmacpherson’s picture

Re #2;

I looked through #2920006: Research accessibility of drag-and-drop grid interfaces. I didn't see anything that was ready for us to right away

That issue languished. It gathered a decent number of libraries, tutorials, demos, etc.. But it didn't carry out any detailed appraisal or comparison of those. I got side-tracked by other stuff like Umami.

I think it's worth waking that issue up again, because we might very well find something usable there, once we understand those approaches better.

There's also the CKEditor toolbar config widget in D8. This is built with a jQueryUI draggable, the same as layout builder I think. Then it has some backbone views to extend it's behaviour. We did get some feedback from some AT users that it's difficult to use though (issue link?). It's missing focus styles for one thing.

andrewmacpherson’s picture

Also re #2:

I think doing the up, down, left and right links would very hard because it is very difficult or impossible to know if regions are next to each other or below. This will change at different breakpoints and themes could override this in the CSS

If I recall, there's something in one of the drag/drop articles or libraries about this.

Although the block region assigments are stored in a theme independent way, the idea of using direction to reorder blocks only applies to the LB interface while it is actually in use.

So for direction controls, I don't think it matters how any given theme styles the layouts. What does matter is where the regions are on the actual layout builder page. If there's a way to compute allowed movements, that can be run as JS on the LB page, by examining the DOM and CSS, then surely it doesn't matter which theme it is?

Breakpoints are an interesting case. If the LB is used at different breakpoints, the spatial arrangements change. So JS can react to viewport changes, and recompute the allowed movements.

There are different points in time where we can try to compute the allowed movements.

  • Before rendering the LB, say the server-side PHP examines the icon_map. We can reject this idea for the reasons already covered.
  • When rendering the LB client-side, after the browser has applied the CSS to the layout. By this time we know how the LB is actually layed out in use.
  • On breakpoint change, same thing really.
  • Alternatively, don't compute the alllowed movements until someone actually focuses on a drag-handle control. Knowing at this point in time would allow us to give the user a hint about which movements they can do.
  • Or, don't compute allowed movements until after the user actually tries to move it into a particular position. If we leave it until this stage, it's only useful for saying "No, not allowed here".

Aside: how does the mouse drag and drop decide whether a drop destination is allowed? That's a different thing , based on knowing what the mouse has moved over I suppose.

We could study how the allowed movements work in the tabledrag library. Note, that works with a keyboard in D7 but is currently broken in D8. We have a fix for it in #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys.

All easier said than done, mind.

tedbow’s picture

re #34

Yes figuring out the 4 direction movement is hard but maybe doable. The patch in #23 shows that is it probably possible to figure out the 4 directional navigation on the client via Javascript. It handleThat patch is limited but I think it could be done.

The description #20 and the video explain how it works regards to breakpoint and figuring out on the client side if a region above or to the side

Anyways if determine that 4 directional control on each block is the best method to be most accessible across various inputs then we should definitely give the JS a better try. No need to see that as technical limitation without exploring it more.

andrewmacpherson’s picture

@tedbow - Good to hear it might be feasible. I'm prone to blue-sky thinking :-)

It'll be worth looking at the various libs in #2920006: Research accessibility of drag-and-drop grid interfaces.. I'm forming a list of comparison questions for that.

tedbow’s picture

@tedbow - Good to hear it might be feasible. I'm prone to blue-sky thinking :-)

I may not be 😟 I still have a couple concerns with this method

  1. I talked to @drpal about this. It seems this could be very error prone depending on CSS that is encountered and also layouts that are provided by themes. Of course this is unknowable unless we vastly restrict in documentation what we recommend for layouts.

    My worry with this approach is we get this working with the layouts provided by core and CSS we think users will encounter but in actuality it becomes frustrating and ineffective for users in cases we have not considered or thought of.

  2. also another concern is

    So we're creating a division where mouse-users get non-modal behaviour, and everyone else gets a modal behaviour.

    @andrewmacpherson mentioned this in relation to the other method but I think this will also be a concern for this method more so.

    For the 4 way direction arrows method to work without putting the user into a modal behavior we have to save the position every time they shift the position of a block. Which means we have to make an Ajax call every time. While this works fine in development if you test this with "slow 3G" connection it becomes very frustrating and would be very complicated if don't stop all other actions on the form while we wait for Ajax to complete(because other actions could affect relative position). If you want move a single block through may regions this becomes very slow.

    If we want to avoid this and only save the position when the user has moved the block into "final" position. We have then introduced a new "mode" the user has to know to exit, saving the position which then defeats one the main advantages of this methods articulated by @andrewmacpherson above.

tedbow’s picture

Though I want to advocated for the method described in #26 first here is one option to solve #37.1(but not .2).

Change the behavior of the up/down/right/left buttons.

To be

  • up/down = previous/next section
  • left/down = previous/next column in the section

Once #2938132: Ship layouts that make sense with Layout Builder's concept of sections lands all core provided layouts will be 1 row with 1 or more columns in the layout. So to make a complex layout you stack single column layouts. So at larger breakpoints this would be the simple 4 way navigation.

but for the case where there narrower breakpoints or there is layout which doesn't use single row method we always before deciding what movement should be up/down/right/left we always call a JS method getPositionForDirection(direction) that is connected to the current section's layout. Cores default implementation of this method would be very simple if the columns are currently displayed horizontally then rely on 4 way if columns are displayed vertically getPositionForDirection('left')|getPositionForDirection('right) would return NULL so only up down would show and these would move to different regions before going to next section.

This would vastly simplify the logic needed to figure out what up/down/left/right actually do but this would make assumptions the layouts. It would not rely on figuring out in JS visually how the blocks are displayed. So hopefully it would be less error prone.
If layout didn't follow pattern this they should provide their own getPositionForDirection() in which they would determine what the "up" position would be.

tedbow’s picture

Regarding the method the @AaronMcHale described in #24 and I tried basic patch in #26

@andrewmacpherson for you concerns about the modal nature of it.

One idea would be try to make this method less modal.
So that after some accessed "Move block"

  1. Don't hide the "Add block" or other any other links
  2. Show the new "Place block here"(other wording) destinations
  3. The user navigates to these destination locations and clicking on 1 would move the block
  4. All other actions would not be removed from navigation.
  5. if the user clicked on any other action on the page, such "Add block" or "Remove section" then the "Place block here" destinations would disappear.

if did this then we aren't putting them in separate mode. they can still all the actions they could do before they clicked "Move block" we are just showing them the next step for the 2-step move block action. If they don't want to move the block then don't have do anything to escape the action.

AaronMcHale’s picture

Just catching up on everything here, in short I agree with @tedbow that it'll likely be to risky and make too many assumptions to successfully implement the directional controls, perhaps they would work as sort of shortcuts to move a block between sections but I think that's about as specific as we can be without risking unreliability or making assumptions. Additionally I like @tedbow's idea in #39, it sounds like it would definitely go some way to address the modal concerns and possibly improve the experience.

webchick’s picture

I really appreciate all of the good discussion and brainstorming happening here. But let's please also try and keep in mind that layout / page building is currently a gigantic gaping hole in Drupal core's functionality, for 100% of our users, and we only have a couple of Drupal 8 releases left (at most) in which to ship / stabilize new features.

So I would encourage us as we're fleshing this plan out to try hard and find any approach we can that satisfies any "must have" accessibility requirements that need to block a stable release, but also identifies additional places we could further iterate on accessibility improvements in the future, after this feature is generally available to everyone.

tedbow’s picture

Ok. Here is a patch that starts from #26 but tries address @andrewmacpherson's feedback

  1. Did my idea in #39,
  2. Change the "Place block" destinations to look different/better, but probably needs more work
  3. Add distinguishing text for every place block link similar to #3013770: Distinguish between the repeated text of buttons in Layout Builder UI

Here is demo
https://youtu.be/PDPaT1q_kJk

tedbow’s picture

Ok I was not very happy with the difference between the "add block" links and the "place block" links.

I think after you click "move block" it should be very obvious where would need to click to place the block.
So I

  1. changed the background of place block to yellow(can find a better color)
  2. Add a thick border around block that has been selected to be moved in the same color
  3. Made the block opaque

The last submitted patch, 42: 2995689-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 43: 2995689-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
19.25 KB

Ok the test broke because it was testing that label was not output but now we would be outputting all the labels of the blocks like
Place block <span class="visually-hidden">in section 1 after block This is an override</span>

@todo Need to look through the existing tests to see if you we are checking for block labels other places where we should be actually now checking that it is an <h2> and not the visually hidden one.

that kind of check won't break now but if the label didn't show up but the test also could keep passing if the <h2> for some reason didn't get output because the test would pick up the new visually hidden text.

tedbow’s picture

Issue summary: View changes

Updated issue summary

andrewmacpherson’s picture

For posterity, here's a pen-and-paper mockup which we looked at in the accessibility review collaboration with the Web Access team at UC Berkeley today. We won't be using this form, but their feedback will inform the next prototype here.

A standard Drupal dialog, with the title Move Block. It has combo-boxes for region and weight.

In this mockup, we see a simple select listing all current regions (from all sections), together with a weight field for the block's position in the region. This is a standard (central) dialog, but it could work equally well as the off-canvas dialog style. The idea is for users to select "Move block" from a block's contextual links, and complete the rest of the movement choices inside the dialog.

Feedback from the UC Berkeley Web Access review:

The weight field lacks context; one block weight on it's own isn't much use. It isn't telling you where that weight is in relation to the other blocks in that region. If the there are 3 blocks, and the middle one has a weight of "-4", what weight does it need to make it the last block in the region? Rather than weight as an arbitrary number, the actual information needed is position-in-set ("2 of 3").

The UC Berkeley team suggested that if the region selector was in the right-hand ("off-canvas") dialog, there would be room for a tabledrag list of all the current blocks in that region. Then the position-in-set info would be clearer.

tedbow’s picture

re #48
Here is patch that opens up a form form a "move block" contextual link.

Right now the keyboard navigation is not working. I assume this is related to #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys but the latest patch did not fix it for me. I didn't have time to investigate that further yet.

The code is pretty rough now but it should give idea of what the form might look like.

For besides the tabledrag keyboard problem the form does allow moving a block.

tedbow’s picture

  1. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,266 @@
    +      '#empty' => $this->t('Sorry, There are no items!'),
    

    There should never be an empty components table because the current block will always be listed.

  2. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,266 @@
    +          '#value' => '* ' . $plugin->label(),
    

    I was trying to highlight the current block in the list but tabledrag also uses an "*" so I switched to a border around the current block's row.

  3. Drupal's ajax system is suppose to refocus on the triggering element but this is not working for the region select.

    I have tried the form outside of the off-canvas dialog and it does work.

    I stepped debugged and this code from ajax.es6.js

    if (target) {
        $(target).trigger('focus');
    }
    

    is being trigger but for reason in the dialog it is not actually setting the focus. I will try to figure out if this layout builder specific or just a problem with ajax replace and dialogs.

tedbow’s picture

tedbow’s picture

Add a title to the form

DuaelFr’s picture

I wonder if it could be possible to replace the "weight" by something more human friendly like "Position" with dynamic options like:

  • First, before block "My actual first block"
  • After block "My actual first block"
  • After block "My actual second block"
  • Last, after block "My actual third block"

Edit: I should have read everything first, that's exactly what was suggested by the UC Berkeley team. I strongly support this, then :)

tedbow’s picture

re #50.3
It is actually a bug in the ajax system. not sure about the fix yet but created #3020061: Ajax replace does not refocus element if inside a dialog that has tests to prove the problem.

I don't think it should stop this issue but the navigation is easier if we could fix that.

andrewmacpherson’s picture

@tedbow - great work! I watched the demo video, and I'll try it manually myself soon.

There are some things we'll need to do to help screen readers...

  1. Firstly, a big gotcha - AFAIK, no current screen readers provide information about HTML optgroups! So if there are two regions with matching labels ("First"), but in different sections, a screen reader user probably can't tell them apart. So we'll need an approach to improve this. (Duplicate option labels in a select may also pose a problem for speech control too, I need to go and investigate that, it really never occurred to me until now.) Possibilities:
    • Easiest approach: Include the section and region in the option label ("Section 2, First region"). We can keep the optgroups for now, they may still aid comprehension.
    • Use separate selects for section and region, where choosing a new section updates the available region options, and maybe uses the layout's default region if that's known from the the layout yaml file. Using separate selects for section and region adds an extra step the user needs to perform, but on the flipside it may provide more clarity.
    • There might be some ARIA approach to this, where we provide a visual hierarchy with optgroups, and override the accessible name of the option. However this would need very careful testing with both screen readers and speech control. Let's not go down this route yet.

    Come to think of it, a sighted user can't see the optgroups either, until they open the select. In the demo video, the region value initially looks like "content", without conveying the section number.

  2. We'll need to convey the change-of-content to screen readers, say when choosing a different region, and the list of blocks in the tabledrag is updated. Drupal.announce() will suffice for this.
  3. The block being moved is highlighted visually with a border. We should have a way to convey this with text too, such as appending "current" to the block name.

Also... I found some interesting prior art in a contrib module - menu_link_weight has done something similar already!

This module replaces the standard numeric weight dropdown widget for menu links in the node form with a tabledrag widget that lists all children for the selected parent.

That's basically what we are dealing with here, how to position the block relative to others in it's parent region.
The module page has screenshots, and it appends a visible "(current menu link)" in the tabledrag.

Makes me wonder whether this approach could be used elsewhere in core, such as the book weight field on the node form. Basically anywhere you are dealing with a weight, but have no idea what weight the siblings already have.

tedbow’s picture

@andrewmacpherson glad you think this is going in the right direction!

re #55

  1. I added the section to the labels. So an example label is "Section: 1, Region: Content". I left the optgroup in for now. As a sighted user it seems to make scanning easier to see the groups of section. I don't think the benefit of optgroup is that much though so if it degrades other users' experience I think it would be find to remove it. But from my reading of @andrewmacpherson's comment these are ignored by screen readers.
  2. I created #3020352: Create a new AnnounceCommand to call Drupal.announce on an AjaxResponse because it seems like we should be able to call Drupal.announce() directly via a command on the AjaxResponse object. I think if we got this it would encourage the use of Drupal.announce() in other cases in core and contrib where an ajax response should notify users of some update.
  3. Luckily we have all of the Javascript we need highlighting from the patch in #2994909: Highlight active element while working with dialogs in Layout Builder. I applied the Javascript directly from that patch. The Javascript already got the ok from @drpal(Javascript maintainer. I added CSS for a border also
  4. ,

One other thing I thought about is that we could offer version of the layout icon map on the move block form which could highlight the select region in the layout as a visual cue as to which region is selected.
Something like this:

For the core layouts after #2938132: Ship layouts that make sense with Layout Builder's concept of sections this might not be super important because they layouts will be simpler but contrib modules or themes might make more complex layouts.

If we decide this is something we want I think we should open up a separate issue to allow highlighting of regions in the generated icon map because this will need changes to an interface and at least 1 class.
If shouldn't be too hard though because \Drupal\Core\Layout\Icon\SvgIconBuilder generates an SVG which of rect tags and we can just apply a new .highlighted-region class to the region.

Tagging for needs tests

tedbow’s picture

Here is a test. It just tests moving to a new region and that block list updates correctly.

After #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys we can test reorder the blocks.

tedbow’s picture

Ok. here is patch that includes the fix from #2995689: Allow reordering blocks without a pointer device. This allows making test that actually reordering the block with the tabledrag.

tedbow’s picture

Some nits I found plus:

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -114,3 +114,12 @@
+  font-weight: bolder;

I was told generally we don't use font-weight in core.

bnjmnm’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/tabledrag.js
    @@ -329,10 +329,10 @@ var _typeof = typeof Symbol === "function" && typeof Symbol.iterator === "symbol
                 if (nextRow) {
    
    +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -114,3 +114,11 @@
       padding: 15px 0 15px 25px;
    ...
    +.ui-dialog-off-canvas #drupal-off-canvas #layout-builder-components-table tr.current-block {
    +  border: solid 3px;
    +}
    

    You can bring back the font-weight rule, but with bold instead of bolder -- however I did notice that you need to either increase the specificity by a td for the rule to be visible, or remove the tr

    .ui-dialog-off-canvas #drupal-off-canvas #layout-builder-components-table tr.current-block td
    

    or
    .ui-dialog-off-canvas #drupal-off-canvas #layout-builder-components-table .current-block

  2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -51,4 +51,18 @@
    +  $(window).on('dialog:aftercreate', (event, dialog, $element) => {
    

    Code does a pretty good job self-explaining, but good to add a lil comment to this and the dialog:afterclose handler.

  3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -51,4 +51,18 @@
    +    if ($element.find('[data-layout-builder-target-highlight-id]')) {
    +      const id = $element
    +        .find('[data-layout-builder-target-highlight-id]')
    +        .attr('data-layout-builder-target-highlight-id');
    +      $(`[data-layout-builder-highlight-id="${id}"]`).addClass(
    +        'layout-builder-highlight',
    +      );
    +    }
    

    Changing to this would deliver the same result with one less query

    const id = $element
          .find('[data-layout-builder-target-highlight-id]')
          .attr('data-layout-builder-target-highlight-id');
        if (id) {
          $(`[data-layout-builder-highlight-id="${id}"]`).addClass(
            'layout-builder-highlight',
          );
        }
  4. While working in the Move Block form, dragging a block position triggers a "You have unsaved changes" warning message - this does not happen when changing block region or changing the weight via select menu. It's presented by tabledrag, so unsure if it is in scope, but it would be nice to make things more consistent have the message respond (or not) to all unsaved changes
  5. After a block has been repositioned and the dialog closes, the next element to receive focus does not correspond to the block that was just moved - in some cases it scrolls to a part of the page where the just-moved-block is not even visible. I checked to see if #2973921: Interactive controls inside preview block in the Layout Builder form should be disabled. might fix this, but it did not.
tedbow’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility +accessibility
FileSize
5.08 KB
25.39 KB

This is reroll I haven't looked at @bnjmnm's review in #60 yet(but thanks!)

blockers got committed 🎉
#3020352: Create a new AnnounceCommand to call Drupal.announce on an AjaxResponse
#2725259: [regression] Table Drag handles no longer respond to up/down arrow keys

#2938132: Ship layouts that make sense with Layout Builder's concept of sections also got committed so the test had to update here for region names and saving section configuration when adding a section

The only changes were a couple other waits in the test to be careful.

I will address #60 next.

Status: Needs review » Needs work

The last submitted patch, 61: 2995689-61.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
918 bytes
25.44 KB

Test fix. also removing "Needs Tests" tag because we do have tests now.

tedbow’s picture

now for #60 review

  1. fixed
  2. fixed
  3. nice catch! fixed
  4. Yeah that is a bit confusing. I think my preference would be not to show any message with the table drag and only the existing message that we have to all layout changes after the block is actually move. Other this would be different than say the configure block form where you can make changes but you don't see a message. But I don't actually know how we would do that aside from css hiding .tabledrag-changed-warning for this table. It seems hard coded in tabledrag.

    going to add that css for now.

    hmmm. since table drag has an inline style this seems only possible with !important 😞

  5. Looking for general solution here #3019849: Return focus to point of action when the off-canvas dialog closes

The last submitted patch, 63: 2995689-63.patch, failed testing. View results

bnjmnm’s picture

In order to work with the highlighting in https://www.drupal.org/project/drupal/issues/2994909 MoveBlockForm will need a data-layout-builder-target-highlight-id. Not sure which issue will get committed first, but perhaps this should get a Todo:?

KarenS’s picture

I don't know if anyone has run into this, but I have a layout with blocks created previously (in 8.6). When I add this patch and check my blocks, none of the existing blocks has the "Move" option. But if I create new blocks they do have it.

tedbow’s picture

@KarenS yes this is problem because contextual links are cached in browser local storage and there is not a good way to invalidate that now. Here is the related issue #2773591: New contextual links are not available after a module is installed

There is a way for us to get around this bug like we did in Settings Tray but if we could get that issue in it would be great!

tim.plunkett’s picture

Status: Needs review » Needs work

I think going with a workaround linked to that issue would be best.

tedbow’s picture

Status: Needs work » Needs review
FileSize
921 bytes
26.83 KB
  1. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,291 @@
    +    $form['#attributes']['data-layout-builder-target-highlight-id'] = "block-$uuid";
    

    re #66. We already have it here. This issue use the same JS. So which patch gets committed second just need to remove some js

  2. re #69
    The workaround we used in settings_tray won't apply here because the blocks in layout builder admin don't have hook_block_view_alter called on them.

    Adding a new unused argument in \Drupal\layout_builder\Element\LayoutBuilder::buildAdministrativeSection() that will ensure that contextual links that were added before this

    Actually I realized we could just add a new metadata key to the contextual links we add in \Drupal\layout_builder\Element\LayoutBuilder::buildAdministrativeSection(). I am adding the a 'operations' key to the metadata key so if we add any new operations we just update the key.

    This will mean that any links that we created before this issue will not have the same contextual id that the contextual module uses to cache the links on the client side. It also means we are free to add new operations in the future.

    so therefore all previous ones will be cleared but ongoing the contextual links will be cached on the client side. I didn't add @todo because I don't think this is really a workaround and #2773591: New contextual links are not available after a module is installed won't solve it.

    We are not causing the client-side caching not to be used where it should be used. We are using metadata to vary for caching purposes which is intended.

tedbow’s picture

reroll after #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features and self review

  1. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,291 @@
    +    $this->classResolver = $class_resolver;
    ...
    +   * Builds block move form.
    

    This is no longer used

  2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,310 @@
    +    $this->clickLink('Save Layout');
    

    This needed to change to pressButton() because of #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features

  3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,310 @@
    +   *   The expected block lables.
    

    "labels"

Status: Needs review » Needs work

The last submitted patch, 71: 2995689-71.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
862 bytes
25.95 KB

I plan to review as well but here's a reroll to get this out of "Needs Work".

bnjmnm’s picture

Status: Needs review » Needs work
FileSize
38.58 KB
47.84 KB
44.27 KB
  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -114,3 +114,16 @@
    +.layout-block-move .tabledrag-changed-warning {
    +  display: none !important;
    +}
    

    Could use a comment explaining why there's an !important overriding styles from another module.

  2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
    @@ -247,6 +247,7 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
    +          $build[$region][$uuid]['#attributes']['data-layout-builder-highlight-id'] = "block-$uuid";
    

    #2994909: Highlight active element while working with dialogs in Layout Builder has been updated to use a trait for adding data-layout-builder-highlight-id

  3. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,286 @@
    +    $current_section = $sections[$selected_delta];
    +    // Create a wrapping element so that the Ajax update also replaces the
    +    // 'Show block weights' link.
    

    It's not officially part of the standards, but I've been asked to add a space above my inline comments. It happens a few other times in this patch - you can decide if its worth doing in these instances.

  4. If you drag a block to a different order, then click "Show row weights", the active row styling is a little off.
  5. Once #2994909: Highlight active element while working with dialogs in Layout Builder is in, the "Blocks" caption shouldn't need to be visible (although it could be useful for screenreader users). There should be enough visual cues for the user to know what they are working with, and this caption between the row labels and "Show row weights" looks a bit scattered.
    Currently:

    How it would look without:

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
26.18 KB
  1. Added
  2. added @todo to use the trait in that issue.
  3. fixed
  4. Moved the border from the td to tr. This fixes it so it looks ok whether the row has the 1 td or 2.
  5. I agree it look a little scattered but we are using the '#caption' property of the table. If the \Drupal\Core\Render\Element\Table puts the caption there we should respect that. I don't think individual issue should determine where table caption goes. I think the caption is good especially since we have one for region.

forgot to pull in #73 before I started working on my local patch so including that and interdiff from #71

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

#75.5 makes sense to keep as-is even if it looks a little clunky, then. RTBC.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
26.23 KB

Reroll

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Tests are still running, but moving back to RTBC since it'll kick back to NW if the reroll fails.

xjm’s picture

Title: Allow reordering blocks without a pointer device. » Allow reordering blocks without a pointer device

@drpal a.k.a. @alwaysworking apparently already signed off on the JS for this issue, so it just needs backend code review.

xjm credited alwaysworking.

xjm’s picture

tedbow’s picture

Status: Reviewed & tested by the community » Needs work
  1. I rechecked the reroll in #77. everything looks good!

    The only file that didn't apply directly from #75 for me was layout-builder.es6.js. And that was just an easy block to move in. All other files where exactly the same.

  2. I did find another small edge-case problem though on inspecting the code.
    +++ b/core/modules/layout_builder/js/layout-builder.js
    @@ -91,4 +91,16 @@
    +  $(window).on('dialog:afterclose', function () {
    +    $('.layout-builder-highlight').removeClass('layout-builder-highlight');
    +  });
    

    We need to check here that Drupal.offCanvas.isOffCanvas($element)

    This not as likely to a problem as with #2994909: Highlight active element while working with dialogs in Layout Builder which shares the same JS code(the 1 committed 2nd will have to be rerolled) because this only deals with 1 form. But it could still happen if something else opens a modal or other dialog on the page. If something else opens a off-canvas dialog we shouldn't have to worry because that would mean our off-canvas is closed(can be only 1) and remove this highlight will be fine.

    I am a similar comment on the other issue

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
26.39 KB

here is the fix for #82

bnjmnm’s picture

Status: Needs review » Needs work

This comment is copied from #2994909: Highlight active element while working with dialogs in Layout Builder as it is responding to the same symptoms:

Very good catch @tedbow. Checked this locally and it looks like a similar conditional needs to be added to 'dialog:aftercreate', otherwise the highlight is removed in the same manner. To reproduce, add a custom block and use ckeditor to add a link to the body field. Without adding a check it 'dialog:aftercreate', the highlighting goes away as soon as the "add link" modal appears.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
26.53 KB

@bnjmnm thanks for checking this and catching the other case.

The interdiff was created with git diff -w to ignore whitespace.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Changes to the dialog listeners look good, back to RTBC

xjm’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -118,3 +118,20 @@
    +.ui-dialog-off-canvas #drupal-off-canvas #layout-builder-components-table tr.current-block {
    ...
    +[data-layout-builder-highlight-id].block-layout-builder.layout-builder-highlight {
    

    These are very specific rules. I believe it's generally preferable to make them as non-specific as possible?

  2. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -118,3 +118,20 @@
    +/*
    +Remove "You have unsaved changes" warning because Layout Builder always has unsaved changes until "Save layout" is
    +submitted
    + */
    

    This is not wrapping at 80 chars; how does it do with our CSS linting?

  3. Does #3034347: Update the Layout Builder UI classes to implement BEM naming conventions affect the CSS here at all (i.e. are we still using the correct class names, and do any added class names also follow the standard)?

bnjmnm’s picture

This patch fixes the following:

#87.1 -

These are very specific rules. I believe it's generally preferable to make them as non-specific as possible?

. Good catch - refactoring to BEM made it possible to reduce the specificity.

#87.2 What you spotted did not pass css linting. This is fixed

#87.3 Changed several class names to adhere to BEM

BUT.. the patch introduces this hassle: The patch and diff are larger than they should because this issue depends on #2994909: Highlight active element while working with dialogs in Layout Builder to pass tests and follow BEM. And that issue depends on #3035669: Block contextual links intermittently ignore mouseup, so those have been applied here. The patches for this issue will be much easier to review once those issues are committed (the latter is RTBC).

Status: Needs review » Needs work

The last submitted patch, 88: 2995689-88.patch, failed testing. View results

tim.plunkett’s picture

Issue tags: -accessibility (duplicate tag) +Accessibility

Fixing tag

bnjmnm’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Since #2994909: Highlight active element while working with dialogs in Layout Builder was switched to postponed and potentially subject to more changes, I replaced the reliance on that patch with a @todo

#3035669: Block contextual links intermittently ignore mouseup is still necessary otherwise test will not pass. The changes in the JS files and LayoutBuilderDisableInteractionsTest.php are from this patch.

lauriii’s picture

Issue tags: +Layout Builder frontend issue
lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
6.1 KB
  • #55.3 is still unresolved.
  • The border and background change on this table element doesn't look quite as good as we'd probably want it to look. We should work with the UX team on a next iteration for the styles.
bendeguz.csirmaz’s picture

tedbow’s picture

Status: Needs work » Needs review
FileSize
30.36 KB
674 bytes
  1. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,300 @@
    +          '#value' => $this->t('@label (current)', ['@label' => $plugin->label()]),
    

    This fixes #55.3

  2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
    @@ -251,6 +251,13 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
    +              // Add metadata about the current operations available in
    +              // contextual links. This will invalidate the client-side cache of
    +              // links that were cached before the 'move' link was added.
    +              // @see layout_builder.links.contextual.yml
    +              'metadata' => [
    +                'operations' => 'move:update:remove',
    +              ],
    

    We actually an empty post update hook for this new matadata on the contextual links to be discovered which trigger the new link appearing.

    This is similar to layout_builder_post_update_routing_defaults

fixing 2)

lauriii’s picture

I tried to work on a more elegant design for highlighting the current block. Any thoughts on this?

The last submitted patch, 97: 2995689-97.patch, failed testing. View results

The last submitted patch, 97: 2995689-97.patch, failed testing. View results

The last submitted patch, 97: 2995689-97.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 97: 2995689-96-reroll.patch, failed testing. View results

bnjmnm’s picture

+1 on the change in #97. There's still sufficient distinction from other blocks to identify the current one, it is less jarring to look at, and fewer lines of css are required.

tedbow’s picture

Also plus 1 on #97. I think with (current) now we don't have to be so bold. It looks better.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
101.24 KB
2.53 KB
24.4 KB

Fixed the test from #97 -- just needed to remove the space from 'Body (current) *' as the asterisk uses a margin for that spacing.

Also added whitespace above a few inline comments since that resulted in a few issues getting kicked back to NW

While working on the patch, also tested this in IE11, and confirmed it looks/works fine. Screenshot attached.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -122,3 +122,16 @@
    +#drupal-off-canvas .layout-builder-components-table__current-row > td:first-child {
    +  border-left: solid 5px;
    +  padding-left: 17px;
    +}
    

    I think this could probably use a comment.

  2. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -122,3 +122,16 @@
    +/*
    +Remove "You have unsaved changes" warning because Layout Builder always has
    +unsaved changes until "Save layout" is submitted.
    + */
    

    Going by @lauriii's feedback in other (Media) issues, I think this comment is improperly formatted -- I believe it needs to be /** ... */, like a PHP comment.

  3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
    @@ -109,6 +109,13 @@ function layout_builder_post_update_routing_defaults() {
    +/**
    + * Clear caches due to new metadata on the Layout Builder contextual links.
    + */
    

    Nit: It's not so much "new metadata" as the addition of an entirely new link (and route to accompany it!)

  4. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +  /**
    +   * The Layout Tempstore.
    +   *
    +   * @var \Drupal\layout_builder\LayoutTempstoreRepositoryInterface
    +   */
    +  protected $layoutTempStore;
    

    Nit: Why is "Layout Tempstore" title-cased?

  5. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getFormId() {
    +    return 'layout_block_move';
    +  }
    

    Other Layout Builder forms' IDs seem to be prefixed with 'layout_builder'. Can we do the same here?

  6. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +   * Builds move block form.
    

    "Builds the..."

  7. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +    $this->sectionStorage = $section_storage;
    +    $this->delta = $delta;
    +    $this->uuid = $uuid;
    +    $this->region = $region;
    

    Is there any merit in validating that these were actually passed, and throw exceptions if they aren't?

  8. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      $layout = $section->getLayout();
    +      $layout_definition = $layout->getPluginDefinition();
    +      $section_label = $this->t('Section: @delta', ['@delta' => $section_delta + 1])->render();
    +      foreach ($layout_definition->getRegions() as $region_name => $region_info) {
    +        // Group regions by section.
    +        $region_options[$section_label]["$section_delta:$region_name"] = $this->t(
    +          'Section: @delta, Region: @region',
    +          ['@delta' => $section_delta + 1, '@region' => $region_info['label']]
    +        );
    +      }
    

    This should probably be moved into a protected helper method. Also, the label "Section: N" could probably just be "Section N", no?

    In fact, that brings something else to mind -- does the use of optgroups here have accessibility implications? How will these options sound to a screen reader?

  9. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +    $selected_region = $this->getSelectedRegion($form_state);
    +    $selected_delta = $this->getSelectedDelta($form_state);
    

    How and why do these variables differ from $this->region and $this->delta? A comment might be useful here.

  10. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +    $form['components_wrapper'] = [
    +      '#type' => 'container',
    +      '#attributes' => [
    +        'id' => 'layout-builder-components-table',
    +        'class' => ['layout-builder-components-table'],
    +      ],
    +    ];
    

    Sorta-nit: I think you can use $form['components']['#theme_wrappers']['container'] to set this stuff up, so that you don't need an additional level of nesting.

  11. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      '#caption' => $this->t('Blocks'),
    

    I wonder if "Blocks in section N" might be a more useful caption for this table from an accessibility standpoint.

  12. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +    uasort($components, function (SectionComponent $a, SectionComponent $b) {
    +      return $a->getWeight() > $b->getWeight() ? 1 : -1;
    +    });
    

    Maybe a premature abstraction, but I wonder if this should be a helper method on the LayoutBuilder render element, or the Section class. (Section::sortComponentsByWeight() or something). Or better yet, is there any reason why getComponents() shouldn't return the components in weight-sorted order?

    Feel free to disregard this feedback for now; I'm mostly thinking out loud. These changes are probably out of scope, but if you like the idea, we could set up a todo.

  13. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      $row_classes = [
    +        'draggable',
    +        'layout-builder-components-table__row',
    +      ];
    +      if ($is_current_block) {
    +        $row_classes[] = 'layout-builder-components-table__current-row';
    +      }
    

    Nit: This could be merged with the $label computation above, so we only need to check $is_current_block once.

  14. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      $form['components_wrapper']['components'][$component->getUuid()] = [
    

    Any reason not to use $component_uuid here?

  15. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +          '#title' => $this->t('Weight for @block block', ['@block' => $plugin->label()]),
    

    Is the word "block" redundant here, from an accessibility standpoint?

  16. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +    if ($selected_section_region = $form_state->getValue('region')) {
    

    This seems like a good place to use $form_state->hasValue().

  17. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      return explode(':', $selected_section_region)[1];
    

    Would it make sense to limit explode() to 2 return values? i.e., explode(':', $selected_region_region, 2)?

  18. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,298 @@
    +      return explode(':', $selected_section_region)[0];
    

    Shouldn't this return value be casted to (int)?

  19. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +  public static $modules = [
    +    'layout_builder',
    +    'block',
    +    'node',
    +    'contextual',
    +  ];
    

    Needs @inheritdoc and should be protected.

  20. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#edit-manage-layout'));
    

    Why is a waitForElement() needed here? Is the link added via JS...?

  21. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertRegionBlocksOrder(
    +      0,
    +      'content',
    +      $expected_block_order
    +    );
    

    Nit: This doesn't need to be split across 3 lines.

  22. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $assert_session->linkExists('Add Section');
    +    $page->clickLink('Add Section');
    

    No need to assert that the link exists; $page->clickLink() will do it for you. :)

  23. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#drupal-off-canvas .layout-builder-configure-section [data-drupal-selector="edit-actions-submit"]'));
    +    $page->pressButton('Add section');
    

    Don't know how I feel about this long selector; maybe something like this?

    $form = $assert_session->waitForElementVisible('css', '#drupal-off-canvas .layout-builder-configure-section');
    $this->assertNotEmpty($form);
    $form->pressButton('Add section');
    
  24. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertRegionBlocksOrder(
    +      1,
    +      'content',
    +      $expected_block_order
    +    );
    

    Can be on one line.

  25. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $region_add_block = $page->find('css', '[data-layout-delta="0"].layout--twocol-section [data-region="first"] .layout-builder__add-block');
    +    $region_add_block->click();
    

    This seems like a good place for $assert_session->elementExists()->click(), because $page->find() will return NULL if it doesn't find the thing, thus causing the test to die with a fatal error.

  26. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $assert_session->waitForElementVisible('css', '#drupal-off-canvas a:contains("Powered by Drupal")');
    +    $assert_session->assertWaitOnAjaxRequest();
    

    The waitForElementVisible() call needs to be wrapped by assertNotEmpty(), and I figure we can probably remove the assertWaitOnAjaxRequest() call unless it needs to be there for some reason...

  27. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '[data-drupal-selector="edit-actions-submit"]'));
    +    $page->pressButton('Add Block');
    

    This also seems like it might be better as:

    $button = $assert_session->waitForElementVisible('named', ['button', 'Add Block']);
    $this->assertNotEmpty($button);
    $button->click();
    
  28. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertRegionBlocksOrder(
    +      1,
    +      'content',
    +      $expected_block_order
    +    );
    

    Can be on one line. I'll stop calling this particular thing out; I think my point has been made. :)

  29. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $block_tds = $page->findAll('css', '[data-drupal-selector="edit-components"] tr td:nth-of-type(1)');
    

    Forgive my ignorance, but what purpose does the :nth-of-type() pseudo-class serve here?

  30. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +  /**
    +   * Gets the body field CSS locator.
    +   *
    +   * @param int $section_delta
    +   *   The section delta.
    +   * @param string $region
    +   *   The region name.
    +   *
    +   * @return string
    +   *   The CSS locator.
    +   */
    +  protected function getBodyFieldLocator($section_delta, $region) {
    +    return "[data-layout-delta=\"$section_delta\"] [data-region=\"$region\"] .block-field-blocknodebundle-with-section-fieldbody";
    +  }
    

    Looks like this is only called once; does it need to be its own method?

  31. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +   * @param array $updated_blocks
    +   *   The update blocks order.
    

    Should be "updated".

  32. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $handle->keyDown($key);
    +    $handle->keyUp($key);
    

    Any reason not to use $handle->keyPress()?

  33. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $page = $this->getSession()->getPage();
    +    $handle = $page->find('css', "[data-drupal-selector=\"edit-components\"] td:contains(\"$block_label\") a.tabledrag-handle");
    +    $this->assertNotEmpty($handle);
    +    return $handle;
    

    This can be reduced to one line: $assert_session->elementExists(). If the element exists, it will return it.

  34. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    foreach ($blocks as $block) {
    +      $block_selector = array_shift($block_selectors);
    +      $expected_block = $page->find('css', "$region_selector $block_selector");
    +      $this->assertSame($expected_block->getAttribute('data-layout-block-uuid'), $block->getAttribute('data-layout-block-uuid'));
    +    }
    +    $this->assertEmpty($block_selectors);
    

    I'm not sure what purpose the assertEmpty() at the end of method is serving?

  35. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveBlockFormTest.php
    @@ -0,0 +1,312 @@
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'select[name="region"]'));
    

    Rather than use a CSS selector, maybe waitForElementVisible('named', ['select', 'Region']) would be clearer here?

lauriii’s picture

Issue summary: View changes
FileSize
36.99 KB
  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -122,3 +122,16 @@
    +.layout-block-move .tabledrag-changed-warning {
    +  display: none !important;
    +}
    

    I'm not convinced removing this is a good idea. This was added on purpose to ensure that users realize their changes are not being saved automatically. Even in Layout Builder, users will have to click the "Move" button before their changes take any effect. But I agree, this can be very confusing for the users because of moving the block doesn't actually cause any permanent change. 🤷‍♂️

  2. The caption and the th elements look confusing on top of the table. Would be good if we could get some input from some UX minded people how we could improve this.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
21.27 KB
25.15 KB

Re #106
1-6. done
7.

Is there any merit in validating that these were actually passed, and throw exceptions if they aren't?

It's certainly possible a contrib module might want to use the form and this would be of use. Probably can't hurt to check this, it's been added.
8.

This should probably be moved into a protected helper method. Also, the label "Section: N" could probably just be "Section N", no?
In fact, that brings something else to mind -- does the use of optgroups here have accessibility implications? How will these options sound to a screen reader?

I'm partial to the"Category: Item" arrangement they currently because they have the same structure -- but I also know my UX instincts can betray me once I've been under the hood for a while. Probably a good one to bring up at a UX meeting (plus the Screenreader question)
9-10. done
11.

I wonder if "Blocks in section N" might be a more useful caption for this table from an accessibility standpoint.

I think you may be right. I updated this to specify the section and the region. It matches the options in the regions select menu. It looks a little off to me, but there may be no objective reason as to why.
Probably another good candidate for a UX meeting.
12.

Maybe a premature abstraction, but I wonder if this should be a helper method on the LayoutBuilder render element...

Looks like getComponentsByRegion already does this, so I made the method public to use that instead of what was there.
13-14. done
15.

Is the word "block" redundant here, from an accessibility standpoint?

This is probably another good one for a UX meeting.
16-2. done
23.

Don't know how I feel about this long selector; maybe something like this?

Went with a slightly different approach (contains()), but the selector is simplified
24-25. Done
26. Done +

we can probably remove the assertWaitOnAjaxRequest() call unless it needs to be there for some reason...

I've addressed a few intermittent test failures by adding assertWaitOnAjaxRequest() after every dialog open.
27. Like item 23, I addressed this but used a different approach - the (contains()) selector.
28. Done
29. Once I figured it out, I added the .layout-builder-components-table__block-label class to the table cell to make things clearer and the selector less complex.
30-31. Done
32.

Any reason not to use $handle->keyPress()?

$handle->keyPress() does not successfully move the blocks. @tedbow wrote that part of the test and may have additional details, but switching to keyPress() definitely didn't work when I switched to it.
33. Done
34. assertEmpty() was confirming the amount of blocks in the section match the number of blocks expected. I removed this and instead checking with $this->assertCount(count($expected_block_selectors), $blocks); as I think that will be clearer without having to explain in a comment. Also renamed $block_selectors to $expected_block_selectors to make it clearer.
35. Done.

Switching to Needs Review to run tests on the patch, but there is still not-fully-addressed feedback that would benefit from UX discussion:

  1. #106.8
  2. #106.11
  3. #106.15
  4. #107.1
  5. #107.2 ( I brought up earlier in the issue and there's a response in #75.5 - probably worth revisiting if I'm not the only once noticing it.
bnjmnm’s picture

FileSize
40.35 KB

Screenshot of change based on feedback from #106.11 to get UX input (can't tell if its an improvement or not...)

bnjmnm’s picture

Based on a recommendation from @DyanneNova #106.11 is taken care of by wrapping the additional label content in a visually hidden span.

The items still requiring UX and/or design input are

  1. #106.8
  2. #106.15
  3. #107.1
  4. #107.2
xjm’s picture

Way trivial nit:

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -121,3 +121,25 @@
+}
+/**

I think there's a missing newline between the rules here.

bnjmnm’s picture

#107.2 should be the last thing that requires design input.
For the other items:
Addressed #107.1 by un-hiding the "You have unsaved changes" message. While some (including myself) have discussed the merits of hiding this, it does change default/expected tabledrag behavior. There isn't strong enough support for this change to justify altering default behavior.
(This also addresses #111 since the rule in question has been removed.)

#106.8

the label "Section: N" could probably just be "Section N", no?

In fact, that brings something else to mind -- does the use of optgroups here have accessibility implications? How will these options sound to a screen reader?

Not worth changing unless specific concerns are brought up. I like the Section: sectionnumber, Region : regionname naming convention as it helps make the Container: item relationship clear. I also tested with a screenreader and which had no problems with the options.

#106.15

Is the word "block" redundant here, from an accessibility standpoint?

Unless specific concerns are brought up, this should be fine. The column name "Blocks" is not read by screenreaders, so there's no repetition in using "Blocks" in the element label.

AaronMcHale’s picture

Not long ago in Slack, someone asked if Sections have the ability to specify a name/description, which got me thinking that perhaps if (in a follow-up issue) we introduced the concept of a standard Section name/title/description optional field, perhaps we could use that to enhance the Section/Region selection list introduced here.

I can definitely see why some people might find it easier to locate the correct Section and Region if the Sections were named in a way that made sense to that particular page, rather than simply just "Section 1", "Section 2", etc. Plus if the Section name/title/description value was displayed above each Section and used when describing each Section to screen readers and other assistive technology then that might also further improve the accessibility of Layout Builder.

lauriii’s picture

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -121,3 +121,18 @@
+/**
+ * This rule ensures the row weight dropdowns in the Move Block dialog maintain
+ * the background color of their container when they are hovered over or are
+ * inside the active row.
+ */
+#drupal-off-canvas .layout-builder-components-table__row .form-item {
+  background-color: transparent;
+}

This sounds like a bug to me. Should we open a follow-up for solving this in a broader scope?

xjm’s picture

Issue tags: +Needs followup

Yep, let's file a followup for #114.

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
50.39 KB

I tested this manually. A couple things:

  1. The table header or caption or whatever can be entirely visually hidden. It's important semantic info for screen readers etc., but confusing graphically.

    Screenshot showing the table metadata in the sidebar form
     

  2. When I select "Move" for a block, the sidebar opens, but there's no indication in the UI which block I just selected to move. It would be helpful to have a border like those added in #2994909: Highlight active element while working with dialogs in Layout Builder. (We might want to wait until that issue lands.)
     

  3. I was expecting there to be some visual indication of what "Section: 2, Region: Second" was in the main form. There doesn't seem to be any. At a minimum, using this form should expose region and section labels; ideally, I think selecting a region from the dropdown would also highlight the targeted region on the page or even update it showing the block there.
     

  4. What if I start moving the block but then decide I don't want to? I'm guessing the correct way is to navigate to the [ x ] that closes the dialog, but it wasn't immediately obvious to me.
     

  5. When I reorder rows, tabledrag adds a "You have unsaved changes" message and an indicator marking the row that had its weight updated. However, moving the block to a different region does not add this message or indicator. I think it needs to say "You have unsaved changes" in both cases.
     

  6. I moved the original mockups to #2920006: Research accessibility of drag-and-drop grid interfaces. so that we still have those designs when we work on the next-gen version, since that's not what this MVP will do. :)

Thanks!

xjm’s picture

One more thing: Maybe the sidebar title should give me some indication of what block is being moved? E.g.: "Move 'Body' field block". (Similar to the labels used in #2959493: Allow Layout Builder live previews to be toggled to allow easier drag-and-drop.)

tim.plunkett’s picture

#116

1)
Agreed!

2)
This deserves a followup, it's a cool idea

3)
This is what is fixed by #3041143: Add ARIA group roles to Layout Builder UI
I think the "ideal" part of this also deserves a followup, maybe the same as 2? Maybe not

4)
Correct

5)
That's from tabledrag, I guess we can emulate that here.

6)
Thanks!

xjm’s picture

Not a full review; got about halfway through the patch.

  1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
    @@ -110,6 +110,20 @@ layout_builder.update_block:
    +    _permission: 'configure any layout'
    

    This will need to be updated once #2914486: Add granular permissions to the Layout Builder lands, I think.

  2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
    @@ -251,6 +251,13 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
    +              // @see layout_builder.links.contextual.yml
    

    I suspect that @see won't work on api.d.o since there's no filepath, but I'm not 100% sure (and it's difficult to test without committing code on d.o). :P

  3. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +   * MoveBlockForm constructor.
    

    I think these are usually "Constructs a Foo", but w/e. :)

  4. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +   * @param int $delta
    +   *   The delta of the section.
    +   * @param string $region
    +   *   The region of the block.
    

    Are these the original delta and region, or the updated ones?

  5. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +  public function buildForm(array $form, FormStateInterface $form_state, SectionStorageInterface $section_storage = NULL, $delta = NULL, $region = NULL, $uuid = NULL) {
    ...
    +      if (is_null($parameter)) {
    +        throw new \InvalidArgumentException('MoveBlockForm requires all parameters.');
    +      }
    

    Why do we make them optional params but then throw exceptions?

  6. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +    // @todo add data-layout-builder-target-highlight-id attribute in
    

    Nit: "Add" should be capitalized.

  7. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +    /*
    +     * $this->region and $this->delta are where the block is currently placed.
    +     * $selected_region and $selected_delta are the values from this form
    +     * specifying where the block should be moved to.
    +     */
    

    Wrong comment format.

  8. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +    $caption = $this->t('Blocks<span class="visually-hidden"> in Section: @section, Region: @region</span>',
    

    So yeah, the whole thing can be visually hidden here. That also means we can move the span outside the string so translators don't have to deal with it.

    Also, way nitpicky, but I'd usually put the first param on its own line as well when using a multiline format like this.

  9. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +        $this->t('Label'),
    

    Maybe "Block label" would be clearer?

  10. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
    @@ -0,0 +1,310 @@
    +    // If the component is not in this region add it to the listed components.
    

    Nit: missing comma after "region".

xjm’s picture

Re: #118, I'm not sure points 2 and 3 are followup material. The form is very disorienting as-is for a sighted user. (Remember that keyboard navigation is not just for screen readers.)

tedbow’s picture

I'm not sure points 2 and 3 are followup material. The form is very disorienting as-is for a sighted user. (Remember that keyboard navigation is not just for screen readers.)

I just want to point out that I have presented this form at least twice(maybe 3x I could check) to the UX meeting and never got the feedback that was in anyway "disorienting". Also most of people at the meeting were less familiar with the layout builder so my guess would be they would more disoriented if anything.

@xjm obviously your experience actually using form is more first hand than me presenting to a group of people but I do think the feedback UX meeting is value as check against more people perception and reaction of the form.

tedbow’s picture

Re #116
For a while this patch had the exact same Javascript as #2994909: Highlight active element while working with dialogs in Layout Builder
It was copied from there

But in #92 @bnjmnm switch the js logic for this todo

+++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
@@ -0,0 +1,310 @@
+    // @todo add data-layout-builder-target-highlight-id attribute in
+    //   https://www.drupal.org/node/2994909.

Because at the time #2994909 was had got more complicated and was dependent on another issue.

Once the #2994909 is committed only this change would be necessary and the block being moved would be highlighted.

We could copy the JS back into this issue if needed. But the other issue has comprehensive test coverage for the highlights also and trait for making the highlight ids.

xjm’s picture

Let's get the highlight issue in first (it's close) and then I think we can just extend it in this patch for the move operation.

lauriii’s picture

#116.1 I agree that we should remove the caption. However, I don't think we should use the caption here since it only describes the contents of the table, without including the weight toggle link. My suggestion is to move the text from the caption to aria-label attribute in the wrapper element.

It seems like the table heading is aligned to left on some browsers, but not on all. We should define some rule to make that consistent so that the heading is always aligned to left.

bnjmnm’s picture

Followup for #114 created: #3042127: Off-canvas tabledrag styles do not fully account for "weight" column.
Holding off on removing the "Needs followup" tag until I'm a little more certain there aren't additional followups needed.

bnjmnm’s picture

Issue tags: -Needs followup

#116.5 Will be re-hiding the "unsaved changes" message in the next patch. This is based on a discussion with @xjm, @timplunkett and @tedbow - the concerns that motivated this are summarized in the followup issue #3042228: "Unsaved Changes" messaging in Off-Canvas dialogs needs to be consistent. . The patch will include a @todo referencing this.

Wim Leers’s picture

Let's get the highlight issue in first (it's close) and then I think we can just extend it in this patch for the move operation.

#2994909: Highlight active element while working with dialogs in Layout Builder landed!

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
58.57 KB
13.38 KB
31.82 KB

This needs some design input

This patch makes changes to the Layout Builder UI to address #116.3 -so @lauriii (or anyone interested) should provide design input on these changes.

When the move block form is open, the UI will display labels specifying each section and region. Only minimal styling was added, and the labels are positioned in the first place that came to mind. The section labels are in the same place as the "Configure section" link. existing "Configure section" links have the section number visible when the move block form is open, and region labels appear at the top of a region. For region labels, I'm unsure if there are additional considerations for regions on custom templates where they don't necessarily appear at the top of a section

Other things covered by this patch

#114 created followup #3042127: Off-canvas tabledrag styles do not fully account for "weight" column. and added @todo to the CSS rule that should be unnecessary once the issue is fixed.
#116.1 The caption is removed and the info is available to screen readers by implementing the suggestion in #124
#116.2 After a reroll #2994909: Highlight active element while working with dialogs in Layout Builder is included and I updated MoveBlockForm to outline the block being moved.
#116.3 Sections and Regions are now labeled when the move block form is open - details in the beginning of this comment.
#116.4 No action needed
#116.5 Re-hiding the "unsaved changes" message, accompanied by a @todo to the followup #3042228: "Unsaved Changes" messaging in Off-Canvas dialogs needs to be consistent. . The followup explains why it was hidden.
#116.6 No action needed
#117 The dialog title is now dynamic, much like the "remove block" dialog is. The name of the block being moved is in the title bar.
#119.1 Added @todo to #2914486: Add granular permissions to the Layout Builder
#119.2 confirmed @see works for config files without filepath from this doc, which has @see filter.permissions.yml in the docblock for the class and is correctly listed in "See Also"
#119.3 Changed comment to match that more common format.
#119.4 Added "original" to relevant comments to make original-location and new-location of blocks clearer. Adam asked for similar clarification elsewhere in the file so it's probably good to make this very obvious anywhere possible.
#119.5 Since buildForm() is defined in FormInterface with two arguments, any additional arguments beyond the first two need to be optional or the form will not load. The exceptions-on-null check was added since those arguments are necessary, but can potentially be null due to FormInterface.
#119.6 Capitalization nit fixed.
#119.7 Comment format corrected.
#119.8 The caption is addressed in my comment about 116.1. Changed the formatting to one line after a regex search revealed that's how other similar uses of t() appear in core.
#119.9 Changing to "Block Label" certainly takes care of any ambiguity caused by removing the caption, so this has been changed.
#119.10 Comma added.

lauriii’s picture

Status: Needs review » Needs work

I have to admit that until today, I had completely misunderstood how this patch is trying to solve this problem. The recent UI improvements helped me to understand that the edit dialog targets a block, not a region, and that the region selection moves the block to another region, rather than selects another region to edit. Huge +1 from me for these UI improvements 👏

  1. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -141,3 +141,47 @@
    +.layout-builder-move-block-form-show,
    +.layout-builder-move-block-form-show-inline {
    +  display: none;
    +}
    +
    +[data-layout-builder-wrapper='move-block-form']  .layout-builder-move-block-form-show {
    +  display: block;
    +}
    +
    ...
    +  display: inline;
    +}
    

    Do you think we could refactor this to use classes instead? Something like:

    .layout-builder__region-label,
    .layout-builder__section-label {
      display: none;
    }
    .layout-builder--move-blocks-active .layout-builder__region-label {
      display: block;
    }
    .layout-builder--move-blocks-active .layout-builder__section-label {
      display: inline;
    }
    
  2. +++ b/core/modules/layout_builder/css/layout-builder.css
    @@ -141,3 +141,47 @@
    +  padding: .5em;
    

    Nit: s/.5/0.5

  3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
    @@ -297,6 +304,22 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
    +        '#markup' => $this->t('Region: @region', ['@region' => $info['label']]),
    

    This information is already available for screen readers, and adding it for sighted users means that this information will be read twice by screen readers. We could use aria-labelledby to avoid this problem.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
31.93 KB

Re: #129

  1. Do you think we could refactor this to use classes instead?

    I like the classnames for the region and section labels and changed those, but want to push back and keep the [data-layout-builder-wrapper='move-block-form'] selector approach. This approach was chosen to make it easier for other dialogs to add UI wrappers in the future. This allows us to reset the wrappers on dialog open/close with

    $('[data-layout-builder-wrapper]').removeAttr(
            'data-layout-builder-wrapper',
          )

    , regardless of the wrapper's value. Without having to create additional logic for every new wrapper value. If there are concerns about this approach let me know.

  2. Padding nit fixed
  3. This information is already available for screen readers, and adding it for sighted users means that this information will be read twice by screen readers. We could use aria-labelledby to avoid this problem.

    Very good catch. I addressed this by adding aria-hidden="true" to the visible label instead of using aria-labelledby because the aria-label for regions specified region and section, while the newly added visual region label only specifies region since it is already in close visual proximity to the section label. If it is preferable for the visual region label to include section information, this can be changed to use the aria-labelledby approach

lauriii’s picture

#130.1 I think it could be a good idea to use the data attribute if this becomes a repeating pattern. To keep our CSS as consistent as possible, would you be fine with using the standard BEM class names for now? We could always rework our CSS to use the data attribute instead if it seems like it could simplify our code.

#130.2 & #130.3 👍

bnjmnm’s picture

Re #131 This makes sense. If refactoring is needed later it won't be particularly difficult, so there's no need to deviate from the BEM class names right now. The selectors now match what was suggested.

xjm’s picture

Thanks @bnjmnm and @lauriii; this is getting much better.

+++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
@@ -0,0 +1,333 @@
+  public function buildForm(array $form, FormStateInterface $form_state, SectionStorageInterface $section_storage = NULL, $delta = NULL, $region = NULL, $uuid = NULL) {
...
+      if (is_null($parameter)) {
+        throw new \InvalidArgumentException('MoveBlockForm requires all parameters.');
+      }

I don't see a response to my question from #116: Why are we accepting null arguments but throwing an exception if a parameter is missing? This is a new form, so we don't need to provide BC for anything, right?

bnjmnm’s picture

Re #133 this was answered in #128, but there were tons of things answered there so here's a recap so you don't have to dig though a pile of laundry:

Since buildForm() is defined in FormInterface with two arguments, any additional arguments beyond the first two need to be optional or the form will not load. The exceptions-on-null check was added since those arguments are necessary, but can potentially be null due to the requirements of FormInterface.

tim.plunkett’s picture

That NULL trick is true of *every* form implementation, and is rather unfortunate. But nowhere else in core that I know of do we have this exception (it's a nice approach!) so I think it's fair to remove if there's more pushback

bnjmnm’s picture

I'm certainly not attached to the exception checking in the form. Just for context: @phenaproxima suggested adding this in #106.7 and my response was

It's certainly possible a contrib module might want to use the form and this would be of use. Probably can't hurt to check this, it's been added.

I did like the suggestion, but if there's a preference to removing it, it's pretty easy to let go of since no other form does it

DyanneNova’s picture

The design in #128 looks good to me! The alignment of the text is consistent with the existing design components.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for all of the excellent work here! It's so great to have this.

  • webchick committed a73bf04 on 8.7.x
    Issue #2995689 by tedbow, bnjmnm, lauriii, bendeguz.csirmaz,...

  • webchick committed 406f483 on 8.8.x
    Issue #2995689 by tedbow, bnjmnm, lauriii, bendeguz.csirmaz,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, tested the crap out of this manually, and was able to use the keyboard alone to enter the Layout Builder, to add a section, to add a block, to move the block to another column within the section, to reorder the block within the section, to configure a block, and to delete a block. Everything worked GREAT! :D

However, two issues were immediately apparent:

1. The focus kept skipping back to the menu bar after completing one of the above actions, resulting in an extra 20+ tabs each time, and this kiiiiiiinda made me want to toss my laptop across the room in a rage. :P A follow-up has been filed for this: #3042107: Layout Builder actions are inconvenient to access via keyboard navigation I plan to check with the a11y team on whether it's best classified "should have" vs. "must have" (it should be noted however that what's here, despite frustrations, is still vastly more usable than the current Block UI, which I can't seem to even get to the "Place block" buttons with the keyboard right now... eek! Can't immediately find an existing issue about this, but yikes).

2. Since I'm a sighted user, I was kind of wishing for a live preview of what changes I was making in the Settings Tray as I was shifting blocks around. This is a "pre-existing condition" with Settings Tray though, and nothing to do with this patch/module. (Also can't find an existing issue about that right now, but it's firmly in "nice to have" territory.)

Looks like we have front-end framework manager sign-off in #91, so let's get 'er done!

Committed and pushed to 8.8.x, and since this is against an experimental module, also cherry-picked to 8.7.x. Thanks!

xjm’s picture

Yay! ⌨️

Just to document after the fact, #134 makes sense to me and I agree the exception is a good addition. Thanks!

xjm’s picture

Issue tags: +Needs followup

Ah, we still need a followup or two for #141. They'd be:

  1. Targeted fix: When selecting a new region the move block form, highlight or otherwise indicate the new region in the main form
  2. General (more difficult) improvement: Provide live updates to the LB UI when making updates in the sidebar tray

Tim indicated point 2 might already have an issue. Point 1 needs a followup filed though.

tim.plunkett’s picture

#143.2, I misremembered the issue I was thinking of. It was similar but different: #2999580: Make blocks appear in the layout builder without saving formatter settings
That one is about not opening the block form by default when adding a block, so that the live update is more immediate.

Status: Fixed » Closed (fixed)

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