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 method="dialog"> should probably not check for noopener #8499

Closed
nt1m opened this issue Nov 14, 2022 · 10 comments · Fixed by #8943
Closed

<form method="dialog"> should probably not check for noopener #8499

nt1m opened this issue Nov 14, 2022 · 10 comments · Fixed by #8943
Labels
topic: dialog The <dialog> element. topic: forms

Comments

@nt1m
Copy link
Member

nt1m commented Nov 14, 2022

According to the spec, we should be checking for the noopener, and then skip to the dialog steps.

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm:get-an-element's-noopener

I think it probably makes sense to skip to the dialog steps earlier, before we even do any navigation checks, given form[method=dialog] does not involve any navigation.

cc @domenic @annevk

@nt1m nt1m added the agenda+ To be discussed at a triage meeting label Nov 14, 2022
@nt1m
Copy link
Member Author

nt1m commented Nov 14, 2022

cc @whatwg/forms too

@annevk
Copy link
Member

annevk commented Nov 14, 2022

So currently <form method=dialog rel=noopener> fails to submit (due to an early return) and with this change it would succeed (or at least continue beyond that point). Seems reasonable to me for the reason you state.

Edit: this analysis was incorrect. The problem is that we end up creating a top-level navigable we don't need.

@domenic
Copy link
Member

domenic commented Nov 16, 2022

Seems good to me. So this would consist of moving what is currently steps 16 and 23 up to after step 9, I think? That is, we still want to:

  • Check "cannot navigate" (multiple times)
  • Check sandboxing
  • Fire submit events
  • Construct the entry list and thus fire formdata events

But we do not want to:

  • Bail early if the action="" value is not parseable as a URL
  • Call "the rules for choosing a navigable" including potentially creating a new window, consulting target/formtarget, etc.

Would you be up for writing a spec PR and tests PR covering these?

@annevk
Copy link
Member

annevk commented Nov 16, 2022

Presumably we just have to move

If the value of method is dialog then jump to the submit dialog steps.

up? Makes sense to ignore action parsing as well. (Ideally we'd not have "jump", but that's a bit more effort.)

@domenic
Copy link
Member

domenic commented Nov 16, 2022

Right, that's step 23; step 16 is establishing the value of method.

@smaug----
Copy link
Collaborator

@sefeng211

@annevk annevk added the topic: dialog The <dialog> element. label Nov 17, 2022
@josepharhar
Copy link
Contributor

Sounds good to me. Based on what I'm reading in chromium it seems like this is what we are already doing.
The check for method=dialog comes before what I believe is the check for the target navigable.

@past past removed the agenda+ To be discussed at a triage meeting label Dec 13, 2022
@smaug----
Copy link
Collaborator

Looks reasonable. @avandolder

@avandolder
Copy link

Yeah, this should also line up with how Gecko already handles submissions - we check for dialog before handling rel values.

annevk added a commit that referenced this issue Feb 24, 2023
Also refactor "submit dialog", "close the dialog", and their callers.

Note that removing the open attribute check is safe as "close the dialog" also handles that.

Tests: ...

Fixes #8499.
@annevk
Copy link
Member

annevk commented Mar 5, 2023

FYI, I ended up finding a "bug" in Gecko. It takes the form's action into account. Bug linked from the PR.

annevk added a commit that referenced this issue Mar 6, 2023
Also refactor "submit dialog", "close the dialog", and their callers.

Note that removing the open attribute check is safe as "close the dialog" also handles that.

Tests: web-platform-tests/wpt#38819.

Fixes #8499.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: dialog The <dialog> element. topic: forms
Development

Successfully merging a pull request may close this issue.

7 participants