🇺🇸United States @sonfd

Needs review and credits.

sonfd → created an issue. See original summary → .

Needs review and credits

sonfd → created an issue.

Needs review and credits.

sonfd → created an issue.

Needs review and credits.

I could not track down a d.o username for Akhil Babu

sonfd → created an issue.

@mortona2k, I was just coming back to this after sleeping on this to say basically the same thing! Except I think I'd recommend the additional checkbox be "Expand items in the active trail" and we can default it to checked.

Actually, scratch pretty much everything I just said.

I'm seeing in MenuLinkTree::getCurrentRouteMenuTreeParameters(), we are explicitly expanding links in the active trail:

/**
   * {@inheritdoc}
   */
  public function getCurrentRouteMenuTreeParameters($menu_name) {
    $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);

    $parameters = new MenuTreeParameters();
    $parameters->setActiveTrail($active_trail)
      // We want links in the active trail to be expanded.
      ->addExpandedParents($active_trail)
      // We marked the links in the active trail to be expanded, but we also
      // want their descendants that have the "expanded" flag enabled to be
      // expanded.
      ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));

    return $parameters;
  }

If we're going to do something here, I think we have to do it there.

I tried to update the issue summary to be a little more clear. I do think this is a duplicate of ✨ Menu item checkbox "Show as expanded" not working properly Needs work , but I think the approach in this issue is the correct way to go so I'd recommend closing the other one.

To @aaronchristian's point, I think this patch is only complete with a change to the template file. Maybe I'm wrong, but I think we shouldn't even be building the subtrees of a menu item if it's not expanded. And if we do that, we won't need a change to a template file. I'll take a stab at that in an MR now.

I think the approach in Issue 3424786 → solves this problem in a more complete way.

I think the approach in the MR is more complete than the patch in #6. Also the patch contains a number of whitespace issues. MR !18 is the same as MR !15, but on top of the 2.x branch which seems to be the current development branch.

I'm concerned that the related issue referenced in #6 has been open for 8 years.

sonfd → made their first commit to this issue’s fork.

@phenaproxima - I think handling for a recipe requirement already being in composer.json is accounted for ~ line 116 of RecipeUnpacker and works as you want, though I didn't check on tests.

The MR adds the extra classes, but I am finding it confusing that that we're using menu__item-* and menu__submenu-item-* classes. I think we'd be better off with just menu__item-* menu__item-*--level-X classes on all levels but I think that could break some existing styles out there.

Leaving this unresolved for now.

sonfd → made their first commit to this issue’s fork.

sonfd → made their first commit to this issue’s fork.

Looks good to me. (And sorry for earlier mistake.)

You're right about expectException(). I got output like 1 Test, 5 assertions so I thought they were working, but they were not.

sonfd → made their first commit to this issue’s fork.

Tests seem to be failing downstream, but MR !11679 takes the approach outlined in #30.

I started work on the approach proposed in #30 in the 3365328-simple-config-array branch.

It works right now, but the big thing I'm not sure about is: On line ~92, I don't like the weird special handling here to return $value['values'] if it exists. When I was returning the whole value array, array_push() was throwing an error about not allowing unrecognized named variables. My thought is to maybe define the expected parameters in the deriver and then we know exactly what we need to pass to the functions. I think I'll try that next.

sonfd → changed the visibility of the branch 3365328-simple-config-add to hidden.

MR sets the aria-label to "Menu" and defaults the visible label (if enabled) to "Menu".

Updated the button label to "Submenu for LINK_TITLE" and set the default value for the visible label (if enabled) to the same.

I have been thinking about this more and I'm convinced the solution is a recipe. The recipe can:

1. Installs Disclosure Menu and Block Breakpoint →
2. Creates a "mobile" menu block that's only shown on smaller screens.
3. Creates a "desktop" menu block that's only shown on larger screens.

We can then link to the recipe from the project page.

Leaving this postponed for now. In the meantime, this thread can serve as the documentation.

I agree that More {{ item.title }} pages is a verbose, despite it matching the Example Disclosure Navigation Menu with Top-Level Links pattern on w3.org.

It looks like Olivero's submenu toggle is using "Toggle sub-navigation". I don't like that at all. I don't really like "sub-navigation" or "submenu," both feel very jargony to me. I'm inclined to recommend {{ item.title }} pages, but I worry that will be verbose too unless the link title is very short. Something like: "Admission & Aid pages" feels like a lot and considering screen readers would process the menu like "Admission & Aid, Admission & Aid pages" which really feels like a mouthful.

I wonder if More {{ item.title }} is a reasonable compromise even though we'd need to swap to Less {{ item.title }} while it was toggled open.

You make a good point. I'm looking at Olivero's menu toggle and they're using "Toggle Menu" as the aria-label and "Menu" as the visible label. I don't think that the word "Toggle" adds anything useful so I'm good with using just "Menu" for the aria-label and the default value for the visible text.

I think this case matches the Disclosure (Show/Hide) Pattern and that pattern doesn't require the aria-haspopup attribute.

@phenaproxima @thejimbirch and I spoke about unpacking in the contrib room at DrupalCon Atlanta. (Some initial discussion is in this slack thread.) Our discussion was about whether recipes should be auto-unpacked and when should they be auto-unpacked. I'm documenting here while it's fresh.

Our starting point was that recipes will be (unless the drupal-unpack plugin is disabled) auto-unpacked when the recipe is required with composer require. This creates two scenarios that we're not sure about:

1. If I `composer require` a recipe and it was the wrong one, I can't just `composer remove` it to get back to the starting point because it's unpacked all of its dependencies into my composer.json.
2. Since recipes can accept input, it's a valid use-case to have a "Content Type generator" recipe, or similar. I may want this recipe to stick around in my site. If it's not managed by composer, it may not stick around in my project and won't be easily updatable. And since recipes are normally managed by composer, they're probably not part of my git repository.

For #1, I think our consensus is that this scenario is not ideal, but also maybe not that big of a problem. In many situations, e.g. while using Project Browser to apply a recipe, users will have no awareness of their composer.json and having extra dependencies won't cause them any problems. And there's been talk of a composer plugin to remove orphaned dependencies that could further reduce the relevance of this problem.

For #2, one idea is that unpacking recipes should be a game-time decision. Maybe by default recipes should be auto-unpacked on require, but something like composer require drupal/some-recipe --no-unpack would prevent the recipe from being unpacked. (But according to some, it may not be possible to add this flag to composer require.)

I took a pass at updating labels and descriptions of the options under the Menu Levels fieldset. I think these might be an improvement, but I think they're still not ideal. See MR !17.

Changes:
I added a description to the "Menu Levels" fieldset:

These options allow you to limit which menu links are available in your block.
Example: Set "First menu level" to 1 and "Total menu levels" to 2, and check "Force all menu items with children to be expandable". This will result in a menu that shows the top level of your menu by default and all menu items will have disclosure buttons to show their child menu items.

I updated the label and description for the "Initial visibility level" field.
Label:
First menu level
Description:
The highest menu level to display. This menu is only visible if the current page is at or below this level.
If you are unsure, keep this set to "1".

I updated the label and description for the "Number of levels to display" field.
Label:
Total menu levels
Description:
The maximum number of menu levels to include in this block.
All submenu items will be initially hidden behind disclosure buttons.

I updated the label and description for the "Expand all menu links" field.
Label:
Force all menu items with children to be expandable
Description:
Without this checked, only menu items that are configured to "Show as expanded" will be expandable with disclosure buttons.
If you are unsure, keep this checked.

sonfd → changed the visibility of the branch 2.1.x to hidden.

sonfd → changed the visibility of the branch 3419021-expand-all to hidden.

@cedewey - I did you dirty. Thank you for your flagging, but there is no module configuration page, the only configuration happens on a per block basis.

For some reason, it seems like we need to manually add the contextual link for the menu. I am not sure why. I looked at the Menu Block module and they're doing it there too so I copied their approach. See Drupal\menu_block\Plugin\Block:build() ~ line 432.

sonfd → made their first commit to this issue’s fork.

I like this idea a lot.

One thing that I think we want to consider is that not all sites have just 2 menus (one for desktop, one for mobile). It may be that they have 3 (mobile, tablet, desktop) or it may be that they have either of those scenarios, but different groups of menus for multiple different sections of a site.

I wonder if there's a way to use a Block visibility condition to achieve the multiple blocks showing / hiding.

And definitely, to @ressa's point, I love the idea of documenting how to achieve this somewhere in the module, probably the README.md. And maybe there's even a way to use a recipe to do it all for us.

Here's a code-only patch for use with composer and 10.5.

MR !15 creates a new template for the subnav toggle icon, i.e. the SVG and uses that instead of defining the SVG within the menu template. (This does nothing to address a template for the menu toggle as a whole.)

sonfd → made their first commit to this issue’s fork.

I found my way here because I was experiencing this issue even though I had 2.1-beta1 installed. It turns out that the site I'm working on had patch #2 applied. Patch #2 is literally just unfixing this issue. I am hiding patch #2 - don't use patch #2! (I think it may be that it was needed when using 2.1 with an earlier version of Drupal.)

I opened a new issue for this and created an MR with r.van.doorn's patch here: 🐛 Can't edit content within a layout Active .

MR !7 is the same as the patch from comment 7 🐛 CKEditor behavior buggy inside layout Fixed by @r.van.doorn on 🐛 CKEditor behavior buggy inside layout Fixed

@maintainers -

Please note contributions from r.van.doorn → and helianthropy → on the related issue.

sonfd → created an issue.

Here's a patch that applies against 6.0.5 (does not include tests)

Patch #12 is the same as MR !109 and can be used with composer. But since we are using MRs here, hiding patches as that is the practice.

This is still an issue on 3.x

I think that, as stated by folks earlier, this issue is because the first time the facet is built, it is built as an empty element, literally an empty array. This is because the facet values are only known after the view query has been executed, but the form is initially built before the query is executed. As a result, facets adds an empty element and then updates the element later.

I'm seeing that, with Select2 enabled, an error is thrown: "The submitted value [value] in the [name] element is not allowed." This seems to be happening because the facet is being validated while it's still just the empty element. From what I can tell, it's really just the facets options that need to be added later. MR !286 updates the FacetsFilter plugin to always build a select element (though initially without options) and then the options are added later. This is basically just a reorganization of the code in FacetsFilter::valueForm().

With the changes in MR !286, this is working for me now.

I did try to just modify the code with something like this at the beginning:

$form['value'] = [];
$form['value']['#process'] = ['facets_exposed_filters_remove_validation'];

because I do think that is really the most important part for resolving this specific error. But then other errors starting popping up, it made it seem like Views really did not like an empty element. This led me to the approach in the MR.

sonfd → made their first commit to this issue’s fork.

Just noting here that I am experiencing this issue using the exact steps as in the issue summary. I only see this issue when Select2 is chosen via BEF as the widget type for my field. When I use a default multi-select widget, it works fine without error.

I have been referencing the Block link cards docs from W3 design system for this sort of thing. The approach is described in the Block Links: The Search for a Perfect Solution CSS tricks article.

But I'm not exactly sure how we would control that approach (Method 4 in the article) through the UI. Maybe we could assume the first link is the main link? (That's what I have been doing :shrug:)

sonfd → created an issue.

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

Attaching patch file with changes from MR !10945

Patch in #127 looks like it includes a lot of additional code, hiding.

sonfd → changed the visibility of the branch 10.5.x to hidden.

sonfd → made their first commit to this issue’s fork.

sonfd → created an issue.

sonfd → created an issue.

I found an approach that seems to work as expected.

toggle.addEventListener('blur', (event) => {
  if (!menu.contains(event.relatedTarget)) hideMenu(toggle, menu);
    if (!menu.contains(event.relatedTarget)) {
      setTimeout(() => hideMenu(toggle, menu), 0);
    }
 });

The important change is that instead of calling hideMenu(toggle, menu) immediately from the blur event handler, instead we call setTimeout(() => hideMenu(toggle, menu), 0); so hiding the menu happens "during the next event cycle."

In most cases, this difference is not relevant, but it is necessary when using a vertical menu style. Without this delay, if a user has toggled open a submenu and then clicks the toggle button of a later submenu, e.g. the next one, the open submenu will close, but the second submenu will not open. This is because the blur event (which closes the first submenu) triggers before the click event (which opens the second submenu) and the click event will not fire if the user's cursor is no longer over the element they originally clicked (in our example scenario, the user's cursor is no longer over the original element because the closing of the first submenu has shifted our element higher on the screen). By using setTimeout with a delay of 0, we can trigger hiding the menu to happen "during the next event cycle" (see https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#delay) which occurs after the click event is resolved.

Swapping to focusout did not resolve anything, the same issue persists.

Taking a look at this - it looks like our issue has to do with the order of the blur / click events.

If I google, "javascript blur click event order", I get an AI explanation with:

The order of events when a user interacts with an element that has both blur and click event handlers is as follows:

1. mousedown: This event fires when the mouse button is pressed down on the element.
2. blur: If the element loses focus (e.g., by clicking outside of it), the blur event fires.
3. mouseup: This event fires when the mouse button is released.
4. click: This event fires after the mouseup event if the mouse button was both pressed and released over the same element.

I'm not easily finding the source of this info, but in our case, if we open one submenu, for example the first submenu in our vertical menu, and then click the second submenu's toggle, only the first submenu is closed, the second submenu is not opened. I believe this is because of this event ordering and because our mouse is no longer hovering over the originally clicked toggle by the time the click event tries to fire.

I can confirm that the click event never fires with some console.logs in toggleMenu, hideMenu, and showMenu functions.

However, if I change the click event to trigger on mousedown, all works as expected. This further supports the theory that the issue is that the mouse is not hovering on the same item that was clicked when the click event attempts to fire.

Also, if I comment the toggle's blur event, my second submenu opens, but of course the first does not close.

I think we can change the toggle's blur event to achieve a similar effect, but without this issue. Perhaps by just substituting with focusout, but I will look into this more tomorrow.

Hiding irrelevant patch - it looks like it was added to the wrong issue.

I believe this issue will only occur when both of the following conditions are met:

1. Initial items is set to a number lower than Items to display
2. The total number of results is great than Initial items, but less than Items to display

And I believe this is a limitation of core's Pager class. Upon initialization, it calculates the total number of pages based only on the items per page, it does not consider a different initial amount visible. See: Pager:setTotalPages() which is called from the constructor.

This is a shame, I would really like to show X initially, then show all after clicking "Show more"

MR !27 addresses this.

But, I believe I see why this was done via submit handler now - if a user logs in via any method other than user_login_form, those timestamps won't be accurate, I believe they'd show the current login time. So rather than address that issue (I don't know how we would address that issue), we are only displaying them if logging in via user_login_form.

As a result, I think this should probably be Closed (works as designed), but I'll leave it to a maintainer to make that decision.

sonfd → created an issue.