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

Improve spec compliance of validity check #27133

Closed
gterzian opened this issue Jul 1, 2020 · 6 comments
Closed

Improve spec compliance of validity check #27133

gterzian opened this issue Jul 1, 2020 · 6 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Jul 1, 2020

This line https://github.com/servo/servo/pull/27100/files#diff-db2031d1f23ecf1a921fc100cfed753dR677

appears incorrect, since the spec only says to fire events on the fields that are invalid, via step 6.1 of https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#statically-validate-the-constraints, which is called into by https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#concept-form-submit

An event is fired on the form element as a whole only as part of the "check validity steps", and those are called into from other places(for example the checkValidity method).

So the implementation of checkValidaty could also be updated to actually fire an event.

see components/script/dom/htmlformelement.rs.

@muodov
Copy link
Contributor

@muodov muodov commented Jul 4, 2020

@highfive assign me

@highfive
Copy link

@highfive highfive commented Jul 4, 2020

Hey @muodov! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Jul 4, 2020
@muodov
Copy link
Contributor

@muodov muodov commented Jul 4, 2020

Turns out, this is quite a recent change in HTML spec. The invalid event on the <form> element was removed.
So it seems like we can just remove that line in the form submission implementation.

Links:

The validation methods (checkValidity() and reportValidity()) of the <form> element specifically don't mention this event either. The "check validity steps" and "report validity steps" are actually never referred to from the validation section of the <form>, so they only apply to form controls and not the form itself. Those cases are already covered in current implementation.

All in all, it seems like I will just need to remove the line and enable this test :)

@muodov muodov mentioned this issue Jul 4, 2020
4 of 4 tasks complete
@gterzian
Copy link
Member Author

@gterzian gterzian commented Jul 5, 2020

Ok, thanks for looking into this, good research!

The validation methods (checkValidity() and reportValidity()) of the

element specifically don't mention this event either.

Re this, doesn't checkValidity directly call into the check validity steps that fire this event?

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 bors-servo closed this in d93e67a Jul 5, 2020
@gterzian gterzian reopened this Jul 5, 2020
@gterzian gterzian changed the title Improve spec compliance of form submit and validity check Improve spec compliance of validity check Jul 5, 2020
@muodov
Copy link
Contributor

@muodov muodov commented Jul 5, 2020

@gterzian it took me some time to figure it out, but checkValidity() in "The constraint validation API" has nothing to do with the checkValidity() of the form element.
The constraint validation API is explicitly referred to from form controls specs, for example input element

@gterzian
Copy link
Member Author

@gterzian gterzian commented Jul 5, 2020

Ah ok, so those I was referring to are about form controls, not the form element, as you already had pointed out. Thanks!

@gterzian gterzian closed this Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.