PolymerElements / iron-form

Custom form element
https://www.webcomponents.org/element/PolymerElements/iron-form
63 stars 81 forks source link

Elements that are not part of the form are submitted #205

Open H3dz opened 7 years ago

H3dz commented 7 years ago

This seems to be a recurring issue, but this time too many elements are submitted, including the ones that are in shadow-root.

For example if your form has a paper-dropdown-menu the following will be submitted:

The question is why do we have to recursively call all nodes? The previous implementation worked correctly, well at least for me. It seems to me that the <x-input-wrapper> in the basic.html test should be created and used like the other components, meaning that a name and value should be declared in the element.

Additionally, the commits of the new code looks weird compared to the rest of the iron-form code, no space after if, else if on 2 lines...

See commit:

TomK commented 7 years ago

Long and short, I think this request to remove shadow traversal is valid. Old (2012) w3c shadow-dom spec (https://www.w3.org/TR/2012/WD-shadow-dom-20120522/#html-forms) makes specific mention that forms should follow scoping constraints and exclude shadow-dom.

Although that is absent in the latest published document (https://www.w3.org/TR/shadow-dom/), I believe it probably still stands.

Anyone with greater understanding of the spec can shed light on this?

@notwaldorf - my bad with this PR, sorry. I should have opened it to discussion.

ianjosephwilson commented 7 years ago

Seems like that would make iron-form a lot less useful if it couldn't access any nested inputs, especially if they were in a more complicated widget. Like a color picker or map pinner.

Regarding your linking of prior specs, I found part of the removal from the document here: https://github.com/w3c/webcomponents/commit/eb3c43a0e18fdffe158ad1eb50f95be7c7909d59#diff-322632c0dc0d55acad89f47aaea675d7

Furthermore following what seemed to be the original link to how controls are associated, the "as specified" link, it seems that the most appropriate solution is buried in this somewhere:

https://www.w3.org/TR/2011/WD-html5-20110525/association-of-controls-and-forms.html#association-of-controls-and-forms

Testing things out in my current Chromium, 60.0.3112.78 , an input nested inside shadow dom inside a form has its form property set to null whereas an input in the regular dom of a normal form element has its form property set to the form element. I tried to make an example here that you can see with the inspector:

https://jsbin.com/hiqasu/edit?html,output

So one solution might be to traverse the shadow dom but only include inputs whose form property points to the original form element wrapped by iron-form. So a composite form control, one with more than one input, could set the form property on its elements in the shadow dom based on searching its ancestor form in the light dom. Psuedo code:

<form id="form1">
<composite-form-controls>
#shadow-root 
<input form={{ancestorForm}}
</composite-form-controls>
</form>

The algorithm would then switch node.root to node.shadowRoot and apply a check if node.form == this._form, such as

      _findElements: function(parent, ignoreName) {
        var nodes = Polymer.dom(parent).querySelectorAll('*');
        var submittable = [];
        for (var i = 0; i < nodes.length; i++) {
          var node = nodes[i];
          // An element is submittable if it is not disabled, and if it has a
          // 'name' attribute.
          if(!node.disabled && (ignoreName || node.name) && node.form === this._form) {
            submittable.push(node);
          }
          else {
            // This element has a root which could contain more submittable elements.
            if(node.shadowRoot) {
              Array.prototype.push.apply(submittable, this._findElements(node.shadowRoot, ignoreName));
            }
          }
        }
        return submittable;
      },

I am not sure if stamped templates need to be traversed differently than regular light dom when they are not in the shadow dom. I have no idea how that works so node.shadowRoot might not be an acceptable replacement for node.root.

ianjosephwilson commented 7 years ago

Just to clarify, the default would then be to not include form controls in the shadowRoot without the shadowRoot's owner explicitly setting each form control's form property.

Also it is easy to imagine that custom elements might have multiple controls with different names that couldn't be reduced to a single name/value pair on the custom element itself. For example an address control might compose a multitude of inputs into one custom element custom-address-input that would need to include name, street1, street2, state, county, postal, etc. This contradicts the original issue description's line: meaning that a name and value should be declared in the element. but the original problem I think could be satisfied by using the form property.

ianjosephwilson commented 7 years ago

Also probably related to this: #204

ianjosephwilson commented 7 years ago

I think I was wrong about how the form attribute/property works. Maybe the only solution is some custom api at the top level api that returns a name->value mapping or just restrict shadow dom elements to a name/value pair. The form property seems to only apply within a single document and the shadow dom as I understand it is outside the main document. So assigning form elements to an element in the shadow dom that are from the main document does not work in chrome.

TomK commented 6 years ago

@notwaldorf shadow traversing has (mistakenly?) been in for quite a while now. It would probably create BC issues for people on ^2.0. Perhaps 3.0 is a good time to address this?

I opened a PR (#208) to revert shadow traversal back in august last year. i've closed it now as things have changed a lot since then.