Jsdom: Form elements defined outside hierarchy with form='form-id' are not captured by form.elements

[FThompson]

Previous issues (#765 #2368) have addressed this issue resulting in PR #2369 but this fix does not add form elements defined outside form hierarchy with form='form-id' in the form.elements member.

Basic info:

• Node.js version: 10.15.3
• jsdom version: 15.1.1

Minimal reproduction case

const { JSDOM } = require('jsdom');

let form = `
    <form id='test-form'></form>
    <input type='text' name='test' form='test-form' />
`;

let dom = new JSDOM(form);
console.log('expecting one form element');
console.log('actual: ' + dom.window.document.forms[0].elements.length); // 0

Repro: https://github.com/FThompson/jsdom-form-attribute-bug

How does similar code behave in browsers?

form.elements detects elements defined outside form hierarchy with form attribute:
https://jsfiddle.net/fthompson/2njuLq3r/2/

[chrisnicola]

Noticed that this also effects the submit button behaviour. A type="submit" button with form= set that is outside of the form element will not trigger submit on the form when clicked.

[ccwebdesign]

This seems to fix the issue, if you (JSDOM contributors) want to add it. It needs proper testing (sorry -- low on time at the moment).

In master lib/jsdom/living/nodes/HTMLFormElement-impl.js:50

  get elements() {
    // TODO: Return a HTMLFormControlsCollection
    return HTMLCollection.createImpl([], {
      element: this,
-     query: () => domSymbolTree.treeToArray(this, {
+     query: () => domSymbolTree.treeToArray(this.ownerDocument, {
-       filter: node => isListed(node) && (node._localName !== "input" || node.type !== "image")
+       filter: node => isListed(node) && (node._localName !== "input" || node.type !== "image") && this == node.form
      })
    });
  }

[FThompson]

@ccwebdesign Your solution looks like it would work, but one concern I have about that approach would be inefficient runtime when dealing with large web pages. I'd need to test it though to be sure. Otherwise, I think the best solution would be to live track which elements in the page correspond to each form in the page, but that would be far more involved to implement.

[ccwebdesign]

@FThompson From what I can tell, the change (above) doesn't track mutations (or doesn't track mutations quickly enough -- re:performance). Below works.

Changes to get elements()

+ const { HTML_NS } = require("../helpers/namespaces");

...

  get elements() {
    // TODO: Return a HTMLFormControlsCollection
    return HTMLCollection.createImpl([], {
      element: this,
-     query: () => domSymbolTree.treeToArray(this, {
-       filter: node => isListed(node) && (node._localName !== "input" || node.type !== "image")
-     })
+     query: () => {
+       let nodes = domSymbolTree.treeToArray(this, {
+         filter: node => isListed(node) && (node._localName !== "input" || node.type !== "image")
+       });
+
+       if (!this.hasAttribute('id')) {
+         return nodes;
+       }
+
+       let id = this.getAttribute('id');
+       let documentNodes = this.ownerDocument.querySelectorAll('[form="' + id + '"]');
+
+       for (let element of documentNodes) {
+         element._namespaceURI = HTML_NS;
+         nodes.push(element);
+       }
+
+       return nodes;
+     },
    });
  }

The HTMLCollection checks for _namespaceURI on namedItem(), so it needs to be added.

This probably has some other consequences that I'm not aware of that will need testing, though it should also be better on performance.

Seems to work alright with the JSDOM / Mocha testing that I'm doing which needs both form.elements to work with external elements and to account for added / removed elements.

[FThompson]

@ccwebdesign Nice update! A couple observations:

1. This version will return duplicate elements if there is an element with form='form-id' attribute inside a form declared wth id='form-id'.
2. This version will return non-form elements, such as p, containing form='form-id' anywhere in the document.

I may have time this weekend to help explore solutions further.

[ccwebdesign]

@FThompson Good point.

Modified the above

- const { HTML_NS } = require("../helpers/namespaces");

...

  get elements() {
+   const baseQuery = node => isListed(node) &&
+     (node._localName !== "input" || node.type !== "image");
+
    // TODO: Return a HTMLFormControlsCollection
    return HTMLCollection.createImpl([], {
      element: this,
      query: () => {
        let nodes = domSymbolTree.treeToArray(this, {
-         filter: node => isListed(node) && (node._localName !== "input" || node.type !== "image")
+         filter: node => baseQuery(node)
        });

        if (!this.hasAttribute('id')) {
          return nodes;
        }

        const id = this.getAttribute('id');
+       const documentNodes = domSymbolTree.treeToArray(this.ownerDocument, {
+         filter: node => baseQuery(node) && !this.contains(node) &&
+           node.hasAttribute('form') && node.getAttribute('form') == id
        });

-       for (let element of documentNodes) {
-         element._namespaceURI = HTML_NS;
-         nodes.push(element);
-       }
-
-       return nodes;
+       return nodes.concat(documentNodes);
      },
    });
  }

Trimmed version for quick copy/paste if needed

• add to /jsdom/lib/jsdom/living/nodes/HTMLFormElement-impl.js:50

// https://html.spec.whatwg.org/multipage/forms.html#dom-form-elements
get elements() {
  const baseQuery = node => isListed(node) &&
    (node._localName !== "input" || node.type !== "image");

  // TODO: Return a HTMLFormControlsCollection
  return HTMLCollection.createImpl([], {
    element: this,
    query: () => {
      let nodes = domSymbolTree.treeToArray(this, {
        filter: node => baseQuery(node)
      });

      if (!this.hasAttribute('id')) {
        return nodes;
      }

      const id = this.getAttribute('id');
      const documentNodes = domSymbolTree.treeToArray(this.ownerDocument, {
        filter: node => baseQuery(node) && !this.contains(node) &&
          node.hasAttribute('form') && node.getAttribute('form') == id
      });

      return nodes.concat(documentNodes);
    },
  });
}

For [email protected]+

• add to /jsdom/lib/jsdom/living/nodes/HTMLFormElement-impl.js:50

// https://html.spec.whatwg.org/multipage/forms.html#dom-form-elements
get elements() {
  const baseQuery = node => isListed(node) &&
    (node._localName !== "input" || node.type !== "image");

  // TODO: Return a HTMLFormControlsCollection
  return HTMLCollection.createImpl(this._globalObject, [], {
    element: this,
    query: () => {
      let nodes = domSymbolTree.treeToArray(this, {
        filter: node => baseQuery(node)
      });

      if (!this.hasAttribute('id')) {
        return nodes;
      }

      const id = this.getAttribute('id');
      const documentNodes = domSymbolTree.treeToArray(this.ownerDocument, {
        filter: node => baseQuery(node) && !this.contains(node) &&
          node.hasAttribute('form') && node.getAttribute('form') == id
      });

      return nodes.concat(documentNodes);
    },
  });
}

This removes the requirement for the HTML_NS import, since it's filtering the nodes through the domSymbolTree, though it still seems like it might be inefficient. Definitely needs testing to check.

edit: trimmed more
Issue exists with JSDOM 16.2.2
fix above works with JSDOM 16.2.2

JSDOM as a whole (with or without patch above) fails with elements that need a collection, such as multiple checkbox elements with the same name. see: #2600

[ccwebdesign]

Updated above for jsdom 16.1.0, issue still exists. Related to issue #1570

[FThompson]

Could you fork the repository and commit your updated elements() function?

According to the contributing guidelines, there's a test/benchmarking suite available in the repository. If you create a fork, then others (including myself) can help test the code, with the eventual goal of opening a pull request to get the code integrated into JSDom.

[ccwebdesign]

Sure -- fork is at https://github.com/ccwebdesign/jsdom along with commit for 16.1.0

Edit: synced fork to 16.2.0

[timrobinson33]

I've raised another bug which I think could be related https://github.com/jsdom/jsdom/issues/2979. - the form validation also excludes controls that aren't physically nested inside the form

[ccwebdesign]

@timrobinson33 Patch above seems to fix the issue (currently working with JSDOM 16.2.2). Yell if you notice otherwise. Edited: I should say 'or can confirm the same'. XD

Without patch:

input.validity.valid: false
form.checkValidity(): true
form.reportvalidity(): true

With:

input.validity.valid: false
form.checkValidity(): false
form.reportvalidity(): false

[ccwebdesign]

Synced fork to 16.2.2, master @ commit https://github.com/jsdom/jsdom/commit/8921128ee1efe6737e3ea200959eb01fe75ff3aa

[timrobinson33]

Sorry but I can't get your fix to work in my scenario. I might have messed up the environment because I'm new to all this stuff, and I'm using create-react-app so it's all pretty complicated but this is what I have done:

1. downloaded a zip of your fork from github (version df600fb you just made) and run yarn prepare
2. deleted all the 'jsdom' subdirectories from my node_modules and confirmed the test doesn't run because it can't find jest
3. put your jsdom folder inside my node_modules/jest-environment-jsdom-sixteen/node_modules folder

now the test runs but it gives the same result. However, if I look at target.elements.length, it returns 2 from your fork but 1 from the production version, which makes me think I am using your version.

[ccwebdesign]

The simplest method is to keep your environment the same so you're using the last release.

Open /jsdom/lib/jsdom/living/nodes/HTMLFormElement-impl.js, then replace get elements() with the above (Trimmed version for quick copy/paste if needed). Hate having to patch it directly, but that's the only option that I've found.

Give that a shot and let me know?

[timrobinson33]

Just pasting in that replacement function wouldn't run - it gave "Error: Internal error: invalid global object" on the HTMLCollection.createImpl(this._globalObject...).

however, I can see that the version I downloaded does have that updated code in it, which correctly returns 2 for elements.length, so I'm pretty sure I'm using your version.

Unfortunately the checkValidity function doesn't call get elements to list all the elements to be validated; it uses domSymbolTree.treeIterator(this).

I tried replacing line HTMLFormElement-impl.js line 201 with
for (const el of this.elements) {

and it _did_ fix my problem. However I wouldn't feel confident putting in a PR for this because I really don't have any knowledge of how jsdom works and whether this might mess anything else up.

I still can't explain why you don't seem to be seeing the same as me. maybe you made some later fixes that haven't been pushed?

[ccwebdesign]

@timrobinson33 Derp, you're right, the Trimmed version is for pre-16.1 when they introduced the global object part.

Sorry, forgot that it was different -- had left it in case anyone was using pre-16.1 JSDOM.

The 'For [email protected]+' version should take care of that error. Should also take care of the line 201 change automatically, since it's using the domSymbolTree.

Give that a go?

[timrobinson33]

Sorry, not sure quite what you mean. I'm using https://github.com/ccwebdesign/jsdom/ rev df600fb (the commit you made today) which still has the form validation bug. are you saying there's a newer one?

[ccwebdesign]

Ignore the previous -- I see the bug you're talking about now. Your fix on 201 gets it.

You should make a fork / push a commit / mention it on #2979 that the two fixes work fine together to get things working better. Hopefully it'll garner some attention to get some testing in and eventually merged.

[FThompson]

Great to see the collaborative effort that led to this fix. Thanks! @ccwebdesign @domenic @timrobinson33