In Drupal.behaviors.dialog.prepareDialogButtons(), Form actions that are submit buttons are visually hidden from the modal content and cloned into a separate button pane. The selector to find these buttons is .form-actions input[type=submit], which misses form elements of type link that have been styled to visually appear as (secondary) submit buttons. As a result, modals do not display these link-buttons inline with the rest of the form actions, which is bad UX.

For an example of an extremely commons link-as-button use case, see: \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink().

The provided patch adds support for link-buttons to Drupal.behaviors.dialog.prepareDialogButtons(). I've also attached screenshots which show what modals look like before/after the patch.

CommentFileSizeAuthor
#16 after.png13.17 KBChernous_dn
#16 before.png15.35 KBChernous_dn
#11 interdiff-2640464-3-11.txt748 bytessamuel.mortenson
#11 dialog-linkbuttons-2640464-11.patch1.54 KBsamuel.mortenson
#3 dialog-linkbuttons-2640464-3.patch1.54 KBsamuel.mortenson
after.png20.61 KBsamuel.mortenson
before.png20.13 KBsamuel.mortenson
dialog-prepare-action-links.patch1.51 KBsamuel.mortenson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Active » Needs review
samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
FileSize
dialog-linkbuttons-2640464-3.patch1.54 KB

Missed e.preventDefault(); in my original patch.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Title: Link styled as buttons not placed inside Dialog's button pane » Links styled as buttons not placed inside Dialog's button pane
Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Links styled as buttons not placed inside Dialog's button pane » Links styled as buttons not placed inside Dialog's button pane — prevents "cancel" link/button from showing up
Component: forms system » javascript
Issue tags: +JavaScript
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
+++ b/core/misc/dialog/dialog.ajax.js
@@ -71,14 +71,22 @@
+            // If the original button is an anchor tag, triggering events will
+            // not work on all browsers. Use the standard click method instead.

Why not? In which browsers?

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

That comment may need updated to "...not work on any browsers", as I can't find evidence that jQuery's $.trigger('click') or $.click() will ever work on anchor tags. I believe that at the time of writing that I did have a source that discussed different click event behavior on different browsers, but I can't find it now.

Explanation of why jQuery's click event doesn't "click" anchor tags - http://learn.jquery.com/events/triggering-event-handlers:

The .trigger() function cannot be used to mimic native browser events, such as clicking on a file input box or an anchor tag. This is because, there is no event handler attached using jQuery's event system that corresponds to these events.

Explanation of how the native DOM's click method differs from triggering the "click" event directly - https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/click:

When click is used with elements that support it (e.g., one of the types listed above), it also fires the element's click event, which bubbles up to elements higher in the document tree (or event chain) and fires their click events, too. However, the bubbling of a click event does not cause an element to initiate navigation as if a real mouse-click had been received.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Ok. Can you update the patch accordingly?

This will need sign-off from one of the JS maintainers. And ideally would get JS test coverage.

droplet’s picture

droplet CreditAttribution: droplet commented
Issue tags: +Needs manual testing

@samuel.mortenson

Can you give more info on which module you're using? I want to test it in the real environment.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
FileSize
dialog-linkbuttons-2640464-11.patch1.54 KB
interdiff-2640464-3-11.txt748 bytes

@wim-leers Here's an updated patch which rewords the "not work on all browsers" section of the comment.

@droplet To see this you can download the Page Manager/Ctools modules, enable Page Manager UI, visit /admin/structure/page_manager/manage/node_view/general, and click "Delete page" in the upper-right hand corner of the screen. This form uses \Drupal\Core\Form\ConfirmFormHelper and as a result contains a "Cancel" anchor tag styled as a button.

droplet’s picture

droplet CreditAttribution: droplet commented

Thanks. Looks right to me.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

OK. I've done my 2nd review. RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: dialog-linkbuttons-2640464-11.patch, failed testing.

The last submitted patch, 11: dialog-linkbuttons-2640464-11.patch, failed testing.

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Reviewed & tested by the community
FileSize
before.png15.35 KB
after.png13.17 KB

Retest patch in #11 and test complete. Test local, and work fine. Attach screenshot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: dialog-linkbuttons-2640464-11.patch, failed testing.

Chernous_dn’s picture

Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: dialog-linkbuttons-2640464-11.patch, failed testing.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Assigned: Unassigned » star-szr

Thanks for working on this! Assigning frontend RTBC issues to Cottser for possible review.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Assigned: star-szr » Unassigned

@samuel.mortenson I followed the steps in #11 to try and test this but I got a 404 at /admin/structure/page_manager/manage/node_view/general. I installed the Page Manager UI and didn't see the Delete option in the top right at /admin/structure/page_manager/manage/node_view either.

Not sure if it's possible to test just with core…

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

@Cottser The ConfirmFormHelper is used in core, but never in a modal, so you won't be able to test this patch with core.

I just installed Drupal 8.2.x (Standard profile) and Page Manger UI (from the dev branch of Page Manager, which could make a difference), and I don't get a 404 at /admin/structure/page_manager/manage/node_view/general. Could you try again with the dev release of Page Manager? Thanks!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Assigned: Unassigned » star-szr

Ah didn't think to try the dev version will try that! Thanks @samuel.mortenson.

  • Cottser committed 00000dc on 8.1.x 
    Issue #2640464 by samuel.mortenson, Chernous_dn, droplet, Wim Leers:...

  • Cottser committed 1aa6458 on 8.3.x 
    Issue #2640464 by samuel.mortenson, Chernous_dn, droplet, Wim Leers:...

  • Cottser committed bfa605e on 8.2.x 
    Issue #2640464 by samuel.mortenson, Chernous_dn, droplet, Wim Leers:...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs manual testing

Committed and pushed 1aa6458 to 8.3.x and bfa605e to 8.2.x and 00000dc to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.