Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<Form> submitter is serialized out of tree order #4342

Closed
jenseng opened this issue Oct 7, 2022 · 7 comments
Closed

<Form> submitter is serialized out of tree order #4342

jenseng opened this issue Oct 7, 2022 · 7 comments
Labels
bug Something isn't working feat:forms

Comments

@jenseng
Copy link
Contributor

jenseng commented Oct 7, 2022

What version of Remix are you using?

1.7.2

Steps to Reproduce

  1. Make a <Form> like so:
<Form>
  <button type="submit" name="foo" value="override">Override</button>
  <input type="hidden" name="foo" value="default" />
</Form>
  1. Click the "Override" button

Expected Behavior

The form fields should be serialized in tree order, i.e. foo=override&foo=default (as is the case with a vanilla <form>).

See Constructing the form data set in the HTML spec. Submitted fields should be sent in the order they appear in the DOM, including the submitter (i.e. the submit button that was clicked).

Actual Behavior

The form is serialized as foo=default&foo=override.

This is problematic on the server-side if you are relying on the order to indicate precedence. formData.get("foo") will yield different values, depending on whether the form was submitted by Remix or native browser behavior.

See also #3611; prior to that fix it was broken in a different way (it would only serialize foo=default)

@jenseng
Copy link
Contributor Author

jenseng commented Oct 7, 2022

On a related note, <Form> serializes image submit buttons in the wrong order and with the wrong key/values.

For example, given: <input type="image" name="image" src="..." />. If you click it, it gets serialized as image= at the very end, rather than the expected image.x=X&image.y=Y in tree order. See Step 5.2.

Probably worth lumping in to this bug report, as the root cause is the same, and the fix should be too 🤞

@jenseng
Copy link
Contributor Author

jenseng commented Oct 10, 2022

I found some more differences between how <Form> and <form> behave:

  1. In <Form> submissions, Safari includes the submit button value twice (once in tree order, once at the end) due to this bug
  2. If you have file inputs and the encType is application/x-www-form-urlencoded (explicit or implicit):
    • <form> will send each file entry's name as the value, as per the spec
    • <Form> will throw an error File inputs are not supported with encType "application/x-www-form-urlencoded" ...; if this invariant check is removed, no value gets submitted.

@jenseng
Copy link
Contributor Author

jenseng commented Oct 11, 2022

There's a proposal to let you pass a submitter to FormData; once implemented and widely available it should generally make this an easy fix.

In the meantime we might consider:

  1. Fully building the FormData ourselves according to the spec. I have this implemented locally in a WIP, though we might need to consider any perf implications around this.
  2. Partially fixing this (e.g. could probably handle the file input and non-image submit button issues in a relatively surgical manner)
  3. Doing nothing, but documenting these differences clearly. These things are all a bit edge-case-y, but it would be good to be clear where we deviate from the spec, so that people aren't surprised like I was :)

@jenseng jenseng changed the title <Form> target is serialized out of tree order <Form> submitter is serialized out of tree order Oct 11, 2022
jenseng added a commit to jenseng/remix that referenced this issue Oct 13, 2022
* Add failing tests for remix-run#4342:
  * Validate various submitter scenarios (tree order, image submit buttons)
  * Validate file handling in URL-encoded payloads
* Rework all submission tests to run both with and without JavaScript.
  This way we can better detect/prevent regressions or inconsistencies
  between how <form> and <Form> work.
@machour
Copy link
Collaborator

machour commented Oct 14, 2022

Great stuff @jenseng, thanks!

@machour machour added bug Something isn't working and removed bug:unverified labels Oct 14, 2022
@jenseng
Copy link
Contributor Author

jenseng commented Oct 17, 2022

As a workaround for non-image submitters, you can use a component like so that does some hidden input hijinks:

function Button(props: DetailedHTMLProps<ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>) {
  const hiddenRef = useRef<HTMLInputElement>(null);
  const buttonRef = useRef<HTMLButtonElement>(null);
  if ((props.type ?? "submit") === "submit" && props.name) {
    return (
      <>
        <input ref={hiddenRef} type="hidden" name={props.name} value={props.value} disabled />
        <button
          ref={buttonRef}
          {...props}
          onClick={(e) => {
            hiddenRef.current!.disabled = false;
            buttonRef.current!.name = "";
            setTimeout(() => {
              hiddenRef.current!.disabled = true;
              buttonRef.current!.name = props.name!;
            });
            props.onClick?.(e);
          }}
        />
      </>
    );
  } else {
    return <button {...props} />;
  }
}

Since the hijinks are done in the onClick handler, the button still has the right attributes to work before/without JavaScript

jenseng added a commit to jenseng/remix that referenced this issue Oct 18, 2022
* Add failing tests for remix-run#4342:
  * Validate various submitter scenarios (tree order, image submit buttons)
  * Validate file handling in URL-encoded payloads
* Rework all submission tests to run both with and without JavaScript.
  This way we can better detect/prevent regressions or inconsistencies
  between how <form> and <Form> work.
jenseng added a commit to jenseng/remix that referenced this issue Oct 22, 2022
* Add failing tests for remix-run#4342:
  * Validate various submitter scenarios (tree order, image submit buttons)
  * Validate file handling in URL-encoded payloads
* Rework all submission tests to run both with and without JavaScript.
  This way we can better detect/prevent regressions or inconsistencies
  between how <form> and <Form> work.
jenseng added a commit to jenseng/remix that referenced this issue Oct 26, 2022
* Rework all form tests to run both with and without JavaScript. This way we
  can better detect/prevent regressions or inconsistencies between how <form>
  and <Form> work.
* Dedup and expand form method tests to ensure we cover all supported Form
  methods and submitter formMethods
* Expand coverage around form serialization (tree order, image submit buttons,
  files in URL-encoded payloads)
* Conditionally mark tests as failing (via test.fail):
   * Non-get/post <Form> methods with JavaScript disabled (remix-run#4420)
   * Form serialization problems with JavaScript enabled (remix-run#4342)
chaance pushed a commit that referenced this issue Oct 28, 2022
* Rework all form tests to run both with and without JavaScript. This way we
  can better detect/prevent regressions or inconsistencies between how <form>
  and <Form> work.
* Dedupe and expand form method tests to ensure we cover all supported Form
  methods and submitter formMethods
* Expand coverage around form serialization (tree order, image submit buttons,
  files in URL-encoded payloads)
* Conditionally mark tests as failing (via test.fail):
   * Non-get/post <Form> methods with JavaScript disabled (#4420)
   * Form serialization problems with JavaScript enabled (#4342)
@brophdawg11
Copy link
Contributor

Thank you for the detailed use cases and spec references @jenseng - these are incredibly helpful 🙌

jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set:

1. Ensure they are serialized in tree order (i.e. where they appear in the DOM)
2. Ensure image inputs are serialized correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Don't send multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
temporarily tweaking the form during submission to get the right entries.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
constructing the form data set ourselves according to the spec.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
constructing the form data set ourselves according to the spec.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
constructing the form data set ourselves according to the spec.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
temporarily tweaking the form during submission to get the right entries.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 1, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
jenseng added a commit to jenseng/remix that referenced this issue Nov 4, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
temporarily tweaking the form during submission to get the right entries.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 5, 2022
Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
constructing the form data set ourselves according to the spec.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/remix that referenced this issue Nov 16, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
jenseng added a commit to jenseng/remix that referenced this issue Dec 6, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
jenseng added a commit to jenseng/remix that referenced this issue Dec 7, 2022
Bring `<Form>` submissions in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value.

References: remix-run#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
kentcdodds pushed a commit that referenced this issue Dec 15, 2022
* Rework all form tests to run both with and without JavaScript. This way we
  can better detect/prevent regressions or inconsistencies between how <form>
  and <Form> work.
* Dedupe and expand form method tests to ensure we cover all supported Form
  methods and submitter formMethods
* Expand coverage around form serialization (tree order, image submit buttons,
  files in URL-encoded payloads)
* Conditionally mark tests as failing (via test.fail):
   * Non-get/post <Form> methods with JavaScript disabled (#4420)
   * Form serialization problems with JavaScript enabled (#4342)
jenseng added a commit to jenseng/react-router that referenced this issue Jan 10, 2023
This is the RR port of remix-run/remix#4475

Bring `<Form>` submissions in line with the spec with respect to how and where
form submitters are serialized within the data set. We accomplish this by
constructing the form data set ourselves according to the spec.

Problems fixed:
1. Serialize submitters in tree order (i.e. where they appear in the DOM)
2. Serialize Image Button submitter correctly (i.e. separate x and y coordinate
   entries, rather than a single empty entry)
3. Stop sending multiple entries in older WebKit

References: remix-run/remix#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set
jenseng added a commit to jenseng/react-router that referenced this issue Jan 12, 2023
This is the RR port of remix-run/remix#4473

Bring FormData serialization in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value, as is the case with a
vanilla `<form>` submission.

Rework various 400-error tests not to rely on the old behavior, as it's no longer
a bad request :)

References: remix-run/remix#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
brophdawg11 pushed a commit to remix-run/react-router that referenced this issue Jan 19, 2023
This is the RR port of remix-run/remix#4473

Bring FormData serialization in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value, as is the case with a
vanilla `<form>` submission.

Rework various 400-error tests not to rely on the old behavior, as it's no longer
a bad request :)

References: remix-run/remix#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Jun 14, 2023
@brophdawg11
Copy link
Contributor

I think this should be resolved when we release Remix 1.18.0 now that the relevant PRs are merged in RR. The latest FormData submitter PR should be released in RR 6.14.0

@brophdawg11 brophdawg11 removed their assignment Jun 14, 2023
@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat:forms
Projects
None yet
Development

No branches or pull requests

3 participants