MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14k stars 926 forks source link

Form checkValidity() sometimes returns true when form fields are invalid #2256

Open kevinkace opened 5 years ago

kevinkace commented 5 years ago

When using m.withAttr() to maintain an input value, the form's checkValidity() method sometimes returns true when form fields are not valid. This happens on the second and subsequent submissions.

Expected Behavior

checkValidity() should always return false if any fields within the form aren't valid, eg: https://codepen.io/kevinkace/pen/rqKroe?editors=1011

Current Behavior

checkValidity() returns true when a field contains an invalid value, on the second and subsequent submits. eg: https://codepen.io/kevinkace/pen/XxYBoK?editors=0011

Steps to Reproduce (for bugs)

https://codepen.io/kevinkace/pen/XxYBoK?editors=0011

  1. use m.withAttr() to set the value of an input with a minlength, eg:
    m("input", {
    minlength : 4,
    value : vnode.state.value,
    oninput : m.withAttr("value", function(value) {
    vnode.state.value = value;
    }
    })
  2. add an onsubmit handler to the form element, eg:
    m("form", {
    oncreate : function(formVnode) {
    vnode.state.formDom = formVnode.dom;
    },
    onsubmit : function(e) {
    e.preventDefault();
    vnode.state.formDom.checkValidity();
    },
    // form elements...
    )
  3. add a button to the form, eg m("button", "submit")
  4. add 3 characters to the input
  5. click the button, checkValidity() should return false
  6. click the button, checkValidity() now returns true

Context

I'm trying to used HTML form validation for form submition.

Your Environment

dead-claudia commented 5 years ago

So here's what I see: when you click the button[type=submit], it no longer sees the input as the active element, and thus doesn't think to check that the values are equal. If we were to remove the $doc.activeElement === vnode.dom check here, I'd suspect that'd likely solve your problem. FWIW, we already had to do that to work around a Chrome bug where changing it to the same value erroneously resets the cursor, so this is actually reducing the amount of work we're doing.

dead-claudia commented 5 years ago

Personally, I'd like to double-check whether document.activeElement really needs checked here as well.

The check here and here is required to work around this IE/old Edge bug and an IE9 bug referenced here, and probably should have a comment added for it at least.


But in summary, it's just a case where we're unnecessarily checking document.activeElement, and I suspect there might be others, too.

fuzetsu commented 5 years ago

@isiahmeadows I agree with your assessment. The document.activeElement in setAttr seems unnecessary.

removing document.activeElement seems to work

StephanHoyer commented 2 years ago

it's still an issue