Implement HTMLFormElement.requestSubmit() by muodov · Pull Request #27100 · servo/servo

muodov · 2020-06-27T00:25:14Z

This PR contains an implementation of HTMLFormElement.requestSubmit()

This is literally my first hundred lines of Rust code, so if I crossed a few sacred lines here and there, please go easy on me :)

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement <form>.requestSubmit() #23417

WPT tests for these changes
There are two tests that still fail because we don't support :invalid CSS selector (see Implement the :valid and :invalid pseudo-classes #10781). I verified that they pass if you change them to not use :invalid. Should be unlocked by Implement :valid :invalid pseudo classes #26729.

highfive · 2020-06-27T00:25:24Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @ferjm (or someone else) soon.

highfive · 2020-06-27T00:25:25Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlformelement.rs, components/script/dom/htmlbuttonelement.rs, components/script/dom/webidls/HTMLFormElement.webidl
@KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlbuttonelement.rs, components/script/dom/webidls/HTMLFormElement.webidl

atouchet · 2020-06-27T01:51:58Z

tests/wpt/metadata/html/semantics/forms/the-form-element/form-requestsubmit.html.ini

 [form-requestsubmit.html]
  [requestSubmit() doesn't run interactive validation reentrantly]
-    expected: FAIL
+    expected: PASS


These passing tests can be removed instead of being marked as PASS.

gterzian

Looks great! Couple of comments...

gterzian · 2020-06-29T07:49:43Z

components/script/dom/htmlformelement.rs

+                let submitters_owner = submit_button.form_owner();
+
+                // Step 1.2
+                if submitters_owner.is_none() ||


let owner = match submitters_owner { Some(owner) => owner, None => return Err(Error::NotFound), }; if *owner != self { return Err(Error::NotFound); }

Done, with a small change: *owner != *self

gterzian · 2020-06-29T07:50:14Z

components/script/dom/htmlformelement.rs

+        let submitter: FormSubmitter = match submitter {
+            Some(submitter_element) => {
+                // Step 1.1
+                let submit_button = match submitter_element.upcast::<Node>().type_id() {


Sort of a nit really, however since you say it's your first hundred lines of Rust:

I think you can simplify this by limited the nesting inside the match, and instead doing a flat series of matches where you would do an early return in the None case, for example:

let element = match match submitter_element.upcast::<Node>().type_id() { Some(element) => element, None => { return Err(Error::Type("submitter must be a submit button".to_string())); } }; let submit_button = match element { HTMLElementTypeId::HTMLInputElement => // HTMLElementTypeId::HTMLButtonElement => // _ => { return Err(Error::Type("submitter must be a submit button".to_string())); } }; if !submit_button.is_submit_button() { return Err(Error::Type("submitter must be a submit button".to_string())); }

You do get some repetition with the error case, but it's much easier to read.

Also regarding the submitter_element.downcast::<HTMLInputElement>().unwrap(), I think it's better to do expect("Failed to downcast submitter elem to HTMLInputElement.") or something similar so that it's easier to figure out what when wrong when it does.

:) As a matter of fact, I was doubting whether I should do this when I was first writing this. Wasn't sure what would be more "Rust-y". I suppose it comes down to a code style really, and I don't have a strong opinion on this, so I changed it as you suggested 👍

gterzian · 2020-06-29T07:50:46Z

components/script/dom/htmlformelement.rs

+        };
+        // Step 3
+        self.submit(SubmittedFrom::NotFromForm, submitter);
+        return Ok(());


nit: no need for the explicit return.

changed it. Implicit returns are hard to get used to :)

gterzian · 2020-06-29T08:01:54Z

components/script/dom/htmlformelement.rs

    InputElement(&'a HTMLInputElement),
-    ButtonElement(&'a HTMLButtonElement), // TODO: image submit, etc etc
+    ButtonElement(&'a HTMLButtonElement),
+    // TODO: the spec doesn't seem to put hard constraints on submitter, so it can theoretically be any element


I think instead of here, we can add a doc to the enum with \\\ <https://html.spec.whatwg.org/multipage/#form-associated-element>, and then add a TODO that would simply state to implement the other types of form associated elements, including custom elements.

gterzian · 2020-06-29T08:13:50Z

components/script/dom/htmlformelement.rs

+            if !submitter.no_validate(self) {
+                if self.interactive_validation().is_err() {
+                    // TODO: Implement event handlers on all form control elements
+                    self.upcast::<EventTarget>().fire_event(atom!("invalid"));


This looks un-necessary, as it is already done at step 6.1 of static validation(which is called into at step 1 of interactive validation).

I didn't change this part, I merely added indentation. Looking at the code you mentioned, I got an impression that static validation handles only elements inside the form, whereas this line fires an event on the form itself. I might be wrong about this, would you mind double-checking?

Ok. You're right about the different event targets, and I actually don't see where in the spec the event is fired on the element as a whole, however if this isn't an actual change, let's leave it for now...

Actually I think I've found where the event should be fired on the element as a whole, and this line appears incorrect, so I've filed a follow-up at #27133

gterzian · 2020-06-29T08:15:48Z

components/script/dom/htmlformelement.rs

-            if self.interactive_validation().is_err() {
-                // TODO: Implement event handlers on all form control elements
-                self.upcast::<EventTarget>().fire_event(atom!("invalid"));
+        if submit_method_flag == SubmittedFrom::NotFromForm {


Please number the various Step 6 substeps, like you've already done with 6.4 below

done, for the whole function (numbers were off, I suppose the spec changed at some point)

gterzian · 2020-06-29T08:18:00Z

components/script/dom/htmlformelement.rs

            let event = event.upcast::<Event>();
            event.fire(self.upcast::<EventTarget>());
+
+            self.firing_submission_events.set(false);


Please number as Step 6.6

gterzian · 2020-06-29T08:21:27Z

components/script/dom/htmlbuttonelement.rs

    }
+
+    #[inline]
+    pub fn button_type(&self) -> ButtonType {


I would rather have a is_submit method on HTMLButtonElement, since the button_type seems to only be used for that purpose(thay way the ButtonType doesn't have to become public either).

Sounds reasonable, done.

gterzian · 2020-06-29T08:45:46Z

components/script/dom/htmlformelement.rs

+    // https://html.spec.whatwg.org/multipage/#concept-submit-button
+    fn is_submit_button(&self) -> bool {
+        match *self {
+            FormSubmitter::InputElement(input_element) => {


I think here we can add a link to https://html.spec.whatwg.org/multipage/#image-button-state-(type=image)

this line handles both type=image and type=submit, I added the links

gterzian · 2020-06-29T08:47:14Z

components/script/dom/htmlformelement.rs

+                input_element.input_type() == InputType::Submit ||
+                    input_element.input_type() == InputType::Image
+            },
+            FormSubmitter::ButtonElement(button_element) => {


and I think this would be https://html.spec.whatwg.org/multipage/#submit-button-state-(type=submit)

I believe this is https://html.spec.whatwg.org/multipage/#attr-button-type-submit-state, added it

muodov

@gterzian Thanks for an in-depth review! I believe I have addressed all the comments, would you mind checking again?

muodov · 2020-06-30T23:08:56Z

components/script/dom/htmlbuttonelement.rs

    }
+
+    #[inline]
+    pub fn button_type(&self) -> ButtonType {


Sounds reasonable, done.

muodov · 2020-06-30T23:11:04Z

components/script/dom/htmlformelement.rs

+        let submitter: FormSubmitter = match submitter {
+            Some(submitter_element) => {
+                // Step 1.1
+                let submit_button = match submitter_element.upcast::<Node>().type_id() {


:) As a matter of fact, I was doubting whether I should do this when I was first writing this. Wasn't sure what would be more "Rust-y". I suppose it comes down to a code style really, and I don't have a strong opinion on this, so I changed it as you suggested 👍

muodov · 2020-06-30T23:11:36Z

components/script/dom/htmlformelement.rs

+                let submitters_owner = submit_button.form_owner();
+
+                // Step 1.2
+                if submitters_owner.is_none() ||


Done, with a small change: *owner != *self

muodov · 2020-06-30T23:12:24Z

components/script/dom/htmlformelement.rs

+        };
+        // Step 3
+        self.submit(SubmittedFrom::NotFromForm, submitter);
+        return Ok(());


changed it. Implicit returns are hard to get used to :)

muodov · 2020-06-30T23:12:39Z

components/script/dom/htmlformelement.rs

-            if self.interactive_validation().is_err() {
-                // TODO: Implement event handlers on all form control elements
-                self.upcast::<EventTarget>().fire_event(atom!("invalid"));
+        if submit_method_flag == SubmittedFrom::NotFromForm {


done, for the whole function (numbers were off, I suppose the spec changed at some point)

muodov · 2020-06-30T23:15:40Z

components/script/dom/htmlformelement.rs

+            if !submitter.no_validate(self) {
+                if self.interactive_validation().is_err() {
+                    // TODO: Implement event handlers on all form control elements
+                    self.upcast::<EventTarget>().fire_event(atom!("invalid"));


I didn't change this part, I merely added indentation. Looking at the code you mentioned, I got an impression that static validation handles only elements inside the form, whereas this line fires an event on the form itself. I might be wrong about this, would you mind double-checking?

muodov · 2020-06-30T23:15:53Z

components/script/dom/htmlformelement.rs

            let event = event.upcast::<Event>();
            event.fire(self.upcast::<EventTarget>());
+
+            self.firing_submission_events.set(false);


muodov · 2020-06-30T23:16:04Z

components/script/dom/htmlformelement.rs

    InputElement(&'a HTMLInputElement),
-    ButtonElement(&'a HTMLButtonElement), // TODO: image submit, etc etc
+    ButtonElement(&'a HTMLButtonElement),
+    // TODO: the spec doesn't seem to put hard constraints on submitter, so it can theoretically be any element


muodov · 2020-06-30T23:16:36Z

components/script/dom/htmlformelement.rs

+    // https://html.spec.whatwg.org/multipage/#concept-submit-button
+    fn is_submit_button(&self) -> bool {
+        match *self {
+            FormSubmitter::InputElement(input_element) => {


this line handles both type=image and type=submit, I added the links

muodov · 2020-06-30T23:17:13Z

components/script/dom/htmlformelement.rs

+                input_element.input_type() == InputType::Submit ||
+                    input_element.input_type() == InputType::Image
+            },
+            FormSubmitter::ButtonElement(button_element) => {


I believe this is https://html.spec.whatwg.org/multipage/#attr-button-type-submit-state, added it

gterzian

Awesome work! Please squash into one commit and then it should be ready to go...

gterzian

The event-firing does appear redundant at that line, and I think it's a good follow-up, since it's not an actual change of this PR.

gterzian · 2020-07-01T02:10:50Z

components/script/dom/htmlformelement.rs

+            if !submitter.no_validate(self) {
+                if self.interactive_validation().is_err() {
+                    // TODO: Implement event handlers on all form control elements
+                    self.upcast::<EventTarget>().fire_event(atom!("invalid"));


Actually I think I've found where the event should be fired on the element as a whole, and this line appears incorrect, so I've filed a follow-up at #27133

muodov · 2020-07-01T15:41:48Z

@gterzian I squashed the commits 👍 I'm happy to work on #27133, it does look like a good follow-up indeed.
Just for my understanding, is it a requirement to squash commits in PRs? The contribution guide doesn't say this, should it be updated?

gterzian · 2020-07-02T02:07:46Z

Just for my understanding, is it a requirement to squash commits in PRs? The contribution guide doesn't say this, should it be updated?

If the PR includes different pieces of work, then it could have multiple commits, so I wouldn't say it's a requirement to always squash. The Github workflow, linked to from the contribution guide, goes into this a bit more I believe.

gterzian · 2020-07-02T02:08:27Z

Thanks!

@bors-servo r+

bors-servo · 2020-07-02T02:08:28Z

📌 Commit 332ad1a has been approved by gterzian

gterzian · 2020-07-02T02:09:24Z

I'm happy to work on #27133, it does look like a good follow-up indeed.

Sounds like a plan!

bors-servo · 2020-07-02T02:52:46Z

⌛ Testing commit 332ad1a with merge 2b060d8...

Implement HTMLFormElement.requestSubmit()  This PR contains an implementation of [HTMLFormElement.requestSubmit()](https://html.spec.whatwg.org/multipage/forms.html#dom-form-requestsubmit) This is literally my first hundred lines of Rust code, so if I crossed a few sacred lines here and there, please go easy on me :) ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23417  - [x] [WPT tests](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-requestsubmit.html) for these changes There are two tests that still fail because we don't support `:invalid` CSS selector (see #10781). I verified that they pass if you change them to not use `:invalid`. Should be unlocked by #26729.

bors-servo · 2020-07-02T03:31:19Z

💔 Test failed - status-taskcluster

gterzian · 2020-07-02T03:54:06Z

0 matches
1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /html/dom/idlharness.https.html?include=HTML.*:
  └ PASS [expected FAIL] HTMLFormElement interface: operation requestSubmit(optional HTMLElement?)
  ▶ Unexpected subtest result in /html/dom/idlharness.https.html?include=HTML.*:
  └ PASS [expected FAIL] HTMLFormElement interface: document.createElement("form") must inherit property "requestSubmit(optional HTMLElement?)" with the proper type
  ▶ Unexpected subtest result in /html/dom/idlharness.https.html?include=HTML.*:
  └ PASS [expected FAIL] HTMLFormElement interface: calling requestSubmit(optional HTMLElement?) on document.createElement("form") with too few arguments must throw TypeError

Great, some more test that require updating to PASS.

…trant form submission behavior

jdm · 2020-07-02T13:10:33Z

@bors-servo r=gterzian

bors-servo · 2020-07-02T13:10:34Z

📌 Commit 8194da2 has been approved by gterzian

bors-servo · 2020-07-02T13:54:49Z

⌛ Testing commit 8194da2 with merge 3bc4a93...

bors-servo · 2020-07-02T15:00:53Z

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 3bc4a93 to master...

@gterzian

Do not fire `invalid` event on form elements This is a follow-up to my [previous PR](#27100) suggested by @gterzian. `invalid` event on the `<form>` element has been [recently removed](whatwg/html#4626) from the spec. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27133  - [x] [WPT test](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/historical.window.js) marked as passing

@gterzian

Do not fire `invalid` event on form elements This is a follow-up to my [previous PR](#27100) suggested by @gterzian. `invalid` event on the `<form>` element has been [recently removed](whatwg/html#4626) from the spec. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27133  - [x] [WPT test](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/historical.window.js) marked as passing

highfive assigned ferjm Jun 27, 2020

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 27, 2020

atouchet reviewed Jun 27, 2020

View reviewed changes

muodov force-pushed the master branch from 080ff17 to 9d9d122 Compare June 27, 2020 09:40

gterzian requested changes Jun 29, 2020

View reviewed changes

gterzian reviewed Jun 29, 2020

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 29, 2020

jdm assigned gterzian and unassigned ferjm Jun 29, 2020

muodov force-pushed the master branch from 9d9d122 to b5e24be Compare June 30, 2020 23:19

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 30, 2020

muodov commented Jun 30, 2020

View reviewed changes

muodov force-pushed the master branch from b5e24be to 19e212d Compare June 30, 2020 23:45

gterzian added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 1, 2020

gterzian approved these changes Jul 1, 2020

View reviewed changes

gterzian reviewed Jul 1, 2020

View reviewed changes

muodov force-pushed the master branch from 19e212d to 332ad1a Compare July 1, 2020 15:37

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jul 2, 2020

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 2, 2020

gterzian mentioned this pull request Jul 2, 2020

Intermittent TIMEOUT /fetch/nosniff/importscripts.html #27144

Closed

Implement HTMLFormElement.requestSubmit()Also includes a fix for reen…

8194da2

…trant form submission behavior

muodov force-pushed the master branch from 332ad1a to 8194da2 Compare July 2, 2020 10:33

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 2, 2020

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 2, 2020

bors-servo merged commit 3bc4a93 into servo:master Jul 2, 2020

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 2, 2020

muodov mentioned this pull request Jul 4, 2020

Do not fire invalid event on form elements #27174

Merged

4 tasks

Uh oh!

Conversation

muodov commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 27, 2020

Uh oh!

highfive commented Jun 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muodov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

muodov commented Jun 27, 2020 •

edited

Loading

gterzian Jun 29, 2020 •

edited

Loading

gterzian Jun 29, 2020 •

edited

Loading

gterzian Jun 29, 2020 •

edited

Loading