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

Implement HTMLFormElement.requestSubmit() #27100

Merged
merged 1 commit into from Jul 2, 2020
Merged

Conversation

muodov
Copy link
Contributor

@muodov muodov commented Jun 27, 2020

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 :)


@highfive
Copy link

highfive commented Jun 27, 2020

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
Copy link

highfive commented Jun 27, 2020

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

@@ -1,22 +1,22 @@
[form-requestsubmit.html]
[requestSubmit() doesn't run interactive validation reentrantly]
expected: FAIL
expected: PASS
Copy link
Contributor

@atouchet atouchet Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gterzian gterzian left a comment

Looks great! Couple of comments...

let submitters_owner = submit_button.form_owner();

// Step 1.2
if submitters_owner.is_none() ||
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if *owner != self {
   return Err(Error::NotFound);
}

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) 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 👍

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

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for the explicit return.

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -1236,7 +1293,8 @@ pub enum FormMethod {
pub enum FormSubmitter<'a> {
FormElement(&'a HTMLFormElement),
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
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

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"));
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@gterzian gterzian Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@gterzian gterzian Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 {
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -630,17 +684,20 @@ impl HTMLFormElement {
);
let event = event.upcast::<Event>();
event.fire(self.upcast::<EventTarget>());

self.firing_submission_events.set(false);
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please number as Step 6.6

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -77,6 +77,11 @@ impl HTMLButtonElement {
document,
)
}

#[inline]
pub fn button_type(&self) -> ButtonType {
Copy link
Member

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, done.

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

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@gterzian gterzian Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@muodov muodov left a comment

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

@@ -77,6 +77,11 @@ impl HTMLButtonElement {
document,
)
}

#[inline]
pub fn button_type(&self) -> ButtonType {
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, done.

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

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) 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 👍

let submitters_owner = submit_button.form_owner();

// Step 1.2
if submitters_owner.is_none() ||
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
// Step 3
self.submit(SubmittedFrom::NotFromForm, submitter);
return Ok(());
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 {
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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"));
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -630,17 +684,20 @@ impl HTMLFormElement {
);
let event = event.upcast::<Event>();
event.fire(self.upcast::<EventTarget>());

self.firing_submission_events.set(false);
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1236,7 +1293,8 @@ pub enum FormMethod {
pub enum FormSubmitter<'a> {
FormElement(&'a HTMLFormElement),
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
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

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

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

input_element.input_type() == InputType::Submit ||
input_element.input_type() == InputType::Image
},
FormSubmitter::ButtonElement(button_element) => {
Copy link
Contributor Author

@muodov muodov Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@gterzian gterzian left a comment

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

Copy link
Member

@gterzian gterzian left a comment

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.

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"));
Copy link
Member

@gterzian gterzian Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

muodov commented Jul 1, 2020

@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
Copy link
Member

gterzian commented Jul 2, 2020

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
Copy link
Member

gterzian commented Jul 2, 2020

Thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

📌 Commit 332ad1a has been approved by gterzian

@gterzian
Copy link
Member

gterzian commented Jul 2, 2020

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

Sounds like a plan!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

Testing commit 332ad1a with merge 2b060d8...

bors-servo added a commit that referenced this issue Jul 2, 2020
Implement HTMLFormElement.requestSubmit()

<!-- Please describe your changes on the following line: -->
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 :)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23417

<!-- Either: -->
- [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.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

💔 Test failed - status-taskcluster

@gterzian
Copy link
Member

gterzian commented Jul 2, 2020

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.

@jdm
Copy link
Member

jdm commented Jul 2, 2020

@bors-servo r=gterzian

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

📌 Commit 8194da2 has been approved by gterzian

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

Testing commit 8194da2 with merge 3bc4a93...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2020

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

@bors-servo bors-servo merged commit 3bc4a93 into servo:master Jul 2, 2020
2 checks passed
bors-servo added a commit that referenced this issue Jul 5, 2020
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27133

<!-- Either: -->
- [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
bors-servo added a commit that referenced this issue Jul 5, 2020
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27133

<!-- Either: -->
- [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants