Allow button tag in LinkGenerator for better accessibility

We need this functionality, too. Rerolling...

I didn't resolve that conflict properly. Trying again.

Fixing a typo

Verified that the latest patch in #12 works.

Fixing tags

+1 for this feature request. The motivation is well described in the issue summary. It's a widely applicable feature for mega-menus, tree-style sidebar menus, login forms presenting login forms on dialogs.

This would benefit from a change record. That should be drafted before RTBC.

The patch looks straightforward enough, but I haven't managed to test this yet. I'm relying on simplytest.me today, and got errors. I've queued another test for the patch in #12.

Draft change record created at https://www.drupal.org/node/3053689

Code looks great.

Also tried it out on a stock Umami install by adding a menu link to the main menu. I set the link to route:<button> and it did indeed show up as a button with the markup:

<button class="menu-main__link" type="button">Empty Button</button>

Queued tests against 8.8.x

@andrewmacpherson are we supposed to see results posted in this issue? I'm not sure how this works.

@mherchel - the test results are noted next to the patch itself. You can see the result in comment #12 - it passes against 8.8.x

1. --- /dev/null
   +++ b/core/lib/Drupal/Core/GeneratedButton.php
   

   I'm not convinced we've using the correct namespace here - but it is the namespace we've used for \Drupal\Core\GeneratedLink and \Drupal\Core\GeneratedNoLink - I think we should consider moving these to either Drupal\Core\Routing, Drupal\Core\Menu, Drupal\Core\Utility (along side LinkGenerator) or somewhere new - let's open a follow-up for that.

2. +++ b/core/lib/Drupal/Core/GeneratedButton.php
   @@ -0,0 +1,15 @@
   +/**
   + * This class holds a <button> generated from the <button> route.
   + */
   +class GeneratedButton extends GeneratedLink {
   

   One thing I was thinking about is how this relates to and more importantly does not relate to \Drupal\Core\Render\Element\Button and what confusion it could introduce. I think the description here might need to go into that. Or at least mention that this is not for generating buttons for forms but for putting a button in a list of links such as a multi-level menu.

Item 1 (from #22) should be resolved with https://www.drupal.org/project/drupal/issues/3074978. Next I'll work on item 2.

Here's the updated patch that addresses item 2 from #22

Issue and comment look good to me. Thanks @mtift!

+++ b/core/lib/Drupal/Core/GeneratedButton.php
@@ -0,0 +1,19 @@
+/**
+ * This class holds a <button> generated from the <button> route.
+ *

<button> generated from the <button> route is very confusing to read. First I thought it was a mistake in the patch, but reading GeneratedNoLink it's correct, but still confusing. I tried to think of alternative wording but struggled a bit.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -170,6 +171,11 @@ public function generate($text, Url $url) {
+    elseif ($url->isRouted() && $url->getRouteName() === '<button>') {

this is getting unwieldy now, can we add a protected method the elements after this that are common and return early instead of stacking up the elseifs

e.g. lines 181-187 of HEAD

if (!($variables['text'] instanceof MarkupInterface)) {
      $variables['text'] = Html::escape($variables['text']);
    }
    $attributes = new Attribute($attributes);
    // This is safe because Attribute does escaping and $variables['text'] is
    // either rendered or escaped.
    return $generated_link->setGeneratedLink('<' . $generated_link::TAG . $attributes . '>' . $variables['text'] . '</' . $generated_link::TAG . '>');

Would become a new method, something like doGenerate

Then inside the first if we'd do

return $this->doGenerate($generated_link, $attributes);

Then the next two elseifs (with this patch) become just ifs that also return

and the final else is not needed, as by that stage the others have returned.

Sounds good.

That last patch doesn't work. Missing $variables and perhaps more.

Let's try this

Verified #31 patch applies and is working correctly!

Any updates on when this will be committed?

Is this going to be committed???

@eeyorr At the earliest this will go in Drupal 9.1 (probably early 2021).

Currently the core committers are trying to work out all of the deprecations in Drupal 9.0 to get that out of the door.

I'm planning on going to Drupal Dev Days in about a month. Assume there are some core committers there, I'm going to bug them :P

@mherchel, thanks for the update!

#26 hasn't been addressed but I can't think of a better wording either. @mherchel actually we can commit this to 8.9.x and 9.0.x now. Nothing is frozen - yes issues like this are not a priority but this is still a good thing to add to core that's very difficult for contrib to do.

Committed and pushed 8ed835c0bc to 9.0.x and 485db38e54 to 8.9.x. Thanks!

diff --git a/core/lib/Drupal/Core/Utility/LinkGenerator.php b/core/lib/Drupal/Core/Utility/LinkGenerator.php
index d5be974951..665e1d3192 100644
--- a/core/lib/Drupal/Core/Utility/LinkGenerator.php
+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -197,7 +197,7 @@ public function generate($text, Url $url) {
    * @param array $variables
    *   The link text, url, and other options.
    *
-   * @return Drupal\Core\GeneratedLink $generated_link
+   * @return Drupal\Core\GeneratedLink
    *   The generated link, along with its associated cacheability metadata.
    */
   protected function doGenerate($generated_link, $attributes, $variables) {

Fixed coding standards on commit.

The change record for this issue is still at draft status. Should it be published?