Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: serialize forms submitted by <button> #57

Closed
wants to merge 1 commit into from

Conversation

hraban
Copy link
Contributor

@hraban hraban commented Jul 4, 2024

A more modern approach using form submit event’s ‘submitter’ property.

https://caniuse.com/mdn-api_submitevent_submitter

Also requires changes to reblocks-ui

A more modern approach using form submit event’s ‘submitter’ property.

https://caniuse.com/mdn-api_submitevent_submitter
var options = options || {};
var action_arguments = form.serializeObjectWithSubmit();
var action_arguments = form.serializeObject();
if (["BUTTON", "INPUT"].includes(event.submitter && event.submitter.tagName)) {
Copy link
Contributor Author

@hraban hraban Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could probably be even more generic as just a

if (event.submitter && "name" in event.submitter && "value" in event.submitter)

but catching all <input> and <button> is a decent start

hraban added a commit to hraban/reblocks-ui that referenced this pull request Jul 4, 2024
Also makes the handlers overridable, which makes it easier to override and/or
implement new behavior.

Depends on 40ants/reblocks#57
@svetlyak40wt
Copy link
Member

Why this is better approach (aside that it is more "modern")?

@svetlyak40wt
Copy link
Member

Also, I see at https://caniuse.com/mdn-api_submitevent_submitter that Internet Explorer does not support "submitter" even in the latest version. This could be a problem for some users.

@hraban
Copy link
Contributor Author

hraban commented Jul 5, 2024

Why this is better approach (aside that it is more "modern")?

This patch a fix for forms containing <button>s. Those aren't caught by the old code.

Another thing this catches is non-click submission events, e.g. if you're in an field and press Enter, a default submit button is selected and used as the submission. A regular post form catches that.

Also, I see at https://caniuse.com/mdn-api_submitevent_submitter that Internet Explorer does not support "submitter" even in the latest version. This could be a problem for some users.

Internet explorer is obsolete and unsupported even by MS now I think. https://en.wikipedia.org/wiki/Internet_Explorer says development stopped in 2016 and all support ended 2022.

@svetlyak40wt
Copy link
Member

Aha! I'll have to play with this first. Thank you for the PR.

@svetlyak40wt
Copy link
Member

@hraban I've build this simple demo using the latest version of Reblocks and Reblocks-ui:

https://gist.github.com/svetlyak40wt/47df0712195ca49c1ae96c62294c4bd3

And both button and input :type button works perfectly with keyboard events.

Could you please update this example to make it possible to reproduce the problem which can be solved by "submitter"?

@hraban
Copy link
Contributor Author

hraban commented Jul 7, 2024

The point is getting the value of the button. Try this one 40ants/reblocks-websocket#4 but substitute the <input name="x" value="y" type="submit"> with <button name="x" value="y">y</button>.

@svetlyak40wt
Copy link
Member

@hraban sorry for a long delay with replies, I'm having some troubles with health and have to solve them asap.

However today I've tested your changes from this PR and from your PR to rebocks-ui, on my example form and noticed that form is submitted as usual POST request with full page reload instead of Ajax request:

Screenshot 2024-07-28 at 00 27 32

Do you have any ideas how to fix this behaviour?

@hraban
Copy link
Contributor Author

hraban commented Jul 28, 2024

@svetlyak40wt if you also use reblocks-ui you need this PR : 40ants/reblocks-ui#10

@hraban
Copy link
Contributor Author

hraban commented Jul 28, 2024

Good luck with your health btw!! Best wishes.

@svetlyak40wt
Copy link
Member

@svetlyak40wt if you also use reblocks-ui you need this PR : 40ants/reblocks-ui#10

Hmm, I've used both PRs. However now after the full application restart it works as Ajax requests. Probably some code was in conflict when I've switched branches and did quickload in my previous attempt.

@svetlyak40wt
Copy link
Member

Merged as part of the #62

@hraban hraban deleted the feat/form-submitter branch July 29, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants