fix(js): implement HTMLInputElement.indeterminate - #741
Open
ntdatt812 wants to merge 1 commit into
Open
Conversation
The property was never defined anywhere in the engine, so `'indeterminate' in checkbox` was false and scripts could neither set nor read the flag. Add it as an IDL-only property backed by a node-keyed store, the same way `checked` is stored, since element wrappers are rebuilt on every lookup. Checkbox activation now clears it too. HTML's legacy-pre-activation behavior toggles checkedness and sets indeterminateness to false, and the canceled path restores both. Two copies of that logic needed the change: `Element#click()` in bootstrap.js, and the pre-click activation snippet in obscura-cdp that drives real dispatched mouse input. Fixes h4ckf0r0day#732.
Author
|
Obstacle course, for the CONTRIBUTING pre-PR item I could not tick from a unit test. Companion repo at the ref your CI pins (
No delta. The single failure is the same stage on both, and it fails on untouched So it is a property of this host rather than of the change, and I have not chased it further since it is outside the diff. It is deterministic rather than flaky here: re-running just that stage with Everything else in this PR is unchanged. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #732.
What changed
indeterminatewas not implemented anywhere in the engine.grep -r indeterminate crates/returned nothing: no prototype accessor to back it,'indeterminate' in checkboxwasfalse, and a script could neither set nor read the flag.crates/obscura-js/js/bootstrap.jsget/set indeterminateon the element prototype, backed by a node-keyed_formIndeterminatestore. Node-keyed rather than per-instance for the same reasoncheckedis: element wrappers are rebuilt on every lookup, so a field on the instance would not survivegetElementByIdhanding back a new wrapper. Being on the prototype is also what makes'indeterminate' in eltrue for a freshly created element, which is the check in the issue's repro.Element#click()clearsindeterminatein the pre-click step and restores the old value when the click is canceled, alongside the existingcheckedhandling.crates/obscura-cdp/src/domains/input.rsThe pre-click activation logic is duplicated in the CDP mouse snippet, which is what real dispatched input goes through. Without the same three lines there,
Input.dispatchMouseEventon a checkbox would leave the flag stuck even once the property exists. This is the duplication the comment inbootstrap.jsalready points at.Radio inputs are unaffected: HTML's pre-activation for radio sets checkedness to true and says nothing about indeterminateness, so the restore path there writes back the value it read.
One correction to the issue, which changes what the fix does
The issue also says an indeterminate checkbox clicked once should report
checked=false, describing the state as "not flipped on the first click". That is not what HTML specifies. Legacy-pre-activation behavior is:Clearing
indeterminateis an extra step, not a replacement for the toggle. An unchecked indeterminate checkbox reportschecked=true, indeterminate=falseafter one click, which is what Chrome and Firefox do, and it is what this PR implements.I have gone with the spec rather than the linked reference implementation because the spec is the thing the rest of the file follows. If you would rather match the reference, it is two lines in each of the two places, and I will send it as a follow-up rather than hold this one.
Validation
New test
test_checkbox_indeterminate_is_idl_only_and_cleared_by_activationincrates/obscura-js/src/runtime.rs, covering the four things that were broken or at risk:'indeterminate' in elis true on a fresh element and readsfalse; setting it round-trips; clicking an indeterminate checkbox yieldschecked=true, indeterminate=false; a canceled click (preventDefault) restores both.I ran the four-configuration sequence from CONTRIBUTING rather than only the crate I touched, since this changes shared DOM and CDP code. All six steps in a
rust:latestcontainer, so the result matches CI rather than my Windows host:cargo build --release -p obscura-cli --bins --features rendercargo nextest run --release --features render --no-fail-fastcargo build --release -p obscura-cli --bins --no-default-featurescargo nextest run --release -p obscura --no-fail-fastcargo nextest run --release --workspace --exclude obscura --exclude obscura-render --no-default-features --no-fail-fastcargo check --release -p obscura-render --no-default-featuresThe focused test, for the log:
I have not run
cargo fmtover the tree, and the diff is the three files the fix needs.Rendering
Not applicable. No layout, paint, screenshot, screencast or PDF code is touched, and the render-feature suite above is unchanged at 1507 passing.
Performance
The two accessors are only reached when a page reads or writes
.indeterminate, which nothing did before this PR. The cost added to the common path is one object write per activation of a checkable input, next to the_formCheckedwrite already there. Nothing is added to layout, paint, or the network path.Checklist