Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This patch allow for buttons (with type=submit) to trigger an ajax enabled exposed form, and is similar to the change in #1123300: Image button in block views with exposed filter breaks AJAX routine.
Release notes snippet
Submit buttons themed as <button type="submit"> elements now trigger AJAX within Views exposed forms, just like they do when themed as <input type="submit"> elements. This now matches the behavior of Drupal 7 Views, fixing a regression from Drupal 8 and earlier versions of Drupal 9.
Comment | File | Size | Author |
---|---|---|---|
#88 | 1551534-80.patch | 4.36 KB | effulgentsia |
#88 | 1551534-80-test-only.patch | 2.9 KB | effulgentsia |
#80 | interdiff_78-80.txt | 717 bytes | vsujeetkumar |
#80 | 1551534-80.patch | 4.36 KB | vsujeetkumar |
#78 | interdiff_75-78.txt | 1.01 KB | vsujeetkumar |
Comments
Comment #1
bcn CreditAttribution: bcn commentedStatus change since there is a patch to review.
Comment #2
dawehnerWhat is the difference between button in input, or why do you need this feature?
Comment #3
bcn CreditAttribution: bcn commentedThe general difference is a button can have html inside.
<button><h1>Click</h1></button>
instead of:
<input type="button" />
--http://stackoverflow.com/questions/469059/button-vs-input-type-button-which-to-use
I needed this because I was using a theme which used a element in the exposed filter of a view.
Comment #4
natted CreditAttribution: natted commentedI've tested this patch and use it also for the same reason as poster #3
Comment #5
JuliaKoelsch CreditAttribution: JuliaKoelsch commentedI also tested it, because I ran into the issue described. Fixes issue for me as well.
Comment #6
zmove CreditAttribution: zmove commented+1 for this, especially with the HTML5 that is coming a support is very important.
Comment #7
cpliakas CreditAttribution: cpliakas commented+1, and I also confirm that the patch fixes the issue I ran into as well. This seems to have multiple testers with positive experiences and no discernable drawbacks, so marking as RTBC.
Comment #8
Jibus CreditAttribution: Jibus commented+1, confirm that the patch works for me. Thanks !
Comment #9
dnewkerk CreditAttribution: dnewkerk commented+1 also. Tested and confirmed this works perfectly. I ran into this issue when using the Twitter Bootstrap theme (which is HTML5-based), though it likely affects most/all of the various HTML5-based themes.
Comment #10
muka CreditAttribution: muka commented+1 same issue on bootstrap theme, works for me
Comment #11
Lucasljj CreditAttribution: Lucasljj commentedallow-buttons-to-trigger-ajax.patch queued for re-testing.
Comment #12
dawehnerGreat. Thanks for providing and testing the patch.
Committed to 7.x-3.x and marked as to be ported to 8.x
Comment #13
batabata CreditAttribution: batabata commentedWELL,HELPsFUL
Comment #14
richmac CreditAttribution: richmac commentedComment #15
richmac CreditAttribution: richmac commentedThe attached patch has been tested against a view created in 8.x-dev.
Comment #16
alansaviolobo CreditAttribution: alansaviolobo commentedreroll
Comment #17
DamienMcKennaDoes this patch need tests? It makes complete sense to me and still applies against HEAD. RTBC?
Comment #18
Lucasljj CreditAttribution: Lucasljj as a volunteer commentedComment #19
mikefyfer CreditAttribution: mikefyfer commented.
Comment #20
mikefyfer CreditAttribution: mikefyfer commentedJust to update this real quick, as it's still an issue. In a hurry, so no patch here, just the code.
as of Drupal 8.0.4
In core/modules/views/js/ajax_view.js - change
Comment #21
heykarthikwithure-roll
Comment #23
devert CreditAttribution: devert commentedThis patch no longer seems to be able to be applied to latest Drupal 8.1.8.
Comment #24
devert CreditAttribution: devert commentedRe-roll so patch will work on Drupal 8.1.8
Comment #25
devert CreditAttribution: devert commentedComment #27
nerdacus CreditAttribution: nerdacus at Metal Toad commentedApplied #24 to Drupal 8.2.1 and it works as promised.
Comment #28
dawehnerI think the steps forward would be to add some test coverage.
Comment #30
mvwensen CreditAttribution: mvwensen commentedClosed duplicate: #2897276: Allow the attachment of AJAX exposed form behavior to a button of type submit
Comment #32
b_sharpe CreditAttribution: b_sharpe at ImageX commentedRe-roll
Comment #33
droplet CreditAttribution: droplet commentedIMO, we should make it new .class rather than tag[attr] if it's a feature request. and mark existing way @deprecated.
Comment #34
nst37 CreditAttribution: nst37 commentedAre there any plans to bring back reset button and make it work with AJAX?
Comment #36
seanBReroll for 8.5 and 8.6, applies to both.
Comment #37
Alexandre360 CreditAttribution: Alexandre360 commentedthe patch is quite simple and probably have no drawback. Any plan to commit it soon ?
Comment #38
b_sharpe CreditAttribution: b_sharpe at ImageX commentedWorks great, can see no way for this to cause issue/conflict
Comment #39
HongPong CreditAttribution: HongPong as a volunteer and at kor group commented+1 looks good to me, this would help with certain things.
Comment #40
alexpottIn order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:
Comment #41
mishac CreditAttribution: mishac commentedPatch didn't apply for me on 8.6-rc2. rerolling.
Comment #43
grantkruger#41 worked for me on 8.6 after #36 stopped working. Thanks to you both.
Comment #44
henkholtrop CreditAttribution: henkholtrop at Alserda Media commentedRedid the patch. Now on the 8.6.x branch.
Comment #45
henkholtrop CreditAttribution: henkholtrop at Alserda Media commentedComment #46
sanderwind CreditAttribution: sanderwind commentedTested #44 and works.
Comment #47
alexpottSee #40 we still need automated test coverage. Also as a new feature we need to target the next minor release branch.
Comment #49
LendudeHere is a test for this, test_only file is also the interdiff.
Comment #51
idebr CreditAttribution: idebr at ezCompany commentedThis should be short array syntax. Test looks good!
Comment #52
LendudeFixed #51
Comment #53
idebr CreditAttribution: idebr at iO commentedThe feedback in #40 has been addressed.
Comment #55
LendudeUnrelated fail
Comment #57
andypostbot flux
Comment #60
LendudeMedia lib fail
Comment #61
heykarthikwithu+1 RTBC
Comment #62
lauriiiIs there a specific reason why we wouldn't allow
<button type="button">
to trigger ajax?Comment #63
lauriiiMoving to needs review for #62.
Comment #64
idebr CreditAttribution: idebr at iO commented#62 Drupal has historically only used input elements, so many components do not support
<button type="button">
simply because the markup was not available. There is no technical reason why a<button>
should not be able to trigger ajax.Comment #66
Lendudeknown random fail, #3099427: [random test failure] FieldLayoutTest::testEntityView()
Comment #67
lauriiiI would prefer allowing
button[type=button]
to trigger ajax too since it seems like there isn't any particular reason to not do that.Comment #68
idebr CreditAttribution: idebr at iO commented#67 Not sure, a button[type="button"] should not trigger a form submit:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button
Comment #70
casey CreditAttribution: casey at SWIS commentedComment #72
ugintl CreditAttribution: ugintl commented#52 did not apply to 8.9. Should I try #70?
Comment #73
neclimdulI don't think 70 would apply to 8.9. I've made a quick convenience patch though for 8.9 even though I don't expect this will be back ported to 8 at this point.
70 failed because it dropped the .theme file so the test failed. Here's basically the same thing but it should pass.
This interdiff is against 70 to show what I fixed but this is the same patch as 52 rerolled onto 9.1.x. There would be no interdiff to 52 because there was just some whitespace conflicts from a es6 transpiler changes and some other patch offset conflicts.
Reroll is not a review, I haven't actually looked at this in a long time just passing through and helping out.
Comment #75
LendudeLets see if this gets it back to green on 9.1.x
Comment #77
idebr CreditAttribution: idebr at iO commentedThe patch in #75 contains an unrelated file `./core/.phpunit.result.cache`
Comment #78
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedRe-roll patch created for 9.2.x, Also removed unreated file './core/.phpunit.result.cache' from the patch, Please have a look and advise.
Comment #79
idebr CreditAttribution: idebr at iO commentedcore/modules/views/js/ajax_view.es6.js does not pass eslint validation
Comment #80
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixed cs issues.
Comment #82
b_sharpe CreditAttribution: b_sharpe at ImageX commentedWorks as intended and has test coverage, lets get this in!
Comment #83
BramDriesen+1 also tested this and it indeed does what it needs to do :)
Comment #84
choudouhu CreditAttribution: choudouhu commentedfor version 9.1.9, 1551534-75.patch not working
Comment #85
choudouhu CreditAttribution: choudouhu commentedFor version 9.1.9, 1551534-75.patch not working
Comment #86
BramDriesen@choudouhu Use the latest one (#80) instead of a year old patch :-)
Comment #88
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAdding issue credits for reviewers and testers, and for @rfbrandsma from #2897276: Allow the attachment of AJAX exposed form behavior to a button of type submit .
Also, reclassifying this as a bug and retitling accordingly.
Also, reuploading #80 to get a new test run on Drupal 9.3, as well as a test-only patch to make sure that it fails as expected without the JS fix.
Comment #89
effulgentsia CreditAttribution: effulgentsia at Acquia commentedResponding to reviews that didn't get incorporated into the patch:
@lauriii in #67:
I agree with #68's answer to this that the ajax that we're triggering is effectively a form submit so it should only apply to
type="submit"
buttons. Also, this way, we match the behavior that's in Views for Drupal 7 in https://git.drupalcode.org/project/views/-/blob/7.x-3.x/js/ajax_view.js#L90, per the commit that happened in #12. If we discover a need to also trigger this for other types of buttons, we can have a followup issue to add that.@droplet in #33:
If we want to do this, let's do it in a followup. The existing code in HEAD targets
input[type=submit]
, andbutton[type=submit]
has essentially the same semantics, so refactoring fromtag[attr]
is out of scope for this issue.Comment #96
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCrediting people from another duplicate: #2998908: Views exposed form Ajax selector doesn't work on buttons.
Comment #101
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMissed some in #96.
Comment #104
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 9.3.x. Tagging for a release notes mention, and added the release notes snippet to the issue summary.
Thank you to the many people who tested, reviewed, and rerolled this over the years.
Comment #105
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #106
effulgentsia CreditAttribution: effulgentsia at Acquia commentedBy the way, there are several other JS files where
input[type=submit]
is in a selector andbutton[type=submit]
isn't. And there might also be PHP code that treats them differently too. From #2998908-10: Views exposed form Ajax selector doesn't work on buttons:#1671190: Use <button /> form element type instead of <input type="submit" /> is probably the best place to discuss that for now. We may want to open new child issues of that one to fix specific bugs ahead of actually changing core to use
<button>
elements itself.