Implement form-associated custom elements and their ElementInternals #31980

cathiechen · 2024-04-02T17:47:38Z

Form-associated elements are now working.

Remaining test failures in custom-elements/form-associated/ are:

Allow setting default accessibility semantics for custom elements whatwg/html#4658 hasn't made it into the standard yet, and ElementInternals-accessibility is all about that functionality.
ElementInternals-setFormValue hits many race conditions with our iframe order of operations. Isolating just the "submit this form with these custom elements, look at the query" part, the payload of each test actually does pass! Running the tests as written, we end up with different tests overwriting each other's elements and getting very confused. One problem is the about:blank double-load-event thing, but fixing that doesn't fix everything.
ElementInternals-validation doesn't work because we don't have form validation in general; I've commented the relevant stubs.
ElementInternals-form-disabled-callback fails on Mutating the tabindex content attribute isn't changing focusability #25713.

This patch is originally from #25705

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement attachInternals and ElementInternals #24988, Origin offset fixes #25025, Intermittent failure with TypeError: Network error occurred #25027
There are tests for these changes

…use the code in Validatable trait. 3. The form associated custom element is not a customized built-in element.

servo-wpt-sync · 2024-04-02T17:49:45Z

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#45471) with upstreamable changes.

servo-wpt-sync · 2024-04-04T12:26:19Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

mrobinson

There's a lot going on here, so I wasn't able to check the logic of everything. I have some comments, but I don't see any glaring issues with this change.

components/script/dom/customelementregistry.rs

components/script/dom/htmlformelement.rs

components/script/dom/raredata.rs

…tion

servo-wpt-sync · 2024-04-08T16:26:40Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

servo-wpt-sync · 2024-04-10T16:46:43Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

cathiechen

To make "readonly" effective to element with internals.

cathiechen · 2024-04-10T16:48:37Z

components/script/dom/elementinternals.rs

+        // or the element has a datalist element ancestor.
+        !self.as_element().read_write_state() &&
+            !self.as_element().disabled_state() &&
+            !is_barred_by_datalist_ancestor(self.target_element.upcast::<Node>())


The readonly attribute affects "barred from constraint validation"

cathiechen · 2024-04-10T16:50:36Z

components/script/dom/htmlelement.rs

+                        },
+                    }
+                }
+            },


Update the read_write_state when "readonly" changes.

cathiechen

The changes to fix the WPT failures.

cathiechen · 2024-04-10T16:53:01Z

components/script/dom/bindings/htmlconstructor.rs

-            if definition.form_associated {
-                element.upcast::<Element>().set_enabled_state(true);
-            }
-


Move this change to AttachInternals which also check the invalid state.

Nice cleanup.

cathiechen · 2024-04-10T16:53:10Z

components/script/dom/create.rs

-                                // of enabled/disabled/not-form-control and
-                                // becoming a form control is a change.
-                                element.upcast::<Element>().set_enabled_state(true);
-                            }


Move this change to AttachInternals which also check the invalid state.

cathiechen · 2024-04-10T16:55:54Z

components/script/dom/customelementregistry.rs

@@ -867,6 +867,7 @@ pub fn upgrade_element(definition: Rc<CustomElementDefinition>, element: &Elemen
            // so it doesn't describe this check as an action.)
            element.check_disabled_attribute();
            element.check_ancestors_disabled_state_for_form_control();
+            element.check_readonly_attribute();


We should also check the readonly attribute after upgrade, so that the value of is_instance_validatable will be correct.

cathiechen · 2024-04-10T16:57:37Z

components/script/dom/element.rs

@@ -3137,6 +3138,9 @@ impl VirtualMethods for Element {
        self.super_type().unwrap().unbind_from_tree(context);

        if let Some(f) = self.as_maybe_form_control() {
+            // TODO: The valid state of ancestors might be wrong if the form control element
+            // is with a fieldset ancestor, for instance: `<form><fieldset><input>`,
+            // if `<input>` is unbinded, `<form><fieldset>` should `update_validity()`.


This affects the test case "[Custom control affects :valid :invalid for FORM and FIELDSET]" in ElementInternals-validation.html

cathiechen · 2024-04-10T17:02:27Z

components/script/dom/element.rs

+            return internals.is_invalid();
+        }
+        true
+    }


Because validatable including the form control elements and elements with internals, it is not easy to check both outside element.rs, so we create functions here.

This function checks if the form control elements or elements with internals are invalid or not.

cathiechen · 2024-04-10T17:16:05Z

components/script/dom/htmlelement.rs

            }
+            element.check_readonly_attribute();


It seems unbind_from_tree won't affect the "readonly" attribute, so maybe we could remove this change.

cathiechen · 2024-04-10T17:16:34Z

components/script/dom/htmlfieldsetelement.rs

-                    false
-                }
-            });
+            .any(|element| element.is_invalid(false));


Use is_invalid instead, and consider of element with internals.

cathiechen · 2024-04-10T17:17:06Z

components/script/dom/htmlformelement.rs

-                validatable.is_instance_validatable() &&
-                    !validatable.validity_state().invalid_flags().is_empty()
-            });
+        let is_any_invalid = controls.iter().any(|control| control.is_invalid(false));


Use is_invalid, and consider element with internals case.

cathiechen · 2024-04-10T17:17:32Z

components/script/dom/htmlformelement.rs

-                        None
+                if let Some(element) = field.downcast::<Element>() {
+                    if element.is_invalid(true) {
+                        Some(DomRoot::from_ref(element))


Same, use is_invalid instead.

cathiechen · 2024-04-10T17:18:21Z

components/script/dom/validitystate.rs

-                self.element.set_state(ElementState::INVALID, false);
-            }
+    pub fn update_pseudo_classes(&self) {
+        if self.element.is_instance_validatable() {


Use is_instance_validatable instead, and handle with the element with internals case.

mrobinson · 2024-04-10T17:25:03Z

components/script/dom/bindings/htmlconstructor.rs

-            if definition.form_associated {
-                element.upcast::<Element>().set_enabled_state(true);
-            }
-


Nice cleanup.

components/script/dom/customelementregistry.rs

components/script/dom/element.rs

components/script/dom/elementinternals.rs

mrobinson · 2024-04-10T17:35:14Z

components/script/dom/elementinternals.rs

-        if !bits.is_empty() && !message.iter().any(|m| m.len() > 0) {
+        // Step 3: If flags contains one or more true values and message is not given or is the empty
+        // string, then throw a TypeError.
+        let bits = ValidationFlags::from(flags);


I think this can be an into() call. I can try to make this change.

OK, thanks:) Then I'll leave it as it is for now.

servo-wpt-sync · 2024-04-10T18:03:34Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

servo-wpt-sync · 2024-04-10T18:36:00Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

mrobinson

It seems that many tests are failing now unfortunately: https://github.com/servo/servo/runs/23680344451

I'm not sure if this was due to changes made during this final review round or before.

mrobinson · 2024-04-10T22:17:27Z

components/script/dom/htmlelement.rs

            }
-            element.check_readonly_attribute();


I notice that this was removed, perhaps it was inadvertent?

I did it intentional, let me take a loot at the failures:)

servo-wpt-sync · 2024-04-11T10:58:02Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

mrobinson · 2024-04-11T12:25:39Z

Looks like this just needs some expectations updates: https://github.com/servo/servo/runs/23705832258

cathiechen · 2024-04-11T12:30:47Z

Looks like this just needs some expectations updates: https://github.com/servo/servo/runs/23705832258

Yeah, true, will do:)

cathiechen · 2024-04-11T12:48:04Z

Done!

servo-wpt-sync · 2024-04-11T12:49:50Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#45471).

servo-wpt-sync · 2024-04-11T12:51:05Z

✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#45471) title and body.

pshaughn and others added 2 commits April 2, 2024 19:27

FACEs work, setFormValue test is awful so now has _mozilla backup

d3df6b1

1. Impl Validatable in ElementInternals instead of HTMLElement. 2. Re…

e10082b

…use the code in Validatable trait. 3. The form associated custom element is not a customized built-in element.

servo-wpt-sync mentioned this pull request Apr 2, 2024

Implement form-associated custom elements and their ElementInternals web-platform-tests/wpt#45471

Merged

add some comments

a7cae7b

mrobinson requested changes Apr 4, 2024

View reviewed changes

cathiechen added 3 commits April 5, 2024 17:29

support readonly attribute and complete barred from constraint valida…

3d256d6

…tion

Addressed the code review comments

949dc37

Updated the legacy-layout results

797a8fa

Fixed the WPT failures in ElementInternals-validation.html

77eb91c

cathiechen commented Apr 10, 2024

View reviewed changes

mrobinson approved these changes Apr 10, 2024

View reviewed changes

Addressed the code review comments

af2dd17

Review suggestions

541252a

mrobinson added this pull request to the merge queue Apr 10, 2024

mrobinson mentioned this pull request Apr 10, 2024

form-associated custom elements and their ElementInternals #25705

Closed

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2024

mrobinson reviewed Apr 10, 2024

View reviewed changes

Fixed silly mistakes and update the test result outside elementinternals

c4736ca

mrobinson approved these changes Apr 11, 2024

View reviewed changes

mrobinson enabled auto-merge April 11, 2024 10:55

mrobinson added this pull request to the merge queue Apr 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2024

update the test results

fb0b91d

mrobinson enabled auto-merge April 11, 2024 12:49

mrobinson added this pull request to the merge queue Apr 11, 2024

Merged via the queue into servo:main with commit 4e4a4c0 Apr 11, 2024
10 checks passed

atouchet mentioned this pull request Apr 12, 2024

Intermittent failure with TypeError: Network error occurred #25027

Open

nolanlawson mentioned this pull request Apr 14, 2024

Implement ARIAMixin on Element #32079

Open

Implement form-associated custom elements and their ElementInternals #31980

Implement form-associated custom elements and their ElementInternals #31980

Conversation

cathiechen commented Apr 2, 2024 • edited by mrobinson

servo-wpt-sync commented Apr 2, 2024

servo-wpt-sync commented Apr 4, 2024

mrobinson left a comment

Choose a reason for hiding this comment

servo-wpt-sync commented Apr 8, 2024

servo-wpt-sync commented Apr 10, 2024

cathiechen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathiechen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

servo-wpt-sync commented Apr 10, 2024

servo-wpt-sync commented Apr 10, 2024

mrobinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

servo-wpt-sync commented Apr 11, 2024

mrobinson commented Apr 11, 2024

cathiechen commented Apr 11, 2024

cathiechen commented Apr 11, 2024

servo-wpt-sync commented Apr 11, 2024

servo-wpt-sync commented Apr 11, 2024

cathiechen commented Apr 2, 2024 •

edited by mrobinson