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.
Comment | File | Size | Author |
---|---|---|---|
#16 | after.png | 13.17 KB | Chernous_dn |
#16 | before.png | 15.35 KB | Chernous_dn |
#11 | interdiff-2640464-3-11.txt | 748 bytes | samuel.mortenson |
#11 | dialog-linkbuttons-2640464-11.patch | 1.54 KB | samuel.mortenson |
#3 | dialog-linkbuttons-2640464-3.patch | 1.54 KB | samuel.mortenson |
Comments
Comment #2
samuel.mortensonComment #3
samuel.mortensonMissed
e.preventDefault();
in my original patch.Comment #4
samuel.mortensonComment #6
Wim LeersComment #7
Wim LeersWhy not? In which browsers?
Comment #8
samuel.mortensonThat 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:
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:
Comment #9
Wim LeersOk. Can you update the patch accordingly?
This will need sign-off from one of the JS maintainers. And ideally would get JS test coverage.
Comment #10
droplet CreditAttribution: droplet commented@samuel.mortenson
Can you give more info on which module you're using? I want to test it in the real environment.
Comment #11
samuel.mortenson@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.
Comment #12
droplet CreditAttribution: droplet commentedThanks. Looks right to me.
Comment #13
droplet CreditAttribution: droplet commentedOK. I've done my 2nd review. RTBC
Comment #16
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedRetest patch in #11 and test complete. Test local, and work fine. Attach screenshot.
Comment #18
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedComment #20
droplet CreditAttribution: droplet commentedComment #21
xjmThanks for working on this! Assigning frontend RTBC issues to Cottser for possible review.
Comment #22
star-szr@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…
Comment #23
samuel.mortenson@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!
Comment #24
star-szrAh didn't think to try the dev version will try that! Thanks @samuel.mortenson.
Comment #28
star-szrCommitted and pushed 1aa6458 to 8.3.x and bfa605e to 8.2.x and 00000dc to 8.1.x. Thanks!