Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement the form owner concept #15283
Conversation
highfive
commented
Jan 28, 2017
|
Heads up! This PR modifies the following files:
|
|
r? @jdm |
|
@bors-servo try |
Implement the form owner concept <!-- Please describe your changes on the following line: --> Implemention of the core logic for form owners. It's rebased version of #6613, addressed most of the comments and fixed some of the code. I needed to update the code by hand instead of rebasing old main commit because of the massive merge conflicts over the years. Currently I'm using my html5ever fork to run the tests. I'll undo the changes in Cargo.toml and Cargo.lock once it gets merged. --- <!-- 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 #3553 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15283) <!-- Reviewable:end -->
|
|
|
|
@jdm I think it's ready to review now :) Failing tests are passing and removed some fail expectations. I want to test it one more time to see if everything is ok. |
Implement the form owner concept <!-- Please describe your changes on the following line: --> Implemention of the core logic for form owners. It's rebased version of #6613, addressed most of the comments and fixed some of the code. I needed to update the code by hand instead of rebasing old main commit because of the massive merge conflicts over the years. The html5ever PR is here: servo/html5ever#249 Currently I'm using my html5ever fork to run the tests. I'll undo the changes in Cargo.toml and Cargo.lock once it gets merged. --- <!-- 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 #3553 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15283) <!-- Reviewable:end -->
|
|
|
I have only skimmed the changes so far, but I would like to find a way to reduce all of the duplicate implementations of FormControl, bind_to_tree, etc. What if we added a |
|
I suppose we can do that, I'll try this. |
|
|
|
I moved the There are still FormControl's methods to move. But they look like they can't removed easily(AFAIK). Because |
|
Thank you so much for tackling this! I've left a bunch of comments, but I appreciate the work you did to update the original PR and clean up some of the code duplication. |
| @@ -2043,6 +2057,7 @@ impl Document { | |||
| ignore_destructive_writes_counter: Default::default(), | |||
| dom_count: Cell::new(1), | |||
| fullscreen_element: MutNullableJS::new(None), | |||
| form_id_listener_map: DOMRefCell::new(HashMap::new()), | |||
This comment has been minimized.
This comment has been minimized.
| let mut data_set = Vec::new(); | ||
| for child in node.traverse_preorder() { | ||
| for child in controls.iter() { | ||
| let child = child.upcast::<Node>(); | ||
| // Step 3.1: The field element is disabled. | ||
| match child.downcast::<Element>() { |
This comment has been minimized.
This comment has been minimized.
jdm
Feb 14, 2017
Member
Downcasting to Element right after upcasting away from it is a bit silly. Let's do:
for child in controls.iter() {
// Step 3.1: The field is disabled.
if child.disabled_state() {
continue;
}
let child = child.upcast::<Node>();
...
}| @@ -646,14 +651,28 @@ impl HTMLFormElement { | |||
| } | |||
| NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::HTMLOutputElement)) => { | |||
| // Unimplemented | |||
| {} | |||
| unimplemented!() | |||
This comment has been minimized.
This comment has been minimized.
jdm
Feb 14, 2017
Member
It's not clear to me that we actually want to panic if some web content calls reset() on a form that contains an <output> element.
This comment has been minimized.
This comment has been minimized.
canova
Feb 15, 2017
Author
Member
Yeah, at first I thought this can panic here but later I realized it's breaking some tests. Removed it afterwards.
|
|
||
| fn to_element<'a>(&'a self) -> &'a Element; | ||
|
|
||
| fn is_reassociatable(&self) -> bool { |
This comment has been minimized.
This comment has been minimized.
| placeholder.appendChild(form1); | ||
| placeholder.appendChild(form2); | ||
|
|
||
| test(function() { |
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,234 @@ | |||
| <!DOCTYPE html> | |||
This comment has been minimized.
This comment has been minimized.
jdm
Feb 14, 2017
Member
All of these tests belong in tests/wpt/web-platform-tests/html/semantics/forms/form-control-infrastructure/ since they're testing behaviour that is described in the specification.
| @@ -651,7 +652,7 @@ impl HTMLFormElement { | |||
| } | |||
| NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::HTMLOutputElement)) => { | |||
| // Unimplemented | |||
| unimplemented!() | |||
| {} | |||
This comment has been minimized.
This comment has been minimized.
| @@ -174,11 +172,18 @@ impl HTMLLabelElement { | |||
|
|
|||
| impl FormControl for HTMLLabelElement { | |||
| fn form_owner(&self) -> Option<Root<HTMLFormElement>> { | |||
| self.form_owner.get() | |||
| if let Some(e) = self.GetControl() { | |||
This comment has been minimized.
This comment has been minimized.
jdm
Feb 14, 2017
•
Member
self.GetControl().and_then(|e| {
let form_control = e.upcast::<Element>().as_maybe_form_control()
.expect("Element must be a form control");
form_control.form_owner()
})| // FIXME: We should call `form_attribute_mutated` method here, | ||
| // but also we don't want to call reset_form_owner() method in | ||
| // `form_attribute_mutated`. We should fix this. | ||
| // self.form_attribute_mutated(AttributeMutation::Removed); |
This comment has been minimized.
This comment has been minimized.
jdm
Feb 14, 2017
Member
Let's split form_attribute_mutated into register_if_necessary, unregister_if_necessary, and form_attribute_mutated. form_attribute_mutated can call the other two methods based on the mutation, and code like bind_form_control_to_tree can call those methods and reset_form_owner as needed.
|
|
canova
force-pushed the
canova:form-owner
branch
2 times, most recently
from
f9267e9
to
32b964d
|
@jdm Sorry couldn't look at this PR recently. I've addressed most of your comments and rebased.
I've looked at the algorithm and the spec but couldn't find the reason. It was like this in the previous PR. Saw your comment about this line and thought it should be necessary:
|
|
|
|
|
|
@jdm Stealing this from you, I'm sure it will make you insanely angry to see people steal reviews from your queue. |
|
Superseded by #15938, thanks @mukilan and @canaltinova. |
canova commentedJan 28, 2017
•
edited
Implemention of the core logic for form owners. It's rebased version of #6613, addressed most of the comments and fixed some of the code. I needed to update the code by hand instead of rebasing old main commit because of the massive merge conflicts over the years.
The html5ever PR is here: servo/html5ever#249
Currently I'm using my html5ever fork to run the tests. I'll undo the changes in Cargo.toml and Cargo.lock once it gets merged.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is