Bug 197958 – Implement <form>.requestSubmit()

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Domenic Denicola]

Spec discussion/motivation: https://github.com/whatwg/html/issues/4187
Spec PR (merged): https://github.com/whatwg/html/pull/4597
Tests PR (merged): https://github.com/web-platform-tests/wpt/pull/16743
Spec: https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit

[Radar WebKit Bug Importer]

<rdar://problem/51002416>

[Rob Buis]

Created attachment 416579 [details]
Patch

[Rob Buis]

Created attachment 416581 [details]
Patch

[Rob Buis]

Created attachment 416586 [details]
Patch

[Rob Buis]

Created attachment 416596 [details]
Patch

[Rob Buis]

Created attachment 416598 [details]
Patch

[Alex Christensen]

Comment on attachment 416598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416598&action=review

> Source/WebCore/html/HTMLFormElement.cpp:288
> +        document().updateLayoutIgnorePendingStylesheets();

I don't understand why we would need to update layout while preparing to submit a form.

[Rob Buis]

Comment on attachment 416598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416598&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:288
>> +        document().updateLayoutIgnorePendingStylesheets();
> 
> I don't understand why we would need to update layout while preparing to submit a form.

Any call site that calls validateInteractively() needs to have a valid layout since validateInteractively() could call isFocusable(), which ASSERTS that the renderer is laid out (according to the comment validateInteractively).
Looking at the code, the ASSERT and comment may be outdated though, I will try without it.

[Rob Buis]

Created attachment 416608 [details]
Patch

[Rob Buis]

Created attachment 416618 [details]
Patch

[Rob Buis]

Created attachment 416632 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 416632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416632&action=review

> Source/WebCore/html/HTMLFormElement.cpp:323
> +    HTMLFormControlElement* control = nullptr;

Please use RefPtr instead.

> Source/WebCore/html/SubmitInputType.cpp:77
>      if (auto currentForm = protectedElement->form())

Please store currentFrom in RefPtr instead with makeRefPtr.

> Source/WebCore/loader/FormSubmission.cpp:150
> +    if (!submitButton)
> +        submitButton = form.findSubmitButton(event);

This isn't safe. We need to store the result in RefPtr instead.

[Rob Buis]

Created attachment 416820 [details]
Patch

[Darin Adler]

Comment on attachment 416820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416820&action=review

I don’t think the tests are extensive enough. There’s no test that checks passing null, and various edge cases not covered by tests that we might actually have wrong in the current patch.

> Source/WebCore/html/HTMLFormElement.cpp:47
> +#include "InputTypeNames.h"

This is a "bad code smell" and we probably don’t want it.

> Source/WebCore/html/HTMLFormElement.cpp:332
> +    RefPtr<HTMLFormControlElement> control;
> +    if (submitter) {
> +        if (is<HTMLFormControlElement>(submitter))
> +            control = downcast<HTMLFormControlElement>(submitter);
> +        // https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit
> +        if (!control || (control->type() != InputTypeNames::submit() && control->type() != InputTypeNames::image()))
> +            return Exception { TypeError };
> +        if (control->form() != this)
> +            return Exception { NotFoundError };
> +    }

This code should call isSubmitButton since that’s what the HTML specification calls for. It shouldn’t reimplement the "is submit button" rule. To do that, we need to cast to HTMLInputElement rather than just to HTMLFormControlElement. Would be simple.

> Source/WebCore/html/HTMLFormElement.cpp:336
> +    // Update layout before processing form actions in case the style changes
> +    // the Form or button relationships.
> +    document().updateLayoutIgnorePendingStylesheets();

No reason to capitalize "form" here. Do we have this covered by test cases?

> Source/WebCore/html/HTMLFormElement.cpp:381
> +        firstSuccessfulSubmitButton = submitter;

Is it really OK that this skips the "is successful" check? Do we have test cases that cover disabled form controls, since that is what the "successful" check is for?

Is it really OK to skip the loop and possibly end up with two buttons that both have setActivatedSubmit(true)? Do we have test cases that cover that?

> Source/WebCore/html/HTMLFormElement.cpp:387
> +                HTMLFormControlElement& control = downcast<HTMLFormControlElement>(*associatedElement);

Should use auto& here.

> Source/WebCore/html/HTMLFormElement.h:80
> -    void prepareForSubmission(Event&); // FIXME: This function doesn't only prepare, it sometimes calls submit() itself.
> +    void prepareForSubmission(Event*, HTMLFormControlElement* = nullptr, FormSubmissionTrigger = NotSubmittedByJavaScript); // FIXME: This function doesn't only prepare, it sometimes calls submit() itself.

I don’t think the HTMLFormControlElement* argument is self-explanatory in purpose from its type alone. I think it needs a name.

Could we maybe rename this function so we can remove the FIXME? Might require a little studying to come up with a great name.

> Source/WebCore/html/HTMLFormElement.h:109
> -    HTMLFormControlElement* findSubmitButton(const Event*) const;
> +    RefPtr<HTMLFormControlElement> findSubmitButton(const Event*) const;

Why this change? It’s not how we’re generally handling object lifetime. Ryosuke said that local variables need to be RefPtr, but not return values from a function like this.

Super confusing that this function will return a clicked form control that is not a submit button, by the way!

Maybe we should rename this to something like effectiveSubmitter or findSubmitter and have it take a HTMLFormControlElement* submitter argument so all the callers won’t need to have the null check and branching.

> Source/WebCore/html/HTMLFormElement.idl:43
> +    [MayThrowException] undefined requestSubmit(optional HTMLElement submitter);

Specification says "optional HTMLElement?", so both *nullable* and optional. I think we need test cases covering that case since this is currently wrong and no test is passing.

[Rob Buis]

Created attachment 417010 [details]
Patch

[Darin Adler]

Comment on attachment 416820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416820&action=review

>> Source/WebCore/html/HTMLFormElement.idl:43
>> +    [MayThrowException] undefined requestSubmit(optional HTMLElement submitter);
> 
> Specification says "optional HTMLElement?", so both *nullable* and optional. I think we need test cases covering that case since this is currently wrong and no test is passing.

Probably obvious, but I meant to say "currently wrong and no test is *failing*".

[Rob Buis]

Created attachment 417018 [details]
Patch

[Rob Buis]

Created attachment 417023 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 417023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417023&action=review

> Source/WebCore/html/HTMLFormElement.cpp:279
> -        auto submitElement = findSubmitButton(&event);
> +        auto* submitElement = submitter ? submitter : findSubmitter(event);

Please use makeRefPtr here.

> Source/WebCore/html/HTMLFormElement.cpp:335
> +    // Update layout before processing form actions in case the style changes
> +    // the Form or button relationships.

Is that really a thing? Do we have a test for this?

> Source/WebCore/html/HTMLFormElement.cpp:701
> +    if (auto* submitter = overrideSubmitter ? overrideSubmitter : findSubmitter(event)) {

Ditto about using makeRefPtr.

> Source/WebCore/loader/FormSubmission.cpp:149
> +    RefPtr<HTMLFormControlElement> submitter = overrideSubmitter ? overrideSubmitter : form.findSubmitter(event);

Use makeRefPtr?

[Rob Buis]

Created attachment 417119 [details]
Patch

[Darin Adler]

Comment on attachment 417119 [details]
Patch

There are many comments from my original review that this patch does not address. The isSubmitButton comment, for example. Not sure whether this is intended or not.

[Rob Buis]

(In reply to Darin Adler from comment #21)
> Comment on attachment 417119 [details]
> Patch
> 
> There are many comments from my original review that this patch does not
> address. The isSubmitButton comment, for example. Not sure whether this is
> intended or not.

With the amount of review comments I feel it is better to address them with multiple patches, to make sure I do not regress anything along the way.

Regarding isSubmitButton, I tried but it seems we'd not just need to cast to HTMLInputElement and check isSubmitButton, but also need to check HTMLButtonElement which can also be a submit button. That seems to me to be a lot of casting and checking, and not really better than the current solution. WDYT?

[Rob Buis]

Comment on attachment 417023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417023&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:279
>> +        auto* submitElement = submitter ? submitter : findSubmitter(event);
> 
> Please use makeRefPtr here.

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:335
>> +    // the Form or button relationships.
> 
> Is that really a thing? Do we have a test for this?

This was discussed in comment 8.

>> Source/WebCore/html/HTMLFormElement.cpp:701
>> +    if (auto* submitter = overrideSubmitter ? overrideSubmitter : findSubmitter(event)) {
> 
> Ditto about using makeRefPtr.

Done.

>> Source/WebCore/loader/FormSubmission.cpp:149
>> +    RefPtr<HTMLFormControlElement> submitter = overrideSubmitter ? overrideSubmitter : form.findSubmitter(event);
> 
> Use makeRefPtr?

Done.

[Darin Adler]

(In reply to Rob Buis from comment #22)
> (In reply to Darin Adler from comment #21)
> > Comment on attachment 417119 [details]
> > Patch
> > 
> > There are many comments from my original review that this patch does not
> > address. The isSubmitButton comment, for example. Not sure whether this is
> > intended or not.
> 
> With the amount of review comments I feel it is better to address them with
> multiple patches, to make sure I do not regress anything along the way.

That doesn’t make sense for comments that are specially about the newly introduced mode. For example "bypassing checks" comments are not about changes to existing behavior, but about how requestSubmit works and what it does and does not check.

If the comments are about refinement to what’s already there, that’s fine, OK to do them in separate patches.

> Regarding isSubmitButton, I tried but it seems we'd not just need to cast to
> HTMLInputElement and check isSubmitButton, but also need to check
> HTMLButtonElement which can also be a submit button. That seems to me to be
> a lot of casting and checking, and not really better than the current
> solution. WDYT?

I think we should add a virtual member isSubmitButton function to HTMLFormControlElement, have the base implementation return false, mark the function in HTMLInputElement as final, and also add one in HTMLButtonElement, also marked final.

[Rob Buis]

Created attachment 417350 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Darin Adler]

Comment on attachment 417350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417350&action=review

> Source/WebCore/html/HTMLFormElement.cpp:329
> +        if (is<HTMLFormControlElement>(submitter))
> +            control = downcast<HTMLFormControlElement>(submitter);
> +        // https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit
> +        if (!control || !control->isSubmitButton())
> +            return Exception { TypeError };

Could write it this way:

    // https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit
    if (!is<HTMLFormControlElement>(submitter))
        return Exception { TypeError };
    control = downcast<HTMLFormControlElement>(submitter);
    if (!control->isSubmitButton())
        return Exception { TypeError };

I like it a little better. What do you think?

> Source/WebCore/html/ImageInputType.cpp:182
> +bool ImageInputType::isSubmitButton() const
> +{
> +    return true;
> +}

Wow, we had this wrong?!

So what is the effect of fixing on the other callers of isSubmitButton? Is there a progression? No effect?

[Rob Buis]

Created attachment 417621 [details]
Patch

[Rob Buis]

Created attachment 417622 [details]
Patch

[Rob Buis]

Comment on attachment 416820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416820&action=review

>>> Source/WebCore/html/HTMLFormElement.idl:43
>>> +    [MayThrowException] undefined requestSubmit(optional HTMLElement submitter);
>> 
>> Specification says "optional HTMLElement?", so both *nullable* and optional. I think we need test cases covering that case since this is currently wrong and no test is passing.
> 
> Probably obvious, but I meant to say "currently wrong and no test is *failing*".

It turns out we are already testing requestSubmit(null) in html/semantics/forms/form-submission-0/form-submission-algorithm.html.

[Rob Buis]

Comment on attachment 417350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417350&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:329
>> +            return Exception { TypeError };
> 
> Could write it this way:
> 
>     // https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit
>     if (!is<HTMLFormControlElement>(submitter))
>         return Exception { TypeError };
>     control = downcast<HTMLFormControlElement>(submitter);
>     if (!control->isSubmitButton())
>         return Exception { TypeError };
> 
> I like it a little better. What do you think?

I do like it better! Done.

>> Source/WebCore/html/ImageInputType.cpp:182
>> +}
> 
> Wow, we had this wrong?!
> 
> So what is the effect of fixing on the other callers of isSubmitButton? Is there a progression? No effect?

This only effects the tooltip codepath in Chrome::getToolTip. No effect on the tests.

[Rob Buis]

Comment on attachment 417622 [details]
Patch

Maybe you can let me know what needs to be fixed before landing and what can be a separate patch, I lost track a bit :)

[Darin Adler]

Comment on attachment 417622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417622&action=review

Looking quickly I could only find one unaddressed comment that seems like it ought to be part of the initial patch. But it’s not a small one!

> Source/WebCore/html/HTMLFormElement.cpp:382
> +        firstSuccessfulSubmitButton = submitter;

I think it we should investigate, test, and address before committing whether it’s OK to bypass/ignore isActivatedSubmit and isSuccessfulSubmitButton in this case. This is new to requestSubmit, and so belongs as part of adding requestSubmit.

[Rob Buis]

Created attachment 418850 [details]
Patch

[Rob Buis]

Created attachment 418982 [details]
Patch

[Rob Buis]

@annevk I have some questions about form submission related to requestSubmit (https://html.spec.whatwg.org/#dom-form-requestsubmit):

- it is my understanding that requestSubmit can be successful even for submit buttons that are disabled?
- WebKit and blink deal with activation of the submit button in their submit algorithm. Is this related to DOM activation? I can't find anything about activation in https://html.spec.whatwg.org/#concept-form-submit.
- the code (https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/html/HTMLFormElement.cpp#L359) seems to treat activation as a way to control the entry list so maybe the activation concept is just a mechanism for "construct the entry list"(https://html.spec.whatwg.org/#constructing-the-form-data-set).

[Anne van Kesteren]

Yeah, it does not seem to look at the disabled state. I'm not sure if that was considered, but it's definitely what's specified.

I'm not sure about the other question. If you could construct a test that would produce different results between Chromium/WebKit and the standard that would help a lot.

[Darin Adler]

I guess we can build the test cases based on what the isActivatedSubmit and isSuccessfulSubmitButton do, without thinking too deeply about what the correct behavior is; just have the test cases demonstrate the difference those functions make. That could perhaps help us focus more clearly on the design issue?

[Rob Buis]

Created attachment 427589 [details]
Patch

[Rob Buis]

Created attachment 427646 [details]
Patch

[Rob Buis]

Created attachment 427667 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #38)
> I guess we can build the test cases based on what the isActivatedSubmit and
> isSuccessfulSubmitButton do, without thinking too deeply about what the
> correct behavior is; just have the test cases demonstrate the difference
> those functions make. That could perhaps help us focus more clearly on the
> design issue?

I added some tests. Looks like this time the implementations are actually in agreement!

[Darin Adler]

(In reply to Rob Buis from comment #42)
> (In reply to Darin Adler from comment #38)
> > build the test cases based on what the isActivatedSubmit and
> > isSuccessfulSubmitButton do
> 
> I added some tests. Looks like this time the implementations are actually in
> agreement!

That’s great. Do you think these tests cover the most crazy corner cases of those functions, isActivatedSubmit and isSuccessfulSubmitButton?

[Darin Adler]

Comment on attachment 427667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

> Source/WebCore/html/HTMLFormElement.cpp:330
> +    // Update layout before processing form actions in case the style changes
> +    // the Form or button relationships.
> +    document().updateLayoutIgnorePendingStylesheets();

Given our comment it seems wrong to do this *after* checking the control’s membership of the form. That sure sounds like a form and button relationship!

Our comment leaves it unclear why we need this here, but do not need it for other callers of the prepareForSubmission function. Let’s explain our thinking.

I suggest lowercasing "Form" here.

> Source/WebCore/html/HTMLFormElement.cpp:388
> +        for (auto& associatedElement : m_associatedElements) {
> +            if (!is<HTMLFormControlElement>(*associatedElement))
> +                continue;
> +            if (needButtonActivation) {
> +                HTMLFormControlElement& control = downcast<HTMLFormControlElement>(*associatedElement);
> +                if (control.isActivatedSubmit())
> +                    needButtonActivation = false;
> +                else if (!firstSuccessfulSubmitButton && control.isSuccessfulSubmitButton())
> +                    firstSuccessfulSubmitButton = &control;
> +            }
>          }

This loop seems like something that should be factored out into a named function. Could even pass the submitter and include the other side of the if statement too. The return value would be a struct or pair with both the button and the boolean.

> Source/WebCore/html/HTMLFormElement.h:80
> +    void prepareForSubmission(Event*, HTMLFormControlElement* = nullptr, FormSubmissionTrigger = NotSubmittedByJavaScript); // FIXME: This function doesn't only prepare, it sometimes calls submit() itself.

This kind of FIXME seems unnecessary. It’s implying we should rename the function. So let us do that! We can rename a function!

> Source/WebCore/loader/FormSubmission.cpp:149
> +    RefPtr<HTMLFormControlElement> submitter = makeRefPtr(overrideSubmitter ? overrideSubmitter : form.findSubmitter(event));

auto

[Ryosuke Niwa]

Comment on attachment 427667 [details]
Patch

Please add a runtime flag.

[Rob Buis]

Created attachment 427743 [details]
Patch

[Rob Buis]

Created attachment 427776 [details]
Patch

[Rob Buis]

(In reply to Ryosuke Niwa from comment #45)
> Comment on attachment 427667 [details]
> Patch
> 
> Please add a runtime flag.

Done!

[Rob Buis]

Comment on attachment 427667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:330
>> +    document().updateLayoutIgnorePendingStylesheets();
> 
> Given our comment it seems wrong to do this *after* checking the control’s membership of the form. That sure sounds like a form and button relationship!
> 
> Our comment leaves it unclear why we need this here, but do not need it for other callers of the prepareForSubmission function. Let’s explain our thinking.
> 
> I suggest lowercasing "Form" here.

I am not sure how the form and button relationship can change because of style?

The updateLayoutIgnorePendingStylesheets call is needed before calling validateInteractively, see comment 8.

>> Source/WebCore/html/HTMLFormElement.cpp:388
>>          }
> 
> This loop seems like something that should be factored out into a named function. Could even pass the submitter and include the other side of the if statement too. The return value would be a struct or pair with both the button and the boolean.

Done.

>> Source/WebCore/html/HTMLFormElement.h:80
>> +    void prepareForSubmission(Event*, HTMLFormControlElement* = nullptr, FormSubmissionTrigger = NotSubmittedByJavaScript); // FIXME: This function doesn't only prepare, it sometimes calls submit() itself.
> 
> This kind of FIXME seems unnecessary. It’s implying we should rename the function. So let us do that! We can rename a function!

Done.

>> Source/WebCore/loader/FormSubmission.cpp:149
>> +    RefPtr<HTMLFormControlElement> submitter = makeRefPtr(overrideSubmitter ? overrideSubmitter : form.findSubmitter(event));
> 
> auto

Done.

[Rob Buis]

(In reply to Darin Adler from comment #43)
> (In reply to Rob Buis from comment #42)
> > (In reply to Darin Adler from comment #38)
> > > build the test cases based on what the isActivatedSubmit and
> > > isSuccessfulSubmitButton do
> > 
> > I added some tests. Looks like this time the implementations are actually in
> > agreement!
> 
> That’s great. Do you think these tests cover the most crazy corner cases of
> those functions, isActivatedSubmit and isSuccessfulSubmitButton?

I can't think of more tests but happy to add if somebody knows some further scenario to test.

[Ryosuke Niwa]

Comment on attachment 427776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427776&action=review

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:865
> +    WebKitLegacy:
> +      default: true
> +    WebKit:
> +      default: true
> +    WebCore:
> +      default: true

Can we land this feature turned off by default first?
Form submission is a complicated enough feature that
we probably want to put it in STP first and let web developers play with it.

[Rob Buis]

Comment on attachment 427776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427776&action=review

>> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:865
>> +      default: true
> 
> Can we land this feature turned off by default first?
> Form submission is a complicated enough feature that
> we probably want to put it in STP first and let web developers play with it.

Sure, will do, the main thing for me is that the tests pass using WTR, and they should since this Is an experimental feature.

[Rob Buis]

Created attachment 427858 [details]
Patch

[Darin Adler]

Comment on attachment 427667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

>>> Source/WebCore/html/HTMLFormElement.cpp:330
>>> +    document().updateLayoutIgnorePendingStylesheets();
>> 
>> Given our comment it seems wrong to do this *after* checking the control’s membership of the form. That sure sounds like a form and button relationship!
>> 
>> Our comment leaves it unclear why we need this here, but do not need it for other callers of the prepareForSubmission function. Let’s explain our thinking.
>> 
>> I suggest lowercasing "Form" here.
> 
> I am not sure how the form and button relationship can change because of style?
> 
> The updateLayoutIgnorePendingStylesheets call is needed before calling validateInteractively, see comment 8.

Then the comment should not say "in case the style changes the form or button relationships".

I did not arrive at that conclusion independently. I just read the comment we are adding in this patch!

[Rob Buis]

Created attachment 427895 [details]
Patch

[Rob Buis]

Comment on attachment 427667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

>>>> Source/WebCore/html/HTMLFormElement.cpp:330
>>>> +    document().updateLayoutIgnorePendingStylesheets();
>>> 
>>> Given our comment it seems wrong to do this *after* checking the control’s membership of the form. That sure sounds like a form and button relationship!
>>> 
>>> Our comment leaves it unclear why we need this here, but do not need it for other callers of the prepareForSubmission function. Let’s explain our thinking.
>>> 
>>> I suggest lowercasing "Form" here.
>> 
>> I am not sure how the form and button relationship can change because of style?
>> 
>> The updateLayoutIgnorePendingStylesheets call is needed before calling validateInteractively, see comment 8.
> 
> Then the comment should not say "in case the style changes the form or button relationships".
> 
> I did not arrive at that conclusion independently. I just read the comment we are adding in this patch!

Fair enough, I added instead the explanation from comment 8. I can't see bug 185173, so for me it is hard to judge how much the original makes sense.

[Darin Adler]

That security bug bug mentions that updateLayoutIgnorePendingStylesheets can call JavaScript. Given that, it can do almost *anything*, which can indeed include changing form and button relationships, like moving the button into a different form.

[Rob Buis]

(In reply to Darin Adler from comment #57)
> That security bug bug mentions that updateLayoutIgnorePendingStylesheets can
> call JavaScript. Given that, it can do almost *anything*, which can indeed
> include changing form and button relationships, like moving the button into
> a different form.

Ok thanks. Can I have access to that bug or see an example where it calls JS? I lack imagination for that. Of course if that is the case, then I agree the updateLayoutIgnorePendingStylesheets should be done before the first checks in HTMLFormElement::requestSubmit.

[Darin Adler]

I’ll give you access to that security bug. I don’t think that’s quickly going to lead you to understanding exactly how calling updateLayoutIgnorePendingStylesheets can lead to a focus event gets dispatched, but I’m sure it will help a lot of people if you can explain it clearly to the rest of us.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #59)
> I’ll give you access to that security bug. I don’t think that’s quickly
> going to lead you to understanding exactly how calling
> updateLayoutIgnorePendingStylesheets can lead to a focus event gets
> dispatched, but I’m sure it will help a lot of people if you can explain it
> clearly to the rest of us.

autofocus & fragment scrolling can happen inside updateLayoutIgnorePendingStylesheets.

[Rob Buis]

Comment on attachment 427895 [details]
Patch

Thanks, I now see what you mean, will have a look tomorrow, clearing review for now.

[Rob Buis]

Created attachment 427982 [details]
Patch

[Rob Buis]

Created attachment 427990 [details]
Patch

[Rob Buis]

Comment on attachment 427667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427667&action=review

>>>>> Source/WebCore/html/HTMLFormElement.cpp:330
>>>>> +    document().updateLayoutIgnorePendingStylesheets();
>>>> 
>>>> Given our comment it seems wrong to do this *after* checking the control’s membership of the form. That sure sounds like a form and button relationship!
>>>> 
>>>> Our comment leaves it unclear why we need this here, but do not need it for other callers of the prepareForSubmission function. Let’s explain our thinking.
>>>> 
>>>> I suggest lowercasing "Form" here.
>>> 
>>> I am not sure how the form and button relationship can change because of style?
>>> 
>>> The updateLayoutIgnorePendingStylesheets call is needed before calling validateInteractively, see comment 8.
>> 
>> Then the comment should not say "in case the style changes the form or button relationships".
>> 
>> I did not arrive at that conclusion independently. I just read the comment we are adding in this patch!
> 
> Fair enough, I added instead the explanation from comment 8. I can't see bug 185173, so for me it is hard to judge how much the original makes sense.

Ok I added a testcase for this scenario. For that case, indeed it is needed to call updateLayoutIgnorePendingStylesheets before doing the requestSubmit checks!

[Darin Adler]

Comment on attachment 427990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427990&action=review

> Source/WebCore/html/HTMLFormElement.cpp:257
> +void HTMLFormElement::submitIfPossible(Event* event, HTMLFormControlElement* submitter, FormSubmissionTrigger formSubmissionTrigger)

Seems like this parameter could be named trigger, does not need a three-word name.

> Source/WebCore/html/HTMLFormElement.cpp:319
> +    document().updateLayoutIgnorePendingStylesheets();
> +    RefPtr<HTMLFormControlElement> control;

I suggest a blank line between these two.

> Source/WebCore/html/HTMLFormElement.cpp:351
> +std::pair<RefPtr<HTMLFormControlElement>, bool> HTMLFormElement::findSubmitButton(bool activateSubmitButton, HTMLFormControlElement* submitter)

I find the activateSubmitButton argument here hard to understand. Maybe a different name for the parameter would help? Note that the findSubmitButton function itself does not actually activate the button. I think the parameter should be called needButtonActivation.

I also think we probably want to reorder the arguments; seems more logical to have the needsButtonActivation argument after the submitter one to me.

After studying the calling code I see that we don’t need two different return values. The two are always used together, so this can just return the form control element, and null if nothing needs activation.

> Source/WebCore/html/HTMLFormElement.cpp:357
> +    for (auto& associatedElement : m_associatedElements) {

This loop does nothing if needButtonActivation is false. We should restructure so this is expressed with a return statement rather than a boolean. Before the loop we would do this:

    if (!needsButtonActivation)
        return nullptr;

Inside the loop, instead of:

    if (control.isActivatedSubmit())
        needButtonActivation = false;
    else
        ...

It would be:

    if (control.isActivatedSubmit())
        return nullptr;
    ...

And we would not need a bool local variable at all.

> Source/WebCore/html/HTMLFormElement.cpp:361
> +            HTMLFormControlElement& control = downcast<HTMLFormControlElement>(*associatedElement);

auto&

> Source/WebCore/html/HTMLFormElement.cpp:371
> +void HTMLFormElement::submit(Event* event, bool activateSubmitButton, bool processingUserGesture, FormSubmissionTrigger formSubmissionTrigger, HTMLFormControlElement* submitter)

Seems like this parameter could be named trigger, does not need a three-word name.

[Rob Buis]

Created attachment 428129 [details]
Patch

[Rob Buis]

Comment on attachment 427990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427990&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:257
>> +void HTMLFormElement::submitIfPossible(Event* event, HTMLFormControlElement* submitter, FormSubmissionTrigger formSubmissionTrigger)
> 
> Seems like this parameter could be named trigger, does not need a three-word name.

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:319
>> +    RefPtr<HTMLFormControlElement> control;
> 
> I suggest a blank line between these two.

Fixed.

>> Source/WebCore/html/HTMLFormElement.cpp:351
>> +std::pair<RefPtr<HTMLFormControlElement>, bool> HTMLFormElement::findSubmitButton(bool activateSubmitButton, HTMLFormControlElement* submitter)
> 
> I find the activateSubmitButton argument here hard to understand. Maybe a different name for the parameter would help? Note that the findSubmitButton function itself does not actually activate the button. I think the parameter should be called needButtonActivation.
> 
> I also think we probably want to reorder the arguments; seems more logical to have the needsButtonActivation argument after the submitter one to me.
> 
> After studying the calling code I see that we don’t need two different return values. The two are always used together, so this can just return the form control element, and null if nothing needs activation.

Also done.

>> Source/WebCore/html/HTMLFormElement.cpp:357
>> +    for (auto& associatedElement : m_associatedElements) {
> 
> This loop does nothing if needButtonActivation is false. We should restructure so this is expressed with a return statement rather than a boolean. Before the loop we would do this:
> 
>     if (!needsButtonActivation)
>         return nullptr;
> 
> Inside the loop, instead of:
> 
>     if (control.isActivatedSubmit())
>         needButtonActivation = false;
>     else
>         ...
> 
> It would be:
> 
>     if (control.isActivatedSubmit())
>         return nullptr;
>     ...
> 
> And we would not need a bool local variable at all.

That is nicer indeed, fixed.

>> Source/WebCore/html/HTMLFormElement.cpp:361
>> +            HTMLFormControlElement& control = downcast<HTMLFormControlElement>(*associatedElement);
> 
> auto&

Done.

>> Source/WebCore/html/HTMLFormElement.cpp:371
>> +void HTMLFormElement::submit(Event* event, bool activateSubmitButton, bool processingUserGesture, FormSubmissionTrigger formSubmissionTrigger, HTMLFormControlElement* submitter)
> 
> Seems like this parameter could be named trigger, does not need a three-word name.

Sure, fixed.

[Darin Adler]

Comment on attachment 428129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428129&action=review

> Source/WTF/ChangeLog:8
> +        Add requestSubmit as experimental feature, enabled by default.

disabled by default -- you changed that at Ryosuke’s request

[Rob Buis]

Created attachment 428156 [details]
Patch

[EWS]

Committed r277257 (237526@main): <https://commits.webkit.org/237526@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428156 [details].

[Chris Dumez]

Why is this feature disabled by default? What's missing to enable it?
We're the only browser not supporting this and I'd really like to enable it.

[Darin Adler]

Ryosuke? I think that question is for you.

[Sam Sneddon [:gsnedders]]

(In reply to Chris Dumez from comment #71)
> Why is this feature disabled by default? What's missing to enable it?
> We're the only browser not supporting this and I'd really like to enable it.


Does the above review comment by Ryouske not answer that?

(In reply to Ryosuke Niwa from comment #51)
> Comment on attachment 427776 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427776&action=review
> 
> > Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:865
> > +    WebKitLegacy:
> > +      default: true
> > +    WebKit:
> > +      default: true
> > +    WebCore:
> > +      default: true
> 
> Can we land this feature turned off by default first?
> Form submission is a complicated enough feature that
> we probably want to put it in STP first and let web developers play with it.