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

Fix form submission's replace-before-load logic #9156

Merged
merged 2 commits into from May 10, 2023
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 13, 2023

In 524485a, we overhauled the logic which converts "push" navigations to "replace" navigations before the load event fires. However, for form submissions we erroneously checked the loading state of the form's document, instead of the loading state of the target document; they can be different when using the form's target="" attribute.

Further investigation reveals that this area is not interoperable, and neither Gecko nor WebKit pass tests for the "push"-to-"replace" conversion for such cross-document form submissions. (It's not clear exactly what they do, as they don't pass tests for the opposite behavior either.) Since we don't have interop here, we can standardize on a simpler behavior, of just avoiding the "push"-to-"replace" conversion.

Closes #9135.


(See WHATWG Working Mode: Changes for more details.)


/form-control-infrastructure.html ( diff )

In 524485a, we overhauled the logic which converts "push" navigations to "replace" navigations before the load event fires. However, for form submissions we erroneously checked the loading state of the form's document, instead of the loading state of the target document. Fix that.

Closes #9135.
@domenic domenic added do not merge yet Pull request must not be merged per rationale in comment topic: navigation topic: forms interop Implementations are not interoperable with each other labels Apr 13, 2023
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Apr 27, 2023
@domenic domenic requested review from zcorpan and annevk April 27, 2023 03:29
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for all form submissions or only POST? It would make more sense to me if that was the dividing line and <form method=get> was the same as <a>.

@domenic
Copy link
Member Author

domenic commented May 8, 2023

This is for all form submissions. I think form method=get is more similar to form method=post, than it is to anchor elements. (And in terms of both spec and implementation code, that seems to be the case, in terms of requiring fewer special cases.)

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'll rubberstamp, but it doesn't make a whole lot of sense to me that form navigations have different session history behavior from other navigations. I could see that for POST navigations though.

@domenic domenic merged commit d67f8f7 into main May 10, 2023
2 checks passed
@domenic domenic deleted the fix-replace-before-load branch May 10, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: forms topic: navigation
Development

Successfully merging this pull request may close these issues.

Form submission does a replace load depending on whether the form document has completely loaded
2 participants