fix: checkbox activation, ordering and live state - #86
Conversation
A click's pre-click activation steps set a checkbox or radio's checkedness *before* the event is dispatched, so every listener sees the value the press produced. Blitz did it afterwards, as part of the default action, which handed `click` listeners the value from before the press. A control that reads `event.currentTarget.checked` -- the ordinary way to read a checkbox -- therefore acted on stale state and responded to every second press. The toggle moves into `run_pre_click_activation`, run by the event driver before the click is dispatched and undone by the canceled activation steps when a listener calls `preventDefault()`. `handle_click` keeps the post-click half: it reports the value with an `input` event, from which `change` is synthesised. A `<label>` had a second, related problem. Its activation behaviour is to fire a click at the control it labels, and Blitz ran that control's default action directly instead -- so the control toggled and fired `input`/`change`, but no `click` ever reached it. Every switch and styled checkbox puts a visually hidden input inside its label, so for the shape that needs it most the click was missing entirely. The label now dispatches the click, which then goes through activation and the listener chain like any other. Covered by five tests: the ordering, a cancelled click restoring the previous state, a label press, a radio's set, a disabled control, and a press driven by coordinates onto a switch whose input is clipped to a pixel.
Setting `check.checked = false` wrote the attribute `checked="false"` and nothing else. Two things went wrong with that. `checked` is an HTML boolean attribute: present means on, whatever the value reads. `create_checkbox_input` seeds the input from `has_attr`, so writing `"false"` turned the input *on*. Every controlled checkbox, radio and switch -- anything that renders `checked` from its own state, which is the ordinary pattern -- came up already selected, whatever the state said. And once the input is constructed its checkedness lives in the element's special data, which is what the renderer, the accessibility tree and `change` all read. The attribute is only the parsed document. So a property write landed somewhere nothing observes, and a controlled component could not drive its own input: pressing it moved the live state, the component wrote its state back, and the write did nothing. The setter now writes the live state and reflects the attribute the way HTML spells it -- present or absent, never `"false"`. Reflecting at all diverges from `defaultChecked`, which the attribute is meant to be; that is the lesser problem, since the attribute is the only carrier of the value before the input is constructed and nothing here resets a form.
A label's activation behaviour now dispatches a click at the control it labels, which is what a visually hidden input inside a label needs. It also has to stop where the control is disabled: the pre-click activation steps refuse to toggle a disabled input, but listeners run before that refusal is visible, so an application's own `click` handler ran on a control whose `disabled` is there to prevent exactly that. The disabled test only watched `change`, which the control never receives either way, so it passed throughout. It now watches `click`, `input` and `change`, and reports `heard click` without the guard. Reported in review of #86.
Selecting a radio clears every other in the set, so the canceled activation steps have to restore all of them, and reading them first walked the whole node arena twice for one press. The membership test stays `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", scoped to neither `type=radio` nor a form owner, so a radio can clear a same-named checkbox in another form. Wrong per HTML and longstanding; matching it keeps this change to the ordering it is about.
The property setter reflected into the content attribute, which is `defaultChecked`: the document as parsed, and not something the IDL property is supposed to move. It also sent every controlled toggle through an attribute snapshot and the style invalidation that follows. Now the live state is written directly and the node is snapshotted only when the value actually moved, which is what `:checked` needs and all it needs. The attribute is still written before the input is constructed, where it is the only carrier of the initial value. Reported in review of #86.
The publish workflow skips a version already on crates.io, so the fixes on this branch reach no consumer without this. Both downstream bumps pin `^0.4.3` precisely so they cannot resolve to 0.4.2, which is a newer engine that still has every one of these bugs.
|
Folded in. Three of these were real, one of them a regression this PR introduced. A label dispatched No 0.4.3 to publish (finding 1). Correct. The workspace said 0.4.2 and the workflow skips a version already on crates.io, so this would have merged and reached nobody while both downstream PRs pinned Attribute reflection (finding 10). Taken. The live state is written directly and the node is snapshotted only when the value actually moved, which is what Radio double scan (finding 6). The second arena walk was mine and is gone: the set is recorded while it is selected, in one pass. The predicate itself I have deliberately left alone and said so in the code. It is Not taken: the checkbox Re-verified: dom suite 66/66, |
The eight tests this replaces built a `ScriptDocument` in-process and read the result out of the engine that produced it. They agreed with the engine by construction: the document was never driven, nothing was ever pressed through an interface a driver has, and a page that merely parsed could satisfy most of what they asserted. These launch the harness as a separate process, open a control session over a socket, and click by CSS selector. Every assertion is something the running page reported: the fixtures record what their listeners saw into `#log`, so "the click listener saw the new checkedness" is read back as text the document is showing rather than as a field on a struct. Each fix on this branch is covered by at least one test that fails when the fix is reverted, checked by reverting them: pre-click activation removed 5 of 7 fail `checked` property reverted 2 fail disabled guard removed 2 fail The harness had two defects of its own, both found by running the suite five times rather than once. Descriptor paths used a wall-clock nonce, which is not unique across seven tests starting in the same microsecond, so two of them read each other's token. And polling with `yield_now` across seven driven processes starved the connections until responses came back truncated, which surfaced as a missing field several frames from the cause. Paths now carry a sequence number, polls are paced, and a refused connection is retried rather than unwrapped. Five consecutive runs, 7/7 each.
The eight tests this replaces built a `ScriptDocument` in-process and read the result out of the engine that produced it. They agreed with it by construction: the document was never driven, nothing was pressed through an interface a driver has, and a page that merely parsed satisfied most of what they asserted. This is ps-qa, the harness every other project here uses, on hand-written pages under `packages/blitz-script/tests/fixtures`. Each page records what its listeners saw into a heading, so every assertion is a name the renderer painted. The host is built from source against this checkout rather than installed. `qa-inspect-host` depends on the published engine, so an installed one judges the last release and no change here could fail it. Cargo takes the patch on the command line, so nothing in the host's repository is edited and there is no manifest to revert. Six groups, and each fix on this branch is covered by checks that fail when it is reverted: pre-click activation removed 5 of 6 groups fail disabled guard removed disabled-label fails `disabled-label` carries two checks on purpose. "The disabled control heard nothing" is satisfied just as well by a press that never arrived, so the page also presses an enabled control the same way and requires that it did hear. Only the pair means anything.
Installing ps-qa while building the host made this job depend on a release of the harness as well as on the checkout, so a change to either could not be used here until it had been published. They move together; take both from the same place. `QA_PS_QA` and `QA_HOST` still short-circuit either, for someone iterating on the harness and the fixtures at once.
Left behind when the invented Rust harness gave way to the ps-qa suite: the step still ran `--test activation`, which no longer exists, so the job died on `no test target named activation` before reaching the fixtures.
The host reaches the engine through `tauri-runtime-blitz`, which brings the Tauri stack with it, and on Linux that wants GTK and its dependencies at build time. The job installed fontconfig and nothing else, so the build died in `glib-sys` with "pkg-config exited with status code 1" -- which reads as a cargo problem rather than a missing system library, the same trap the fontconfig note above this describes.
The packages were not the problem. `tauri-runtime-blitz` does not compile on Linux at all: its `RawWindow` impl is missing `default_vbox` and `gtk_window`, so no amount of GTK development headers gets the host built there.
`tauri-runtime-blitz`, which the host reaches the engine through, does not compile on Linux: its `RawWindow` impl is missing `default_vbox` and `gtk_window` there. That is a portability limit of the crate, not a missing development package, so the GTK headers I added first could never have helped. Its own job on macOS, rather than riding along on the Linux one.
macOS runners are not used for CI here.
It asserted that the control ended up off, which a press that toggled nothing satisfies just as well. So it passed on an engine with no activation at all, and the A/B that was meant to prove these checks have teeth showed it plainly: with the pre-click activation steps disabled, every other fixture failed and this one passed. I ran that comparison, read the output, and did not notice. The page now records the checkedness inside the click listener, where the pre-click activation steps have already run and it has to be `true`, and again once the press is over, where the canceled activation steps have to have put it back. `Press saw true, left false` distinguishes an undo from an engine that never did anything; neither half does alone. Same shape as the pair in `disabled-label`, which exists for the same reason.
A label's activation behaviour now dispatches a click at the control it labels, which is what a visually hidden input inside a label needs. It also has to stop where the control is disabled: the pre-click activation steps refuse to toggle a disabled input, but listeners run before that refusal is visible, so an application's own `click` handler ran on a control whose `disabled` is there to prevent exactly that. The disabled test only watched `change`, which the control never receives either way, so it passed throughout. It now watches `click`, `input` and `change`, and reports `heard click` without the guard. Reported in review of #86.
The property setter reflected into the content attribute, which is `defaultChecked`: the document as parsed, and not something the IDL property is supposed to move. It also sent every controlled toggle through an attribute snapshot and the style invalidation that follows. Now the live state is written directly and the node is snapshotted only when the value actually moved, which is what `:checked` needs and all it needs. The attribute is still written before the input is constructed, where it is the only carrier of the initial value. Reported in review of #86.
Three bugs in the same path, each of which makes a checkbox, radio or switch
report something other than what it is.
The click was dispatched before the checkbox took its new value
HTML flips checkedness in the pre-click activation steps, before the event is
dispatched, so every listener sees the value the press produced. Blitz did it
afterwards in the default action, so a
clicklistener got the value frombefore the press:
Anything reading
event.currentTarget.checked, which is the ordinary way toread a checkbox, therefore acted on stale state and responded to every second
press. The toggle moves into
run_pre_click_activation, run by the event driverbefore dispatch and undone by the canceled activation steps when a listener
calls
preventDefault().handle_clickkeeps the post-click half.A
<label>never dispatched a click at the control it labelsIts activation behaviour is to fire a click at the control; Blitz ran the
control's default action directly instead. The control toggled and fired
input/change, but noclickever reached it. Every switch and styledcheckbox hides its input inside its label, so for the shape that needs it most
the click was missing entirely.
el.checked = falseturned the input onThe setter wrote the attribute
checked="false"and nothing else.checkedisan HTML boolean attribute and
create_checkbox_inputseeds fromhas_attr, sowriting
"false"set it: every controlled checkbox, radio and switch came upalready selected whatever its state said. And once the input is constructed its
checkedness lives in the element's special data, which is what the renderer, the
accessibility tree and
changeall read, so a property write landed wherenothing observes it and a controlled component could not drive its own input.
The setter now writes the live state and reflects the attribute the way HTML
spells it, present or absent, never
"false".Verification
Seven new tests in
packages/blitz-script/tests/dom.rs: the ordering, acancelled click restoring the previous state, a label press, a radio's set, a
disabled control, a press driven by coordinates onto a switch whose input is
clipped to a pixel, and the two
checkedproperty cases. The dom suite is 66/66and
cargo clippy --workspace -- -D warningsis clean.Checked end to end in a downstream application built against this branch. A
setting persisted as
falsecame back reportingselectedafter a restart, anddoes not with these commits; that application's full native suite is 310/310 on
the fixed engine, so the ordering and label changes do not disturb other
controls.
packages/blitz-wasm/tests/end_to_end.rsneeds a prebuilt guest module and failslocally without one, unrelated to this change.
For consumers moving to 0.4
blitz-shellon this line requireswinit ^0.31.0-beta.3. A consumer whoselockfile pins
0.31.0-beta.2fails to resolve rather than failing to compile,with
failed to select a version for winit, and needscargo update -p winit --precise 0.31.0-beta.3. Found while buildingAgencyZero against this branch.
Verified against this exact commit:
@pathscale/ui's component gate is 17/17across switch, checkbox, radio and connection-settings and 73/74 over the whole
library, the one failure being a Select paint check that fails identically on
the published engine. AgencyZero is 310/310 with the reported bug gone.
Since review
Four commits added. A label no longer forwards a press to a disabled control:
the pre-click activation steps refuse to toggle it, but listeners ran first, so an
application's
onClickfired on a control whosedisabledexists to stop exactlythat. It was a regression from this PR's own label change, and the disabled test
watched only
change— which never arrives either way — so it passed throughout.It now watches
click,inputandchange.The
checkedproperty setter no longer reflects into the content attribute, whichis
defaultChecked; it writes the live state and snapshots the node only when thevalue moved, which is what
:checkedneeds. The attribute is still written beforethe input is constructed, where it is the only carrier of the initial value.
A radio's set is recorded while it is selected rather than in a scan beforehand,
so activation walks the arena once.
And the workspace is now 0.4.3. It was 0.4.2, and the publish workflow skips a
version already on crates.io, so this would have merged and reached nobody while
both downstream PRs pin
^0.4.3.