Skip to content

Remove submit logic from click event handler #79

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

Merged
merged 1 commit into from
Mar 10, 2018
Merged

Remove submit logic from click event handler #79

merged 1 commit into from
Mar 10, 2018

Conversation

tilboerner
Copy link
Contributor

There is no need to have submit logic take place in the on click handler. It only deals with the mouse click event, not the submit event that will follow.

Before this change, blocking the default action on the click event prevents a following submit event from taking place: form.onsubmit and other registered listeners waiting for a 'submit' will never be called.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
There is no need to have submit logic take place in the on click handler. It only deals with the mouse click event, *not* the submit event that will follow. 

Before this change, blocking the default action on the click event prevents a following submit event from taking place: `form.onsubmit` and other registered listeners waiting for a 'submit' will never be called.
@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

Merging #79 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #79   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          74     74           
=====================================
  Hits           74     74

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e7656d...ab6dbdd. Read the comment docs.

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

@tilboerner I recommend checking out this bit. I needs to still work, I don't know if it is covered in the tests tho: https://stackoverflow.com/questions/47231094/get-submit-button-value-onsubmit-event

let submitButton = e.target
let form = submitButton.closest('form')
const submitInput = document.createElement('input')
submitInput.type = 'hidden'
submitInput.value = submitButton.value || true
submitInput.name = submitButton.name
form.appendChild(submitInput)
uploadS3Inputs(form)
Copy link
Owner

Choose a reason for hiding this comment

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

@tilboerner how does this still work?

Copy link
Contributor Author

@tilboerner tilboerner Mar 9, 2018

Choose a reason for hiding this comment

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

It still works because with the other removed line in this patch, the default action for the click event is not prevented. That default action is to cause a 'submit' event on the form. (Note that the function containing the change is not handling the submit event itself, but the click event leading up to it. Preventing its default action means no submit event gets dispatched, and not submit event handlers get called, even if we later submit the form from the prototype.)

The submit event is then handled by the form.onsubmit function which was set below, on 'DOMContentLoaded':

form.onsubmit = (e) => {
e.preventDefault()
uploadS3Inputs(e.target)
}

If I understand JS correctly, things proceed synchronously, and we can trust at this point that if the submit was triggered by a click, the click handler has added the hidden input to the form.

I found one caveat: When manually dispatching a click event for the button, it needs to be a MouseEvent to trigger the submit logic; a generic Event("click") triggers the click handler, but does not default to submitting the form. Without the change in this PR, any old Event would do.

However, the canonical click event is a MouseEvent, and gets dispatched as such even when triggering the button by keyboard or calling button.click(). All things considered, I think it's better to make the proposed change and honor the expectation of other submit handlers that they will be called on submit, rather than cater to code that is trying to use non-standard, non-MouseEvents to trigger submit clicks.

edit Forgot to mention @codingjoe. :)

@codingjoe codingjoe merged commit 495cf93 into codingjoe:master Mar 10, 2018
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.

None yet

2 participants