Make menu trail behaviour in SystemMenuBlock optional

MR 12375 is up.
I added tests for the active trail functionality, but I didn't do one for the #states handling in the UI. Is that needed?

Also, I wonder if there needs to be either help text in the description or more conditional logic so that the new setting isn't applied when the start level is configured to be greater than 1? Effectively it creates an empty menu if the start level is greater than 1, the menu is set to expand all items, and ignoring the active trail is enabled.

    // For menu blocks with start level greater than 1, only show menu items
    // from the current active trail. Adjust the root according to the current
    // position in the menu in order to determine if we can show the subtree.

Added more #states handling and the config schema validation constraint.
Also added functionaljavascript test for the form.

Yeah, I went with the negative to make the upgrade path easier (NULL being the same as FALSE), despite that it makes the wording maybe a bit awkward.

Though maybe setting the value of the new property in defaultConfiguration() to TRUE makes that concern go away?

Then the #states could only show the levels etc. options if it's checked, otherwise hidden. However that would complicate the upgrade path because we'd need to set it for existing config using those options to prevent them being hidden in the form when it's edited.

Thought about this some more. If we have the new "track active trail" (wording TBD) field control the level and expand_all_items, and assuming "level" is set to 2, or expand_all_items unchecked, then when going from "track active trail" checked to unchecked, those fields would be hidden. And the result might be a bit unexpected to the user, even if help text is provided. We'd probably also have to disable those fields when "track active trail" is unchecked, because #states can't change field values. Then in the form submit handler, we'd have to interpret those NULL values as level = 1 and expand_all_items as TRUE.

Made the text changes and pushed. Thinking about this now, though:

When this is checked, the menu will have the same markup on every page. Otherwise the current page's position in the menu structure will be calculated and an 'active' class added to relevant menu links if it is found.

The class added in the core theme templates is menu-item--active-trail. There's also an is-active class added to a menu link if it is the current page, but that is added through JS. Still I wonder if "active" is not informative enough? Also, I'm not sure what the antecedent to the final "it" in the description text is.

My attempt/suggestion for an edit:

When this is checked, the menu will have the same markup on every page. Otherwise, the current page's position in the menu structure will be calculated and an "active-trail" class added to menu links in the current page's menu hierarchy.

MR updated per #12.

Yes, the issue with LB overrides mentioned in #3521221: Block plugins need to be able to update their settings in multiple different storage implementations, among others, is why I wanted to avoid doing an upgrade path. I think it's okay to have the config key introduced over time on block saves?

For block entities specifically, I wonder if we can change get() so that calling get('settings') will merge in configuration from the plugin collection. That way additions in defaultConfiguration() from the plugin will be picked up in the loaded block entity after a cache rebuild. That doesn't really change anything here, but it could maybe apply to other situations?

The issue with this is how often do these blocks get resaved?

The issue with this is how often do these blocks get resaved?

Here, it doesn't much matter because NULL/undefined is the same as FALSE for the new property, and FALSE is always a valid value.

It'd be an issue for any new block property that doesn't work like that. What I suggested in #15 could help address that, but if there are constraints where the default value is not valid with existing configuration, that would be another problem.

A just-in-time API could maybe do address above with an optional callback parameter. If it's not passed, then the default API behavior is to merge in default configuration from the plugin, otherwise the callback function would handle any logic.

I was more concerned about how long we'd need to handle the missing config.

After looking at the actual code it's not really a huge concern.

@catch asked my opinion on the nullable approach. I think it is a good one because the new key is not added during a config import so you're not getting unexpected changes. The one thing that gives me pause is how are we going to tell any modules or recipes that supply this config that they need to update. I.e. will we leave tis key as optional...

Updated MR per feedback and adjusted tests.

Updated the IS proposed resolution to match the work done so far.

Could we update the some before/after screenshots of the new config form. May need UX sign off.

Think it would need a CR?

CR: https://www.drupal.org/node/3533514

Note: https://www.drupal.org/project/menu_trail_by_path adds this as a third party setting per menu as opposed to the block, so the context is still there, but by disabling it for a menu link the footer it will always have the same empty value and as a result, not vary.

This overlaps and you can then end up with a somewhat confusing situation where you enable it on the menu but not the block. Or the other way round, where it's enabled on the block but actually disabled on the menu. That means the block can force disabling it but can't force enable it.

I think that's OK and it's probably not worth to reconsider and move to the menu (and support more than one state).

Usability review

We discussed this issue at #3536866: Drupal Usability Meeting 2025-07-25. That issue will have a link to a recording of the meeting. (See the second half of the recording.)

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell. I am adding issue credit for them.

We all agreed that the option should be reversed. The current label (title), "Show the same menu markup on every page", does not give enough information, so the user has to read the description (help text) to figure out what it means. It is sort of like trying to describe what an option does not do instead of describing what it does.

We did not have time to recommend replacement text, but start with something like

Add the active-trail class

Maybe just use that and keep further explanation in the help text. In the help text, mention the pros and cons: the theme may style the active-trail class, but it has a performance impact.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

This issue reminds me of some work I did several years ago. I even gave a few presentations on it. (The most recent is https://slides.benjifisher.info/caching-midcamp-2020.html#/strategy. I have to do something about the TLS certificate.) At the time, it did not occur to me to make the same change in Drupal core.

Why not use JavaScript to add the active-trail class? It is a lot simpler: JavaScript is really good at traversing the DOM, and the current PHP code is a little messy.

But code simplification is just a side benefit. The real advantage is that you can cache the navigation and still have the active-trail class.

I see. That is different from what I worked on a few years ago. There, the navigation came from the book module, and it contained the full structure of the book. So all my JS had to do was find the current page in that tree and then climb up the tree, adding the active-trail class.

I'm not sure what the best path forward is.

After making the switch in the UI from essentially "ignore active trail" to "show active trail", if we flip the config property to match, then we end in the position of needing to update existing block configurations and how to deal with them in layout builder. We could just update global block configs, leave LB blocks unchanged, and then in code treat the "show_active_trail" property being unset to be the same as TRUE.

Alternatively, we could leave the property as "ignore_active_trail", and then in the block config form submit, flip the form value* before saving to config. I don't know if that's done anywhere else though, and it seems counterintuitive and bad DX.

*I think there's also some consideration needed on how handle the form value because the checkbox can be disabled by states, though maybe the easiest thing would be not to disable the field and only set visible/invisible.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

Random thought would be to do some sort of check in the status report, though the detection would likely be limited to globally placed menu blocks, and maaaaybe menu blocks in LB defaults. But then being able to set the config property would require some pretty technical knowledge, whether setting via drush or the config sync UI or whatever.

We looked at this issue again at #3540379: Drupal Usability Meeting 2025-08-15. (Present: benjifisher, rkoller, simohell.)

We still think that something positive ("Add a CSS class") is clearer than something negative ("Be the same on all pages"). But you make a good point about not mentioning the "active-trail class" in the UI. Can we compromise on something less specific, like one of these?

• Add a CSS class to parent links
• Highlight the ancestors of the active menu link with a CSS class
• Add a CSS class to ancestors of the current page
• Add a CSS class to links leading to [or above] the current page

The goal is that the typical user will have to read the help text (description) once, and then the label should be enough when they return to the configuration form. So the label does not have to say too much. On the other hand, the help text should explain

• This option has an effect when the block is shown on a page listed in the block.
• It applies to all "parents" or "ancestors" of the current page that are shown in the block.
• Adding the CSS class has performance implications.

@godotislate:

I think the BC problems are easier to handle than you suggested in #36.

A boolean field effectively has 3 possible values: TRUE, FALSE, or NULL. When rendering the block and inspecting the value of the new config item, treat NULL as TRUE.

Someone (@alexpott) once commented that ... ?? TRUE had a bad "code sniff" or something like that. So probably add a code comment, explaining that this is for BC.

Couple questions on the MR.

I also pulled the latest changes down locally and took this screen recording.

• Menu level 1
• Number of levels 2
• Expand all items
• Add CSS class unchecked

After saving and re-opening, the dialog showed it as checked.

https://www.drupal.org/files/issues/2025-12-01/Screen%20Recording%202025...

Hmm, maybe we just leave it as toggling visible/invisible then?

One more thing I noticed in testing again just now: we added the condition that a level 1 menu with depth of 1 can also ignore the active trail in the UI, but this isn't being handled in the API. I just tested and the active trail class appears when the checkbox is unchecked, and looking at the logic in build() seems to confirm. I don't think this is related to the issue in the screen recording in #43.

Thinking about it some more, my feeling is the best way to go would be to make the checkbox always visible. Make it required via states except when depth = 1 and level = 1 or depth = 1 and expand all items = 1. Browser validation will prevent the user needing to wait for submission backend processing to get an error message. This is also probably one of the easiest implementation paths on the FE and BE as well.

The drawback is that the user may not get great feedback about why the checkbox is required, but this might be OK compared to drawbacks with other methods.

This is just my own opinion though, and I am not a UX expert.

Did some more manual testing and added MR comments to fix some form behavior with the checkbox state and saving.

I have reviewed the changes and tested with Umami locally, and this looks good.

Nice work to set the configuration the Umami footer and header menu blocks. It really demonstrates the performance gains.

I am +1 for RTBC, but since this issue has never made it to RTBC and I did a lot of initial work on the MR, I think it'd be best to get another pair of eyes for RTBC +1.

Updated the IS with new screenshots and documented API changes.

Rebased for merge conflict in the performance test.

Rebased and update performance tests stats to resolve merge conflict.

> This will be most useful for sites with large mega menus - because they simultaneously don't need the active trail behaviour usually (all links are shown on all pages) and also have a big expensive menu to render for every active trail variation, and also have a lot of unique active trails because there's so many links in the menu.

Somewhat off topic, but In many cases, such mega menus still need the active trail classes to style the active section/trail. We tried a few things in a project where we not only have such a menu but the site also has dozens of sections with their own version of the menu. This results in thousands of variations of the main navigation. We experimented quite a bit with pushing things into JS, but it's surprisingly noticeable, especially when you click through multiple pages in the same section. An intermediate option is to use the depth limit option in menu_trail_by_path so that it only needs to built variations for the top-level links, but for our specific site, that's complicated too. another idea is to do some kind of post-processing in PHP, a bit like that active link class stuff.

The other aspect of the cost of these large menus is invalidation, because every time you save a node form with menu_ui for a node that's in that menu, you basically invalidate your whole site as every page has the node cache tag for every linked node. I'm working on that in #3485030: Avoid saving menu links through node form when they do not change + https://www.drupal.org/project/menu_cache.

('ve been planning on writing a blog post on all this for a while, but would love to do it after that core issue is fixed and there are actually improvements in this space).

Back to this issue. That doesn't mean that it's not useful, it is useful for various use cases, especially secondary/footer menus and is a bit more performant on that than what menu_trail_by_path is doing, but maybe not quite as much for that mega menu use case.

Posted some comments on the MR. The CR looks pretty good to me and I think clarifies the styling part. I find the last part about config import a bit confusing, why does it specifically mention that? Wouldn't it be sufficient to say "explicitly changed in the block configuration", it doesn't really matter how that's done (you could use also use drush cset, or an update function, or ...)

I can review the MR comments later, but for now, I made the change to the CR. Wrote it a long time ago, so who knows what I was thinking then. :)

Added shouldSetActiveTrail() method to consolidate the logic.
Also added a blockValidate() implementation to validate the required state of the active trail checkbox when submitting via LB or any AJAX submit that bypasses browser form validation. PHPStan wanted the `void` return type declaration added, and if I tried the @return void annotation instead, PHPCS wanted a description for that, so I just added the type declaration. I checked, and the contrib module Menu Block does not implement blockValidate(), so that at least is safe.

Also, having the checkbox in the form be named ignore_active_trail while it functions as the opposite of that was breaking my brain, even though I knew what was going on. So I renamed the form element add_active_trail_class, and its value is the inverse of the ignore_active_trailconfig property.

Rebased for performance tests again.

I think this is ready now. There's a lot of code for validation, states, and extra careful checks to not break the active trail when it should work, but it's all pretty sensible. There are some limitations to this, but many sites have secondary/top/footer menus that do not have active trail styling and it helps with those quite a bit.

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

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

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

Not sure why the bot thinks there's a PHPStan issue, but rebased anyway and tests are still passing. Back to RTBC.

Committed and pushed ba110231a9e to main and 97c31d32ebf to 11.x. Thanks!