Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr()

[kevinkace]

Remove vnode.dom === $doc.activeElement from isFormAttribute(), and setAttribute() to fix validityCheck(), #2256

Description

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.

Motivation and Context

This fixes checkValidation().

#2256

How Has This Been Tested?

Here's a version of the modified code on flems.io

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.* 1 test was failing before update
• I have updated docs/change-log.md

[fuzetsu]

Are we sure removing this activeElement check makes sense? It seems to be associated to changing the selected attr. Do we know what this check was trying to achieve? I don't think it's related to #2256
Here is the old condition with parentheses to clarify it.

return (
  attr === 'value' ||
  attr === 'checked' ||
  attr === 'selectedIndex' ||
  (attr === 'selected' && vnode.dom === $doc.activeElement) ||
  (vnode.tag === 'option' && vnode.dom.parentNode === $doc.activeElement)
);

[dead-claudia]

One nit below, but you also need to write some tests so we know the fix actually works.

[dead-claudia]

Could you please revert this for now and only fix the one issue with input.value? The comment in the bug regarding this was me noting I wanted to investigate (since it appeared similar in use), not that I actively wanted to change it.

[kevinkace]

Will do!

[kevinkace]

This is certainly wrong, but I'm having a heck of a time wrapping my head around ospec.

o("checkValidity", function() {
    var value = "";
    var spy = o.spy()
    var context = {
        handler: withAttr("value", spy)
    }
    var form = {tag: "form", attrs: {
        novalidate: true,
        onsubmit: function(e) {
            e.preventDefault();
        }
    }, children: [
        {tag: "input", attrs: {value: value, oninput: withAttr("value", function(v) { value = v; })}},
        {tag: "button", attrs: {type: "submit"}}
    ]}

    render(root, [form])

    context.handler({currentTarget: {value: "abcd"}})
    // submit form
    o(form.dom.checkValidity()).equals(false)
    context.handler({currentTarget: {value: "abc"}})
    // submit form
    o(form.dom.checkValidity()).equals(false)
})

https://codepen.io/kevinkace/pen/MPPOML

[dead-claudia]

@kevinkace

1. You don't need to rebuild anything - we have a bot that does it for us.
2. Have you taken a look at ospec's docs?

[kevinkace]

@isiahmeadows
I did look at the docs. I couldn't figure out how to make a test for this (submitting a form, using withAttr). Sadly I can't justify spending more time to learn this test framework.

I'm including the Fork in my project ATM.

[barneycarroll]

@isiahmeadows

you also need to write some tests so we know the fix actually works

I don't think this is fair, since this bugfix merely reverts bugfix code which wasn't tested for anyway.

Even if that weren't the case, extending Mithril's DOM mocks to emulate W3C form validation — let alone to the point that we can reasonably account subtle implementation details like this one — is a hiding to nothing.

I think having the back link to the test cases provided in the original issue is a good enough validation.

[dead-claudia]

@barneycarroll

I don't think this is fair, since this bugfix merely reverts bugfix code which wasn't tested for anyway.

Could you find the commit that added it, so we can find the surrounding context?

Even if that weren't the case, extending Mithril's DOM mocks to emulate W3C form validation — let alone to the point that we can reasonably account subtle implementation details like this one — is a hiding to nothing.

Or we could just invoke vnode.dom.focus() in a hook that runs before all DOM hooks (like onbeforeupdate), to trigger the behavior. It'd have the same effect, and it'd be a lot less work.

I think having the back link to the test cases provided in the original issue is a good enough validation.

I don't like adding bug fixes without tests, just in principle. I'd rather not us be back here a second time because of an erroneous "bug fix" that breaks it again - it's happened before already IIRC.

[StephanHoyer]

Already fix in next