[Platform-provided behaviors] Enable form submission [chromium/src : main]
Ana Sollano Kim (Gerrit)
Ana Sollano Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ana Sollano Kim
| Commit-Queue | +1 |
1 comment
Please take a look when you get the chance 😊
Related details
- Dan Clark
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark added 14 comments![]( "Open in Gerrit")
Great to see this coming together!
DCHECK(RuntimeEnabledFeatures::ElementInternalsBehaviorsEnabled());Can this be `CHECK`?
if (submit_button) {I think we should still do this stuff in the `behavior` case. Any reason not to? We're looking for stuff associated with the form, not the button, so this should still work in the same way.
submit_event_init->setSubmitter(event_submitter);Can this whole thing just be this, without the dual casting with `event_submitter`?
`submit_event_init->setSubmitter(DynamicTo<HTMLElement>(submitter));`
if (!internals) {Does it make sense to `CHECK` that we have an internals if `HandleActivation` is called? I'm not sure how it could be triggered if no element is associated.
return disabled_ || (internals && internals->IsActuallyDisabled());Seems `internals->IsActuallyDisabled()` does things like inherit the disabled state from ancestor elements. Are there tests that can be added for that?
HTMLSubmitButtonBehavior* FindSubmitBehavior(const HTMLElement& element) {This helper is implemented in a couple places. Consider just putting it on Element (`SubmitBehavior()` would probably be a better name in that case).
if (behavior && !behavior->name().empty()) {For my own understanding, where is the equivalent happening for the `HTMLFormControlElement` submitter case?
function clickSubmitsForm(element) {It'd be great to also use testdriver.js to ensure that user-initiated actions also submit the form, for:
-- user clicks on the custom element
-- user hits 'enter' on the custom element
-- user hits 'space' on the custom element
-- anything else I'm forgetting?
Or feel free to add these in a follow-up CL.
promise_test(async () => {Can you include tests for `formenctype` and `formmethod`, too?
}, 'formAction on HTMLSubmitButtonBehavior overrides form action');How is this test validating `'formAction on HTMLSubmitButtonBehavior overrides form action'`? `submitAndGetQuery` only checks `window.location.search` rather than the path name so it seems to me like this would pass whether the form sent the data to `'/should-not-be-used.html'` or to `'/common/blank.html'`.
await submitAndCollectFormData(customSubmit);Since this isn't using the result of `submitAndCollectFormData`, it's maybe simpler to do `assert_true(clickSubmitsForm(customSubmit), ...)` rather than reimplementing the `onsubmit` check inside this test.
customSubmit.setAttribute('form', 'test-form');This one is kind of interesting. We're using the attribute directly from the custom element rather than requiring it to be plumbed through to the behavior. In other cases like `disabled` we ignore the attr value on the custom element unless it's been plumbed through.
Is this the right behavior? Is it worth discussing more?
this.attachInternals({ behaviors: [new HTMLSubmitButtonBehavior()] });It doesn't need to be part of this change, but an idle thought: should we emit a console warning when someone uses SubmitButtonBehavior on a non-form-associated custom element? That seems like it'll be a common mistake.
Related details
- Ana Sollano Kim
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
4 comments
Dan's review is great. Other than his comments, just one nit of my own. So I'll mark it LGTM for now, from me.
if (submit_button) {I think we should still do this stuff in the `behavior` case. Any reason not to? We're looking for stuff associated with the form, not the button, so this should still work in the same way.
+1, I think this is a bug. As-is, if submitting via a custom element, the use counters and unclosed element checks will be skipped.
// For behaviors, use `submitter` instead of `submit_button`.I find the one-or-the-other behavior a bit confusing. Perhaps just add this here, to clarify it a little bit?
```
CHECK(!submitter || !submit_button);
```
dom_form_data->AppendFromElement(behavior->name(), behavior->value());Maybe add
```
CHECK(!submit_button);
```
Related details
- Ana Sollano Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
// For behaviors, use `submitter` instead of `submit_button`.I find the one-or-the-other behavior a bit confusing. Perhaps just add this here, to clarify it a little bit?
```
CHECK(!submitter || !submit_button);
```
Done
dom_form_data->AppendFromElement(behavior->name(), behavior->value());Mason FreedMaybe add
```
CHECK(!submit_button);
```
Done
Related details
- Ana Sollano Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |