test: expand test coverage around forms

[jenseng]

Expand test coverage around forms:

• 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 (Deprecate non-GET/POST <Form> methods? Or polyfill when scripts are missing/pending/disabled? #4420)
  • Form serialization problems with JavaScript enabled (<Form> submitter is serialized out of tree order #4342)

References: #4342

• Tests

[changeset-bot]

⚠️ No Changeset found

Latest commit: 718cef5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[machour]

Thank you for that js/no-js refactoring 🙌🏼

Shouldn't the tests related to #4342 fail?

[jenseng]

@machour the new tests do fail with JavaScript enabled, but they are annotated with test.fail(...) to indicate that they are supposed to fail, which makes them pass if they fail and fail if they pass 🙃. See here and here... it's the playwright equivalent of @ts-expect-error

So when we fix the behavior, we'll need to remove those annotations as well, otherwise the tests will start failing because they are passing 😆

[jenseng]

I'm thinking it might be good to merge this PR independently of the actual fixes, since it expands coverage generally and gives us a little more flexibility in how/when we fix ... e.g. file handling fix could be separate from submitter fixes, and I have two proposals on how to fix the latter (implement the spec vs. do some temporary form DOM shenanigans)

[machour]

@jenseng agreed. I'll get eyes on this soon!

[mcansh]

thank you @jenseng! 🙌

didn't realize these we were only running these with js enabled 😱

edit: looks like something went awry on dev which so all integration tests are currently failing there but they passed in the PR that was merged... i'll merge this one once we get that cleaned up

[jenseng]

In light of this imminent fix I'll rebase and amend the tests, as this is one instance where <form> and <Form> should behave differently

[brophdawg11]

This looks great @jenseng - thank you!

[chaance]

@jenseng Have you already started working on fixes to these issues or should we prepare for that internally? I'd like to prioritize these bugs for the next release if possible.

[jenseng]

@chaance I've got fixes/PRs basically ready, was just holding off till this gets merged... IMO the url-encoded-files fix should probably be a separate PR from the submitter fixes, and I have two proposals/approaches for the latter

[brophdawg11]

@jenseng Would you mind tagging me on those as well? We'll want the same changes in react-router-dom's <Form>/useSubmit so we don't lose them when RR 6.4 is migrated into Remix. I can copy your changes over there once they're good to go in Remix 👍