[webkit-reviews] review granted: [Bug 197958] Implement <form>.requestSubmit() : [Attachment 416820] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 2 16:31:32 PST 2021


Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 197958: Implement <form>.requestSubmit()
https://bugs.webkit.org/show_bug.cgi?id=197958

Attachment 416820: Patch

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




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 416820
  --> https://bugs.webkit.org/attachment.cgi?id=416820
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.


More information about the webkit-reviews mailing list