Change html-macro's checked attribute to specify checkedness (#206)

This PR changes the meaning of `checked` as specified in the `html!` macro.

Originally, specifying `checked` would add or remove the `checked` HTML attribute. This changed the default checkedness of the input element, but may or may not actually change the rendered state of the element.

Now, specifying `checked` directly specifies the rendered state of the element. (The default checkedness is not modified, and can be set manually.)

---

## Motivation

This change makes it easier to do the more commonly-desired task of specifying the rendered state of the checkbox (likely according to application state) rather than merely the default checkedness.

This change mitigates shooting ourselves in the foot by assuming `checked` sets a rendered checkbox's checkedness.

---

Further discussion is available in the issue #205 as well as in the new book entry and tests

https://github.com/SFBdragon/percy/tree/checked_sets_checkedness/book/src/html-macro/special-attributes

https://github.com/SFBdragon/percy/blob/checked_sets_checkedness/crates/percy-dom/tests/checked_attribute.rs

This resolves #205 for the time being.

Cargo.toml

@@ -24,6 +24,7 @@ members = [
   "crates/percy-router-macro-test",
   "crates/virtual-node",
   "examples/component-preview",
+  "examples/embed-non-percy-node",
   "examples/isomorphic/app",
   "examples/isomorphic/client",
   "examples/isomorphic/server",

book/src/SUMMARY.md

@@ -16,6 +16,7 @@
       - [On Create Element](./html-macro/real-elements-and-nodes/on-create-elem/README.md)
       - [On Remove Element](./html-macro/real-elements-and-nodes/on-remove-elem/README.md)
     - [Boolean Attributes](./html-macro/boolean-attributes/README.md)
+    - [Special Attributes](./html-macro/special-attributes/README.md)
   - [Lists](./lists/README.md)
   - [Virtual DOM](./virtual-dom/README.md)
     - [Unit Testing your Views](./virtual-dom/unit-testing-views.md)

book/src/html-macro/special-attributes/README.md

@@ -0,0 +1,35 @@
+# Special Virtual DOM Attributes
+
+Some virtual DOM attributes do not merely set or remove the corresponding HTML attribute of the same name.
+
+## `checked`
+
+According to the [HTML spec](https://html.spec.whatwg.org/multipage/input.html#attr-input-checked), the `checked` HTML attribute only controls the default checkedness.
+Changing the `checked` HTML attribute may not cause the checkbox's checkedness to change.
+
+By contrast: specifying `html! { <input checked={bool} /> }` causes `percy` to always render the checkbox with the specified checkedness.
+- If the VDOM is updated from `html! { <input checked=true /> }` to `html { <input checked=false /> }`, the input element's checkedness will definitely change.
+- If the VDOM is updated from `html! { <input checked=true /> }` to `html { <input checked=true /> }`, the input element's checkedness will be reverted to `true` even if the user interacted with the checkbox in between.
+
+`percy-dom` updates the `checked` property (current checkedness, not reflected in HTML).
+
+This behavior is more desirable because `percy` developers are accustomed to declaratively controlling the DOM and rendered HTML.
+
+`percy-dom` does not affect the presence of the `checked` attribute (default checkedness, reflected in HTML).
+
+This means that if you need to configure the `checked` attribute (this should almost never be the case if your GUI state is driven by the backend state) then `percy-dom` won't get in your way. 
+
+## `value`
+
+According to the [HTML spec](https://html.spec.whatwg.org/multipage/input.html#attr-input-value), the `value` HTML attribute only controls the default value.
+Changing the `value` HTML attribute may not cause the input element's value to change.
+
+By contrast: specifying `html! { <input value="..." /> }` causes `percy` to always render the input element with the specified value.
+- If the VDOM is updated from `html! { <input value="hello" /> }` to `html { <input value="goodbye" /> }`, the input element's value will definitely change.
+- If the VDOM is updated from `html! { <input value="hello" /> }` to `html { <input value="hello" /> }`, the input element's value will be reverted to `"hello"` even if the user interacted with the input element in between.
+
+`percy` updates both
+- the `value` attribute (default value, reflected in HTML) and,
+- the `value` property (current value, not reflected in HTML).
+
+Setting the `value` property is desirable because `percy` developers are accustomed to declaratively controlling the DOM and rendered HTML.

crates/percy-dom/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "percy-dom"
-version = "0.9.9"
+version = "0.10.0"
 authors = ["Chinedu Francis Nwafili <[email protected]>"]
 description = "A standalone Virtual DOM creation, diffing and patching implementation"
 keywords = ["virtual", "dom", "wasm", "assembly", "webassembly"]

crates/percy-dom/src/diff.rs

@@ -282,6 +282,8 @@ fn find_attributes_to_add<'a>(
                     attributes_to_add.insert(new_attr_name, new_attr_val);
                 } else if new_attr_name == "value" {
                     ctx.push_patch(Patch::ValueAttributeUnchanged(cur_node_idx, new_attr_val));
+                } else if new_attr_name == "checked" {
+                    ctx.push_patch(Patch::CheckedAttributeUnchanged(cur_node_idx, new_attr_val));
                 }
             }
             None => {
@@ -1246,6 +1248,37 @@ mod tests {
         .test();
     }
 
+    /// Verify that if an input (e.g. checkbox) has `checked` specified, we always push a
+    /// patch for setting the checked attribute and property so that the checkbox is
+    /// rendered in the specified state, regardless if any intervening input has occurred.
+    #[test]
+    fn always_pushes_patch_for_checked() {
+        for checkedness in [false, true] {
+            DiffTestCase {
+                old: html! { <input checked={checkedness} /> },
+                new: html! { <input checked={checkedness} /> },
+                expected: vec![Patch::CheckedAttributeUnchanged(0, &checkedness.into())],
+            }
+            .test();
+        }
+
+        for old_checkedness in [false, true] {
+            let new_checkedness = !old_checkedness;
+
+            DiffTestCase {
+                old: html! { <input checked=old_checkedness /> },
+                new: html! { <input checked=new_checkedness /> },
+                expected: vec![Patch::AddAttributes(
+                    0,
+                    vec![("checked", &new_checkedness.into())]
+                        .into_iter()
+                        .collect(),
+                )],
+            }
+            .test();
+        }
+    }
+
     /// Verify that we push an on create elem patch if the new node has the special attribute
     /// and the old node does not.
     #[test]

crates/percy-dom/src/patch.rs

@@ -100,6 +100,10 @@ pub enum Patch<'a> {
     /// The value attribute of a textarea or input element has not changed, but we will still patch
     /// it anyway in case something was typed into the field.
     ValueAttributeUnchanged(BreadthFirstNodeIdx, &'a AttributeValue),
+    /// The checked attribute of a input element has not changed, but we will still patch
+    /// it anyway in case the checkbox was toggled by the user. Otherwise the checkbox
+    /// could have a different state to what is rendered.
+    CheckedAttributeUnchanged(BreadthFirstNodeIdx, &'a AttributeValue),
     /// Add attributes that the new node has that the old node does not
     AddAttributes(BreadthFirstNodeIdx, HashMap<&'a str, &'a AttributeValue>),
     /// Remove attributes that the old node had that the new node doesn't
@@ -153,6 +157,7 @@ impl<'a> Patch<'a> {
             Patch::RemoveAttributes(node_idx, _) => *node_idx,
             Patch::ChangeText(node_idx, _) => *node_idx,
             Patch::ValueAttributeUnchanged(node_idx, _) => *node_idx,
+            Patch::CheckedAttributeUnchanged(node_idx, _) => *node_idx,
             Patch::SpecialAttribute(special) => match special {
                 PatchSpecialAttribute::CallOnCreateElemOnExistingNode(node_idx, _) => *node_idx,
                 PatchSpecialAttribute::SetDangerousInnerHtml(node_idx, _) => *node_idx,
@@ -210,6 +215,9 @@ impl<'a> Patch<'a> {
             Patch::ValueAttributeUnchanged(node_idx, _) => {
                 to_find.insert(*node_idx);
             }
+            Patch::CheckedAttributeUnchanged(node_idx, _) => {
+                to_find.insert(*node_idx);
+            }
             Patch::SpecialAttribute(special) => match special {
                 PatchSpecialAttribute::CallOnCreateElemOnExistingNode(node_idx, _) => {
                     to_find.insert(*node_idx);

crates/percy-dom/src/patch/apply_patches.rs

@@ -241,7 +241,13 @@ fn apply_element_patch(
                         }
                     }
                     AttributeValue::Bool(val_bool) => {
-                        if *val_bool {
+                        // Use `set_checked` instead of `{set,remove}_attribute` for the `checked` attribute.
+                        // The "checked" attribute only determines default checkedness,
+                        // but `percy-dom` takes `checked` to specify the actual checkedness.
+                        // See crates/percy-dom/tests/checked_attribute.rs for more info.
+                        if *attrib_name == "checked" {
+                            maybe_set_checked_property(node, *val_bool);
+                        } else if *val_bool {
                             node.set_attribute(attrib_name, "")?;
                         } else {
                             node.remove_attribute(attrib_name)?;
@@ -390,6 +396,11 @@ fn apply_element_patch(
 
             Ok(())
         }
+        Patch::CheckedAttributeUnchanged(_node_idx, value) => {
+            maybe_set_checked_property(node, value.as_bool().unwrap());
+
+            Ok(())
+        }
         Patch::SpecialAttribute(special) => match special {
             PatchSpecialAttribute::CallOnCreateElemOnExistingNode(_node_idx, new_node) => {
                 new_node
@@ -512,6 +523,13 @@ fn maybe_set_value_property(node: &Element, value: &str) {
     }
 }
 
+// See crates/percy-dom/tests/checked_attribute.rs
+fn maybe_set_checked_property(node: &Element, checked: bool) {
+    if let Some(input_node) = node.dyn_ref::<HtmlInputElement>() {
+        input_node.set_checked(checked);
+    }
+}
+
 // Looks for a property on the element. If it's there then this is a Percy element.
 //
 // TODO: We need to know not just if the node was created by Percy... but if it was created by

crates/percy-dom/tests/checked_property.rs

@@ -0,0 +1,186 @@
+//! `percy-dom` treats the `checked` virtual node attribute specially.
+//! `percy-dom` sets the `checked` element property (actual checkedness),
+//! not the `checked` HTML attribute (default checkedness).
+//!
+//! Developers, are likely to assume that `checked` specifies the state of the checkbox
+//! directly. `percy-dom` ensures that this is true.
+//!
+//! See the tests for more details. Start with [`patch_uses_set_checked_function`].
+//!
+//! To run all tests in this file:
+//!
+//! ```sh
+//! wasm-pack test --chrome --headless crates/percy-dom --test checked_property
+//! ```
+
+use wasm_bindgen_test::*;
+
+use percy_dom::event::VirtualEvents;
+use wasm_bindgen::JsCast;
+use web_sys::*;
+
+use percy_dom::prelude::*;
+
+wasm_bindgen_test_configure!(run_in_browser);
+
+/// Verify that `percy_dom::patch` uses `set_checked` to set the checkedness
+/// of an input element when specified.
+///
+/// ## Why?
+///
+/// The `checked` HTML attribute only determines the default checkedness.
+/// The browser uses the default checkedness as the checkbox's
+/// checkedness until the user clicks on the checkbox, setting the "dirty checked" browser
+/// flag https://html.spec.whatwg.org/multipage/input.html#concept-input-checked-dirty
+/// which results in the browser maintaining the checkbox's state INDEPENDENTLY from the
+/// default checked state. i.e. Changing the default checkedness no longer affects the actual
+/// checkedness after the user has pressed the input.
+///
+/// We want `html!{ ... checked=val ... }` to specify the current checkedness of the checkbox
+/// directly - avoiding the checkbox being toggled differently to what the developer
+/// specified in the virtual DOM.
+///
+/// Using web-sys's `set_checked` sets the actual checkbox's checkedness. Futhermore, it enables the
+/// dirty-checked flag (NB: BUT ONLY WHEN THE CHECKBOX STATE IS CHANGED), which we can test for.
+///
+/// ## Test approach
+///
+/// - Create a virtual node with the checkbox having checkedness !C, and patch it to have checkedness C.
+///     (This should cause the dirty flag to be set IF `set_checked` is used.)
+/// - Assert that the corresponding DOM element has checkedness of C.
+///
+/// - Now, remove the attribute if the checkbox is checked, or set the attribute if not.
+///     (The checkbox should hold its state as the dirty flag is checked, therefore
+///     changing the default checkedness through the `checked` attribute no longer
+///     should affect the checkedness of the checkbox.)
+/// - Assert that the checkedness of the checkbox element is still B.
+#[wasm_bindgen_test]
+fn patch_sets_checked_property() {
+    for checkedness in [false, true] {
+        let start_input = html! {<input checked={!checkedness}>};
+        let end_input = html! {<input checked=checkedness>};
+
+        let mut events = VirtualEvents::new();
+        let (input_node, enode) = start_input.create_dom_node(&mut events);
+        events.set_root(enode);
+
+        let input_elem = input_node.dyn_ref::<HtmlInputElement>().unwrap();
+
+        let patches = percy_dom::diff(&start_input, &end_input);
+        percy_dom::patch(input_node.clone(), &end_input, &mut events, &patches).unwrap();
+        assert_eq!(input_elem.checked(), checkedness);
+
+        if checkedness {
+            input_elem.remove_attribute("checked").unwrap();
+        } else {
+            input_elem.set_attribute("checked", "").unwrap();
+        }
+        assert_eq!(input_elem.checked(), checkedness);
+    }
+}
+
+/// Verify that `percy_dom::patch` uses `set_checked` to set the `checked` property
+/// of an input element even if the the specified `checked` value does not change
+/// between the `old` and `new` virtual nodes.
+///
+/// ## Why?
+///
+/// Note: the rationale given in [`patch_sets_checked_property`] is prerequisite reading.
+///
+/// The user might interact with the checkbox in between the previous render and the next
+/// one, changing the checkedness state in the browser, but `diff` would not realize this
+/// assuming the rendered `checked` value does not change.
+///
+/// For example:
+/// - Developer renders `html! { ... checked=true ... }`
+/// - User clicks on the checkbox, changing the browser's checkbox checkedness to false.
+/// - Developer renders `html! { ... checked=true ... }`
+/// - `diff` doesn't realize anything needs to change, so it doesn't issue any changes.
+/// - Developer is still trying to render the checkbox as checked but the browser checkbox
+///     stays unchecked.
+///
+/// If `percy_dom::diff` always specifies that `percy_dom::patch` should set the `checked`
+/// property if its specified, then the above cannot happen. The element's checked state
+/// will be fixed when `percy_dom::patch` is called, keeping the developer-specified `checked`
+/// value and the checkbox element's visual state in sync.
+///
+/// ## Test approach
+///
+/// - Create a a DOM node with the checkbox having checkedness C.
+/// - Set it's checkedness to be !C.
+/// - Diff and patch with the virtual node still specifying it's checkedness as C.
+/// - Assert that the checkedness has been reset to C, even though the virtual node did not change.
+#[wasm_bindgen_test]
+fn patch_always_sets_checked_property() {
+    for checkedness in [false, true] {
+        let start_input = html! {<input checked=checkedness>};
+        let end_input = html! {<input checked=checkedness>};
+
+        let mut events = VirtualEvents::new();
+        let (input_node, enode) = start_input.create_dom_node(&mut events);
+        events.set_root(enode);
+
+        let input_elem = input_node.dyn_ref::<HtmlInputElement>().unwrap();
+        assert_eq!(input_elem.checked(), checkedness);
+
+        input_elem.set_checked(!checkedness); // modify checked property
+
+        let patches = percy_dom::diff(&start_input, &end_input);
+        percy_dom::patch(input_node.clone(), &end_input, &mut events, &patches).unwrap();
+
+        assert_eq!(input_elem.checked(), checkedness);
+    }
+}
+
+/// Verify that `percy_dom::patch` does not set the default checkedness.
+/// That is to say: it does NOT manipulate the HTML `checked` attribute.
+///
+/// ## Why?
+///
+/// Note: the rationale given in [`patch_sets_checked_property`] is prerequisite reading.
+///
+/// `percy` is intended to serve the developer who is making a nontrivial app who's state
+/// is driven by the backend of the application. The application state is _not_ driven by
+/// the frontend. Therefore the state of checkboxes is specified explicitly idiomatically.
+///
+/// `percy-dom` does not currently allow for a way to directly manipulate the default
+/// checkedness of an input element. Default checkedness is not useful unless the state
+/// of an input element is driven by the frontend.
+///
+/// Users may want to break out of this mold however, and modify the default checkedness.
+/// In this case, it's much simpler if `percy` doesn't change the default checkedness
+/// unnecessarily. Equivalently, `percy-dom` does not manipulate the presence of the HTML
+/// `checked` attribute.
+///
+/// ## Test approach
+///
+/// - Create an input element with checkedness C.
+/// - Set the presence of the HTML attribute (default checkedness) to be D.
+/// - Update the input element's checkedness to be E.
+/// - Assert that the default checkedness is still D.
+#[wasm_bindgen_test]
+fn patch_does_not_set_default_checkedness() {
+    for prev_checkedness in [false, true] {
+        for next_checkedness in [false, true] {
+            for default_checkedness in [false, true] {
+                let start_input = html! {<input checked=prev_checkedness>};
+                let end_input = html! {<input checked=next_checkedness>};
+
+                let mut events = VirtualEvents::new();
+                let (input_node, enode) = start_input.create_dom_node(&mut events);
+                events.set_root(enode);
+
+                let input_elem = input_node.dyn_ref::<HtmlInputElement>().unwrap();
+                assert_eq!(input_elem.checked(), prev_checkedness);
+
+                // Modify presence of `checked`` attribute.
+                input_elem.set_default_checked(default_checkedness);
+
+                let patches = percy_dom::diff(&start_input, &end_input);
+                percy_dom::patch(input_node.clone(), &end_input, &mut events, &patches).unwrap();
+
+                assert_eq!(input_elem.default_checked(), default_checkedness);
+            }
+        }
+    }
+}

crates/percy-router/src/route.rs

@@ -216,7 +216,7 @@ mod tests {
                 ("/users/foo", false),
             ],
         }
-            .test();
+        .test();
     }
 
     /// Verify that a `/` route definition doesn't capture `/some-route`.
@@ -230,7 +230,7 @@ mod tests {
                 ("/foo", false),
             ],
         }
-            .test();
+        .test();
     }
 
     /// Verify that we correctly match when a static segment comes after a dynamic segment.
@@ -247,7 +247,7 @@ mod tests {
                 ("/5/bar", false),
             ],
         }
-            .test();
+        .test();
     }
 
     struct MatchRouteTestCase {