Skip to content
This repository was archived by the owner on Nov 19, 2018. It is now read-only.

Unexpected array values #73

Closed
rajsite opened this issue Sep 20, 2015 · 20 comments
Closed

Unexpected array values #73

rajsite opened this issue Sep 20, 2015 · 20 comments
Milestone

Comments

@rajsite
Copy link

rajsite commented Sep 20, 2015

I recently migrated from ajax-form 1.5.4 to 2.1.0 as part of a polymer 1.0 migration and I have noticed that I now get back an array of values for my paper-input for fields when submitting a form as application/json:

{"first":[["1"],["1"]],"second":[["3"],["3"]]}

I see in the documentation for the enctype property that if multiple input fields have the same name that this is expected behavior for json data. However this is how my source looks:

        <form is="ajax-form" id="add-form" method="post" enctype="application/json" action="/lvwd-components-start/calc/add">
            <paper-input name="first" label="First number" floatinglabel></paper-input>
            <paper-input name="second" label="Second number" floatinglabel></paper-input>
            <paper-button id="add-submit" raised>Submit</paper-button>
        </form>

I think it is related to a querySelectorAll in ajax-form.js that ends up catching paper-input fields and the child input fields of the paper-input fields, but I haven't walked through to see if that is definitely the case.

When I set a breakpoint we can see that both will be processed but I do not know if the subroutines try to filter out the nested elements:

capture

@rnicholus
Copy link
Owner

I haven't verified, but your explanation sounds plausible. Polymer has been tough to support due to the instability of the library and the community of elements. At some point, I believe all of these Polymer custom input fields hid the underlying native form fields in the shadow DOM. It looks like the Polymer team has abandoned shadow DOM entirely as of v1, replacing it with something they call "shady DOM". No idea what "shady DOM" is, and I don't really care. I have no use for Polymer anyway.

One possible fix may be for me to modify ajax-form to start by looking at the children of a <form>. For each child of the form that appears to be a form field record that field and ignore any children. For any children that are not form fields, continue to visit children, using the same logic. I think this may be easily implemented simply by sifting through the results of the existing selector, and then closely examining elements with matching name attributes. If two elements with matching names have an descendent/ancestor relationship, throw out the descendent and use the ancestor instead.

@rnicholus rnicholus added the bug label Sep 20, 2015
@rnicholus rnicholus added this to the 2.1.1 milestone Sep 20, 2015
@rnicholus
Copy link
Owner

Just remembered that the code in ajax-form that attempts to related custom elements to form fields may be the cause of this issue. The final sentence of my above comment can probably just be applied to elements located during the custom element form field that have native form field matches. In that case, the custom field should be thrown out, provided the match is a descendent of the custom field.

@rajsite
Copy link
Author

rajsite commented Sep 20, 2015

Looking around at other form serializers, the Polymer iron-form may be a good reference.

To handle Polymer form elements it listens for the iron-form-element-register event but since Polymer 1.0 is exposing the underlying input elements then maybe using HTMLFormElement.elements is enough to capture all the elements?

It looks like the iron-form is using the HTMLFormElement.elements property at least in-part, but they also handle some checkbox behavior.

@rnicholus
Copy link
Owner

I think an adjustment to the existing custom element selector code is all
that is needed. I do want to be able to continue to handle custom form
fields that do not expose their native form fields in the light dom as
well. I won't be listening for any proprietary events.
On Sun, Sep 20, 2015 at 11:34 AM Milan Raj [email protected] wrote:

Looking around at other form serializers, the Polymer iron-form may be a
good reference
https://github.com/PolymerElements/iron-form/blob/master/iron-form.html#L170
.

To handle Polymer form elements it listens for the
iron-form-element-register event
https://github.com/PolymerElements/iron-form/blob/master/iron-form.html#L105
but since Polymer 1.0 is exposing the underlying input elements then maybe
using HTMLFormElement.elements
https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements
is enough to capture all the elements?

It looks like the iron-form is using the HTMLFormElement.elements
property at least in-part
https://github.com/PolymerElements/iron-form/blob/master/iron-form.html#L194,
but they also handle some checkbox behavior.


Reply to this email directly or view it on GitHub
#73 (comment).

@rajsite
Copy link
Author

rajsite commented Sep 20, 2015

Oh yea, definitely avoid their internal events :P I was just curious to see how Polymer was handling it. Sounds good!

@rnicholus
Copy link
Owner

Can you provide me with the composed DOM of your <form> containing paper-inputs, according to dev tools? I'd like to see if the paper-inputs, which have a name attribute, are passing that same name attribute to the underlying <input> elements.

Better yet, a link to a live page where this can be reproduced...?

rajsite added a commit to rajsite/lvwd-components-start that referenced this issue Sep 21, 2015
Polymer 1.0 changes cause an unexpected change in resulting json in
ajax-form: rnicholus/ajax-form#73

Switching to iron-form until the issue is resolved
@rnicholus
Copy link
Owner

Nevermind, I just looked at your initial post again, missed the screenshot the first time around. It seems like a bug in paper-input. It really shouldn't be passing the name attribute from the paper-input to the underlying input, in my opinion. I'd file a bug report with the Polymer people, and convince them that it is odd to have duplicate name attributes in this case. If they resist, feel free to mention the Polymer issue number and I'll jump in to the discussion as well. Failing all of that, I'll consider a workaround here, but I certainly don't want to get into the practice of writing code that is Polymer-specific.

@rnicholus rnicholus removed this from the 2.1.1 milestone Sep 21, 2015
@rajsite
Copy link
Author

rajsite commented Sep 21, 2015

Hmm, I'm not convinced that it would be a Polymer bug. The ajax-form behavior of taking all elements that have a name attribute and a value property as elements to serialize is a different behavior than what HTMLFormElements.elements would choose to serialize.

I like the clarity of this approach over the DOM selection rules used by HTMLFormElements.elements and I think it's just a question of how the library handles the case of nested elements containing name attributes.

<div class="make-it-pretty">
    <div name="hello">
        <div name="world"></div>
    </div>
</div>

I think it would be reasonable to say that the only element that would be part of serialization would be div[name="hello"] and maybe even give some console.warns that div[name="world"] will be ignored.

@rnicholus
Copy link
Owner

I really don't see a good reason why <paper-input> should duplicate the name attribute and pass it on to the underlying <input>.

The ajax-form behavior of taking all elements that have a name attribute and a value property as elements to serialize is a different behavior than what HTMLFormElements.elements would choose to serialize

That's correct, and this was an explicit decision on my part. Relying solely on HTMLFormElements.elements will only return native form elements. This means that ajax-form would no longer be able to parse custom form elements that do not contain native form element child (or custom form elements that do contain a native form element in the shadow DOM).

The real question here is, which element should ajax-form extract the value from: <paper-input> or the child <input>? Could the two values differ? Certainly. Even if <paper-input> does not behave this way, I could certainly see a custom form field that maintains its own distinct value, one which differs from the underlying child <input>. In fact, <file-input> behaves this way. It filters the selected files that may be on the child <input type="file"> based on the validation restrictions associated with the <file-input> element.

@rnicholus
Copy link
Owner

The ajax-form behavior of taking all elements that have a name attribute and a value property as elements to serialize

I also want to point out that this is not exactly how ajax-form functions. It only uses this logic for custom elements. It checks for :valid and :invalid pseduo-classes to detect native form fields.

@rajsite
Copy link
Author

rajsite commented Sep 21, 2015

One case I can imagine is for accessibility on screen readers, etc, that would not be aware of custom elements. If a screen reader was to look through the form in the DOM it would follow the HTMLFormElements.element rules. If all the Polymer form elements expose a corresponding native field then assistive technologies would see the native fields (surrounded by a bunch of elements they don't care about).

In addition, if a user uses a normal form tag (instead of an ajax-form or iron-form, etc) then having the underlying native element allows the form to operate as expected (with the added benefit of pretty material design etc.). I haven't confirmed any of these assumptions, but It seems like they are doing it for web platform compatibility reasons.

@rnicholus
Copy link
Owner

It seems like they are doing it for compatibility reasons

Could be, but that doesn't really address any of the issues I mentioned in my last comment. The fact remains that there is no single approach that will properly support all custom form fields. I'm apprehensive to make any changes to better support Polymer if it means that non-Polymer custom elements will suffer. The easiest solution would be for the Polymer team to ensure the name attribute is not present on the underlying native form field. I'm not quite sure what accessibility implications that will bring, if any.

@rajsite
Copy link
Author

rajsite commented Sep 21, 2015

sure, then I think it boils down to how the following case mentioned previously should be handled:

<div class="make-it-pretty">
    <div name="hello">
        <div name="world"></div>
    </div>
</div>

Assuming the divs have a value property added to them how would the JSON be serialized?

{ "hello": "val", "world": "val"}

{"hello": {"world": val}} ? // doesn't really make much sense, would need to show relationship and values

{"hello": "val"}

The current behavior appears to be the first option and it is possible that is the expected behavior for the ajax-form custom element.

@rnicholus
Copy link
Owner

I'm confused. The issue we've been discussing with a custom form field and it's child native form element, both sharing the same name attribute. The issue you've outlined in your previous markup deals with serializing two entirely different fields (neither of which are form elements or custom elements) as JSON.

@rajsite
Copy link
Author

rajsite commented Sep 21, 2015

Yea! I was just trying to see how a general solution, like the one initially proposed would behave (as I understood it).

So in the following example:

<div class="make-it-pretty">
    <div name="hello">
        <div name="world"></div>
    </div>
</div>

The current behavior is to serialize to:
{"hello":"val", "world":"val"}

And the algorithm of ignoring form elements (elements that have an attribute called name and a property called value) that are contained inside another form element would instead ignore the div[name="world"] element and serialize to:
{"hello":"val"}

An analogy that would also happen to cover the issue of native elements inside custom elements would be:

<div class="make-it-pretty">
    <div name="hello">
        <div name="hello"></div>
    </div>
</div>

Which currently serializes to:
{"hello": ["val", "val"]}

But with the ignore nesting algorithm would instead serialize to:
{"hello": "val"}

@rnicholus
Copy link
Owner

I'm going to make a couple changes to your markup, as I think this is more representative of what we are discussing. I'm then going to go over a few assumptions. Please let me know if I'm misinterpreting anything:

<form action="/endpoint" is="ajax-form" enctype="application/json">
   <custom-form-field name="hello">
      <input name="hello">
   </custom-form-field>
</form>

The above markup is the final result of placing a <custom-form-field> in a document, which is then upgraded either by the browser or the WC shim, resulting in the child <input> which is ostensibly part of the <custom-form-field> element's template.

Currently, ajax-form serializes this to: {"hello": ["val", "val"]}. But with the changes I outlined, the result would instead be {"hello": "val"}. The custom element would be examined, and it's child input would be ignored.

Are we both on the same page? If so, then I think perhaps I can work this in to the next version of ajax-form. Note that the adjustment would not affect form fields with identical names that are siblings, as we would want all of these fields to be sent to the server with all values contained in an array. After all, this is how native <form> elements behave (assuming they could serialize fields as JSON, which they can't).

@rnicholus rnicholus added this to the 2.1.1 milestone Sep 21, 2015
@rnicholus
Copy link
Owner

Going the direction outlined above as of 1a21a11 on hotfix/2.1.1. This will not be a trivial change, as I will now need to maintain a mapping of elements to their field names so I can eliminate any parent/child redundancies.

@rnicholus
Copy link
Owner

Few more adjustments in 2845fa3 before I begin working on this.

rnicholus pushed a commit that referenced this issue Sep 21, 2015
rnicholus pushed a commit that referenced this issue Sep 22, 2015
rnicholus pushed a commit that referenced this issue Sep 22, 2015
@rnicholus
Copy link
Owner

@rajsite I believe I have a workaround in place as of my latest commit to the hotfix/2.1.1 branch. Let me know if this fixes the issue for you. If it does, I'll push out a new hotfix releaase.

@rnicholus
Copy link
Owner

Due to lack of response to this issue and the proposed fix, I'm not convinced that any change is needed on my end. So, I'm going to close this, but will consider reopening if I receive confirmation that this issue needs to be addressed in ajax-form and confirmation that my adjustment in the hotfix branch addresses the issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants