fix: checkbox activation, ordering and live state

.github/workflows/ci.yml

@@ -443,3 +443,23 @@ jobs:
           bun run build
       - name: Drive the fixture through the real process boundary
         run: cargo test -p ps-blitz-script --features debug-control --test debug_control
+      # Activation behaviour, driven by ps-qa against this checkout.
+      #
+      # The host is built here rather than installed: `qa-inspect-host` depends
+      # on the published engine, so an installed one judges the last release and
+      # a change in this workspace could not fail it. `ps-qa` itself carries no
+      # engine, so the published binary drives whatever host it is given.
+      - uses: actions/checkout@v5
+        with:
+          repository: pathscale/ps-observability
+          path: .ps-observability
+      # Both binaries from the same checkout, rather than `cargo install ps-qa`.
+      # The harness and the host move together, and installing one while
+      # building the other makes this job depend on a release of the harness as
+      # well as on the checkout, so a change to either has to be published
+      # before it can be used here.
+      - name: Drive the fixtures with ps-qa
+        env:
+          PS_OBSERVABILITY: .ps-observability
+          QA_BUILD_PS_QA: "1"
+        run: tests/qa/run.sh

Cargo.toml

@@ -39,7 +39,7 @@ exclude = ["sites", "packages/blitz-wasm/guest"]
 resolver = "2"
 
 [workspace.package]
-version = "0.4.2"
+version = "0.4.3"
 license = "MIT OR Apache-2.0"
 homepage = "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/pathscale/ps-blitz"
 repository = "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/pathscale/ps-blitz"

packages/blitz-dom/src/document.rs

@@ -151,6 +151,14 @@ pub trait Document: Any + 'static {
     }
 }
 
+/// What the pre-click activation steps changed, so a cancelled click can put
+/// it back. Produced by [`BaseDocument::run_pre_click_activation`].
+#[derive(Debug, Clone, PartialEq)]
+pub struct PreClickActivation {
+    /// Every node whose checkedness moved, paired with the value it held.
+    previous: Vec<(NodeId, bool)>,
+}
+
 pub struct PlainDocument(pub BaseDocument);
 impl Document for PlainDocument {
     fn inner(&self) -> DocGuard<'_> {
@@ -756,6 +764,79 @@ impl BaseDocument {
         }
     }
 
+    /// The checkedness a click changed, kept so the click's *canceled
+    /// activation steps* can put it back when a listener calls
+    /// `preventDefault()`.
+    ///
+    /// A radio carries its whole set, because selecting one clears the others.
+    pub fn run_pre_click_activation(&mut self, target: NodeId) -> Option<PreClickActivation> {
+        let node_id = crate::events::pointer::checkable_activation_target(self, target)?;
+        let el = self.get_node(node_id)?.data.downcast_element()?;
+        let is_radio = el.attr(local_name!("type")) == Some("radio");
+
+        if !is_radio {
+            let previous = el.checkbox_input_checked()?;
+            let el = self.get_node_mut(node_id)?.data.downcast_element_mut()?;
+            Self::toggle_checkbox(el);
+            return Some(PreClickActivation {
+                previous: vec![(node_id, previous)],
+            });
+        }
+
+        let radio_set = el.attr(local_name!("name")).map(str::to_string);
+        let Some(radio_set) = radio_set else {
+            let previous = el.checkbox_input_checked()?;
+            let el = self.get_node_mut(node_id)?.data.downcast_element_mut()?;
+            *el.checkbox_input_checked_mut()? = true;
+            return Some(PreClickActivation {
+                previous: vec![(node_id, previous)],
+            });
+        };
+
+        // Recorded *while* selecting rather than before it. Selecting one radio
+        // clears every other in the set, so cancelling has to restore all of
+        // them, and reading them first meant walking the whole arena twice for
+        // one press. The membership test is `toggle_radio`'s, deliberately: two
+        // predicates that disagreed would restore a different set than the one
+        // that changed.
+        //
+        // That predicate is name plus "has checkbox state", which is neither
+        // scoped to `type=radio` nor to a form owner. Wrong per HTML, and
+        // longstanding; matching it here keeps this change to the ordering it
+        // is about.
+        let mut previous: Vec<(NodeId, bool)> = Vec::new();
+        for (id, node) in self.nodes.iter_mut() {
+            let Some(el) = node.data.downcast_element_mut() else {
+                continue;
+            };
+            if el.attr(local_name!("name")) != Some(&*radio_set) {
+                continue;
+            }
+            let Some(is_checked) = el.checkbox_input_checked_mut() else {
+                continue;
+            };
+            previous.push((id, *is_checked));
+            *is_checked = id == node_id;
+        }
+        Some(PreClickActivation { previous })
+    }
+
+    /// Undo [`Self::run_pre_click_activation`]. The click's *canceled
+    /// activation steps*.
+    pub fn undo_pre_click_activation(&mut self, activation: PreClickActivation) {
+        for (node_id, was_checked) in activation.previous {
+            let Some(node) = self.get_node_mut(node_id) else {
+                continue;
+            };
+            let Some(el) = node.data.downcast_element_mut() else {
+                continue;
+            };
+            if let Some(is_checked) = el.checkbox_input_checked_mut() {
+                *is_checked = was_checked;
+            }
+        }
+    }
+
     pub fn toggle_checkbox(el: &mut ElementData) -> bool {
         let Some(is_checked) = el.checkbox_input_checked_mut() else {
             return false;

packages/blitz-dom/src/events/driver.rs

@@ -341,8 +341,26 @@ impl<'doc, Handler: EventHandler> EventDriver<'doc, Handler> {
 
     fn process_queue(&mut self) {
         while let Some(mut event) = self.queue.pop_front() {
+            // HTML runs a click's *pre-click activation steps* before the event
+            // is dispatched, so a checkbox or radio has already taken its new
+            // value by the time any listener runs. Doing it afterwards -- as
+            // the default action, with everything else -- hands `click`
+            // listeners the value from before the press, which reads as a
+            // control that responds to every second press. Cancelling the click
+            // runs the canceled activation steps and puts it back.
+            let activation = match event.data {
+                DomEventData::Click(_) => {
+                    self.doc.inner_mut().run_pre_click_activation(event.target)
+                }
+                _ => None,
+            };
+
             let event_state = self.run_handler_event(&mut event, EventState::default());
-            if !event_state.is_cancelled() {
+            if event_state.is_cancelled() {
+                if let Some(activation) = activation {
+                    self.doc.inner_mut().undo_pre_click_activation(activation);
+                }
+            } else {
                 self.run_default_action(&mut event);
             }
         }

packages/blitz-dom/src/events/mod.rs

@@ -2,7 +2,7 @@ mod driver;
 mod focus;
 mod ime;
 mod keyboard;
-mod pointer;
+pub(crate) mod pointer;
 
 use crate::util::Point;
 use blitz_traits::events::{DomEvent, DomEventData, PointerCoords, UiEvent};

packages/blitz-dom/src/events/pointer.rs

@@ -597,6 +597,56 @@ pub(crate) fn handle_pointerup<F: FnMut(DomEvent)>(
     }
 }
 
+/// The checkbox or radio that a click at `target` activates, or `None` if the
+/// click activates something else.
+///
+/// This walks the tree the way [`handle_click`] does -- stopping at a disabled
+/// element or a text input -- because the two have to agree. The pre-click
+/// activation steps flip the checkedness of whatever this returns, and
+/// `handle_click` reports it afterwards; if they disagreed, a control would
+/// either toggle without an `input` event or fire one without having toggled.
+///
+/// A `<label>` ends the walk with `None`. The label is the nearest thing with
+/// an activation behaviour of its own, and that behaviour is to fire a *fresh*
+/// click at the control -- which arrives here again with the control as its
+/// target. Following the label from here as well would toggle twice and leave
+/// the control exactly as it was.
+pub(crate) fn checkable_activation_target(doc: &BaseDocument, target: NodeId) -> Option<NodeId> {
+    let mut maybe_node_id = Some(target);
+
+    while let Some(node_id) = maybe_node_id {
+        let node = doc.get_node(node_id)?;
+        let Some(el) = node.data.downcast_element() else {
+            maybe_node_id = node.parent;
+            continue;
+        };
+
+        if el.attr(local_name!("disabled")).is_some() {
+            return None;
+        }
+        if let SpecialElementData::TextInput(_) = el.special_data {
+            return None;
+        }
+
+        match el.name.local {
+            local_name!("input")
+                if matches!(
+                    el.attr(local_name!("type")),
+                    Some("checkbox") | Some("radio")
+                ) =>
+            {
+                return Some(node_id);
+            }
+            local_name!("label") => return None,
+            _ => {}
+        }
+
+        maybe_node_id = node.parent;
+    }
+
+    None
+}
+
 pub(crate) fn handle_click(
     doc: &mut BaseDocument,
     target: NodeId,
@@ -629,7 +679,10 @@ pub(crate) fn handle_click(
 
             match el.name.local {
                 local_name!("input") if el.attr(local_name!("type")) == Some("checkbox") => {
-                    let is_checked = BaseDocument::toggle_checkbox(el);
+                    // Checkedness was already flipped by the pre-click
+                    // activation steps, so this is only the post-click half:
+                    // report the value the press produced.
+                    let is_checked = el.checkbox_input_checked().unwrap_or(false);
                     let value = is_checked.to_string();
                     dispatch_event(DomEvent::new(
                         node_id,
@@ -645,12 +698,7 @@ pub(crate) fn handle_click(
                     break 'matched true;
                 }
                 local_name!("input") if el.attr(local_name!("type")) == Some("radio") => {
-                    if let Some(radio_set) = el.attr(local_name!("name")).map(str::to_string) {
-                        BaseDocument::toggle_radio(doc, radio_set, node_id);
-                    } else if let Some(is_checked) = el.checkbox_input_checked_mut() {
-                        *is_checked = true;
-                    }
-
+                    // The selection was made by the pre-click activation steps.
                     // TODO: make input event conditional on value actually changing
                     let value = String::from("true");
                     dispatch_event(DomEvent::new(
@@ -695,15 +743,33 @@ pub(crate) fn handle_click(
                         }
                     }
                 }
-                // Clicking labels triggers click, and possibly input event, of associated input
+                // A label's activation behaviour is to fire a click at the
+                // control it labels. Dispatching that click -- rather than
+                // running the control's default action here -- is what puts it
+                // through the pre-click activation steps and past every
+                // listener, so a press on the label is indistinguishable from a
+                // press on the control. Running the default action alone meant
+                // a component whose input is visually hidden behind its label,
+                // which is every switch and every styled checkbox, never saw a
+                // `click` at all.
                 local_name!("label") => {
                     if let Some(target_node_id) =
                         doc.label_bound_input_element(node_id).map(|n| n.id)
                     {
-                        // Apply default click event action for target node
-                        let target_node = doc.get_node_mut(target_node_id).unwrap();
-                        let syn_event = target_node.synthetic_click_event_data(event.mods);
-                        handle_click(doc, target_node_id, &syn_event, dispatch_event);
+                        let target_node = doc.get_node(target_node_id).unwrap();
+                        // A disabled control has no activation behaviour, so
+                        // the label has nothing to forward and the press ends
+                        // here. Dispatching anyway would run the control's own
+                        // `click` listeners, which is the one thing `disabled`
+                        // is there to prevent -- and the pre-click activation
+                        // steps refusing to toggle it would not stop them.
+                        let disabled = target_node
+                            .element_data()
+                            .is_some_and(|el| el.attr(local_name!("disabled")).is_some());
+                        if !disabled {
+                            let syn_event = target_node.synthetic_click_event(event.mods);
+                            dispatch_event(DomEvent::new(target_node_id, syn_event));
+                        }
                         break 'matched true;
                     }
                 }

packages/blitz-dom/src/lib.rs

@@ -85,7 +85,10 @@ pub use crate::node::{
 };
 pub use blitz_traits::node_id::NodeId;
 pub use config::{DocumentConfig, StyleThreading};
-pub use document::{AnimationPacing, BaseDocument, DocGuard, DocGuardMut, Document, PlainDocument};
+pub use document::{
+    AnimationPacing, BaseDocument, DocGuard, DocGuardMut, Document, PlainDocument,
+    PreClickActivation,
+};
 /// Per-resolve layout counters. Present only with `log-phase-times`, which is
 /// the same feature that pays for the counting.
 #[cfg(feature = "log-phase-times")]

packages/blitz-script/src/dom/element.rs

@@ -466,13 +466,56 @@ fn set_checked(this: &JsValue, args: &[JsValue], context: &mut Context) -> JsRes
     let ctx = dom_ctx(context)?;
     let node_id = this_node_id(this)?;
     let checked = args.first().map(JsValue::to_boolean).unwrap_or(false);
-    // blitz-dom's checked handling parses the value as a boolean
-    write_attr(
-        &ctx,
-        node_id,
-        "checked",
-        if checked { "true" } else { "false" },
-    );
+
+    /*
+     * Once the input has been constructed its checkedness lives in the
+     * element's special data, and that is what the renderer, the accessibility
+     * tree and `change` all read. The attribute is `defaultChecked`: the
+     * document as it was parsed, which the IDL property is not supposed to
+     * move.
+     *
+     * Writing only the attribute left a controlled component -- one that
+     * renders `checked` from its own state -- unable to drive its own input at
+     * all, because the write landed nowhere anything observes.
+     *
+     * The snapshot is what asks for a restyle. `:checked` is matched from this
+     * same state, and taking the attribute write away takes its invalidation
+     * with it.
+     */
+    let has_live_state = {
+        let mut doc = ctx.mutate_doc();
+        match doc
+            .get_node_mut(node_id)
+            .and_then(|node| node.data.downcast_element_mut())
+            .and_then(|element| element.checkbox_input_checked_mut())
+        {
+            Some(is_checked) => {
+                let changed = *is_checked != checked;
+                *is_checked = checked;
+                if changed {
+                    doc.snapshot_node(node_id);
+                }
+                true
+            }
+            None => false,
+        }
+    };
+
+    /*
+     * Before construction the attribute is the only carrier of the initial
+     * value, so a property write has to reach it or the value is lost.
+     *
+     * `checked` is an HTML boolean attribute: present means on, whatever the
+     * value reads. Writing `checked="false"` therefore *set* it, and every
+     * controlled checkbox, radio and switch came up already on.
+     */
+    if !has_live_state {
+        if checked {
+            write_attr(&ctx, node_id, "checked", "");
+        } else {
+            clear_attr(&ctx, node_id, "checked");
+        }
+    }
     Ok(JsValue::undefined())
 }