Implement a caching strategy for the menu links

ckrina → credited catch → .

Working on it.

Worked on a POC that use client-side cache for the whole navigation tree, while loading only the 1st level from the server.

The approach is a cloned adapted from how Toolbar module works.

My JS skills are limited, so would be great if someone could take a look into them.

Also need to figure out how to avoid duplicated IDs when same block is placed more than once clean_unique_id twig filter does not work well for individual AJAX calls.

My initial thought was to use the block ID, but I found this issue: 💬 Retrieve unique block ID or machine name in build() method of BlockBase class Active

Feedback and ideas are welcome!

Also need to figure out how to avoid duplicated IDs when same block is placed more than once clean_unique_id twig filter does not work well for individual AJAX calls.

On 1.x it is already appending --<NUMBER> to avoid the duplicates if > 1 of the same Navigation Block is placed.

I'm specifically looking at the ids that the MR modifies in templates/navigation-menu.html.twig, and those seem to be fine. If there are specific steps that lead to the duplicate ID issue I'm interested in knowing what they are.

Let's make sure that any caching strategy that we implement is built in mind that we want to keep the navigation completely static in between page loads with the exception of the active marker moving to a different menu link. This means we want to avoid any flickering or even loading animations when you navigate within Drupal, including the username. The navigation is really close to being able to achieve this which is quite remarkable.

Hi @bnjmnm

The reason why it failed is because the --NUMBER suffixes added to the IDs modify the HTML markup, hence the hash generated from that markup is not consistent.

This is because Html::getUniqueId() methods adds a random identifier for AJAX calls

Here are the steps to reproduce it:

• Replace clean_id with clean_unique_id in navigation-menu.html.twig
• Clear Drupal cache
• Clear browser local storage
• Disable render cache from /admin/config/development/settings or config files
• Reload the page and confirm that there's an AJAX error and navigation menu nested levels are not being loaded.

Getting rid of the id suffix solves the issue in the meantime, even if the HTML markup is not correct. It has to be resolved somehow, though.

Another option would be to get rid of id attributes in the template, but I'm not sure if that's possible for accessibility reasons.

plopesc → closed merge request !259

Closed MR against contrib navigation module. Created equivalent MR against core.

I'm not 100% sure about this, it would be good to get confirmation from someone with more accessibility knowledge, but I think ID attributes are only required for form elements, and anything else should be OK with aria roles etc. - so it may just be fine to remove the ids, use data- attributes if necessary, and add aria markup if it's not already there.

Toolbar flickering was fixed in 🐛 Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays Fixed / 🐛 Investigate better ways to add anti-flicker JS Fixed , so unless there is more flickering going on, that might not be an issue any more? That issue only landed in the past few months after many years of toolbar in core though.

On the accessibility question -- I don't anticipate an issue there. Assistive devices only need an HTML ID if something in the HTML is trying to reference the element by its ID, e.g. via a label's for= attribute or an aria-labelledby reference.

Sometimes in nav elements it's nice to reference the inner heading to give the nav a unique name. But that could be done in the JS with a simple global counter.

During Drupalcon sprints worked on an approach where ID attributes, and all the references to them are removed from the HTML markup before generating the hash, so the problem is gone, but at the cost of some extra calculations that would be great to avoid.

Would be great to have some extra feedback about whether we can get rid of id attributes or should we explore other paths assuming that IDs could be there.

On the other hand, if we remove IDs from the core templates, but a specific site overrides the template making use of them, the navigation would be broken.

That's something that already would happen with current toolbar, but need to confirm if that scenario should be supported by navigation.

#17 is pretty convincing to me that we can do without the ID attributes.

Postponing this until ✨ back the ID and aria-labelled-by with JS Postponed is in. Discussing this with @plopesc.

I think we might be better off doing 📌 Render the navigation toolbar in a placeholder Active (see some of the discussion in that issue).

If we do that, then navigation needs to use a #lazy_builder for the navigation render array and core will handle efficiently rendering it via render caching / dynamic_page_cache / bigpipe depending on what's enabled. The HTML will be served directly, no need for local storage and hence no need to worry about the flickering issues that affected the toolbar module or custom js etc.

This needs one improvement to core's placeholder rendering strategy (the CachedPlaceholderStrategy sketched out in that issue), but that improvement would benefit any situation where placeholders are used, not just Navigation.

Not closing this as a duplicate yet but if it all works, then we'll be able to keep things simple in Navigation itself, and also improve overall core page serving performance at the same time.

The approach proposed by @catchlooks great to me.

📌 Add render caching for the navigation render array Active landed which unblocks 📌 Render the navigation toolbar in a placeholder Active .

Marking this as PP-2 and added that issue to the issue summary here. I'll swap the navigation stable blocker tags over too. If we have to change direction again we can switch things around again, but pretty sure this will be both easier and better than adapting the toolbar tree caching implementation so let's try it!