Local action links should be inside UL element

This MR looks good to me. We should definitely be aiming for correct semantics.

To avoid having to change this in every theme (including contrib themes) would it be better if the local actions manager attached a theme wrapper or similar to wrap the links in a ul?

lt seems a bit strange to have to wrap the actions with a <ul> in the block template. Ideally I think we would implement something in \Drupal\Core\Menu\Plugin\Block\LocalActionsBlock::build to ensure the local actions are always wrapped with a <ul>. We could follow the pattern local tasks are setting by having a separate template for the wrapper. We should also continue wrapping the menu with the <nav> element.

I'm not sure if I agree with using #theme_wrappers since there's been some on going efforts to try to reduce our usage of it 🌱 Remove usages of #theme_wrappers Active .

CSS is already there. I tried to restore original functionality so it is not broken for contrib themes (like bootstrap5).
We can skin the cat many ways but I would suggest to quick fix and then create follow up task to improve it.
Otherwise Claro would be using ul class="local-actions" and core and stable9 would be using nav 🤷‍♂️

I'm not sure I follow what is broken for contrib themes. Couldn't contrib themes fix this bug by providing a template override?

I guess the challenge with the quick fix is that it may make it harder for us to provide the fix later because people could have copies of these templates leading into a duplicate <ul>.

I'm not sure I see at the moment how either leaving out the <nav> element or adding the new classes would make this less disruptive 🤔

On one hand, semantics is broken according to Mozilla's LI specification.

The <li> HTML element ... must be contained in a parent element: an ordered list (<ol>), an unordered list (<ul>), or a menu (<menu>)...

And, secondly, there is no consistency between core themes in something simple like local actions.

This was a bugsmash daily triage target a few day ago.

The remaining tasks in the issue summary are out of date, adding tag for an update. This still needs a change record. Leaving at needs work.

Change record: [#3347200]

Agree with #36 and think it would be the correct way to fix this. The release notes should clearly point out, that this *might* break things, as @laurii wrote in his MR comments.

But IMHO this shouldn't go into 10.x, but just 11.x.

Was reported once again in 🐛 Local actions template misses UL-element (block--local-actions-block.html.twig) Closed: duplicate

We should match the pre-existing element, and therefore leave the class out from the

.

@laurii what exactly do you mean by "match"? Sorry for the dumb question, but I'd like to get that right and seems like it wasn't solved yet, so perhaps I'm not the only one... ;)

Another possible approach just stop using UL for local actions. Really, UL is intended for lists. It's useful say for SR users to understand how long the list is. However, our local actions are typically consist of just one link. It does not make sense to wrap a single link into UL list.