fix(nanoviews): fire every on* prop, not just the bubbling ones - #204
Merged
Conversation
Handlers were delegated: one listener per event type on `document`, and the handler stashed on the element in a `__type` slot. Events that do not bubble never reach a document listener in the bubble phase, so `onFocus`, `onBlur`, `onMouseEnter`, `onScroll`, `onPlay`, the media events and `load`/`error` were typed, accepted and silently dead. So was every `*Capture` prop - the prop name was lowercased whole, and `"clickcapture"` is not an event. So was `onDoubleClick`, for the same reason: the DOM spells it `dblclick`. Handlers now go on their own elements. The dispatcher, the prototype slots and the mount marker that stopped its walk are gone, and `controls.ts` binds through the same call - a registration dies with its element, so a control binding needs no teardown and no effect node to carry one. `value$` and an `onInput` handler no longer fight over one slot: the browser holds any number of listeners per element and event. Two more things follow from being on the element rather than on `document`. `preventDefault()` in `onWheel`, `onTouchStart` and `onTouchMove` works - document listeners for those types are passive by default and the browser was ignoring it. And a third-party `stopPropagation()` below the document no longer silences an element's own handler. `onDoubleClick` becomes `onDblClick`, which is what the event is called; the prop has never fired, so nothing can depend on it. Event names are memoised per prop name - the browser atomises a freshly built string on every registration, and handing back the same string object is worth about a fifth of the attach path. `createElementPropertySetter` gains `@__NO_SIDE_EFFECTS__`. It was called at module root without the annotation, so a bundler had to keep it and everything it referenced: importing `value$` alone shipped the `checked$` and `selected$` implementations too.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
+ Coverage 85.29% 85.32% +0.03%
==========================================
Files 141 139 -2
Lines 3168 3142 -26
Branches 593 591 -2
==========================================
- Hits 2702 2681 -21
+ Misses 335 332 -3
+ Partials 131 129 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
onFocusdid not fire. Neither didonBlur,onMouseEnter,onScroll,onPlay, any media event,load,error, any*Captureprop, oronDoubleClick. All of them are in the type surface, all of them were accepted without complaint, and none of them ever called anything. Verified in Chrome 151 — of seven handlers put on one<input>, exactly one fired:{"delegated": {"click": 1}, "direct": {"focus": 1, "blur": 1, "mouseenter": 1, "scroll": 1, "play": 1}}The second row is plain
addEventListeneron the same element in the same test: the events happen, delegation just never saw them.Why
Handlers were delegated — one
document.addEventListener(type, dispatcher)per event type, the handler stashed on the element aselement.__click, and dispatch walkingcomposedPath()from the target up. A listener ondocumentin the bubble phase cannot see an event that does not bubble, andfocus,blur,mouseenter,mouseleave,scroll,load,errorand the media events do not.*Capturefailed differently:name.slice(2).toLowerCase()turnsonClickCaptureinto the event"clickcapture", which does not exist.onDoubleClickbecame"doubleclick"; the DOM spells itdblclick.What replaces it
The listener goes on its own element:
That is the whole event system.
internals/elements/events.tsis deleted, along withdefineProtoPropand the__mpmount marker whose only job was stopping the dispatcher's walk. Theuntrackedwrapper stays and is load-bearing:autoFocus$callsfocus()from inside an effect, and without it a handler reading a signal would subscribe that effect.controls.tsbinds through the same call. A registration dies with its element, so a control binding needs no teardown — and therefore no effect node to carry one, which is what the old code used a dependency-lesseffectfor. Thevalue$-versus-onInputcollision disappears with the single slot they used to share: the browser holds any number of listeners per element and event, and runs them in registration order.Two more fixes fall out of being on the element instead of on
document:preventDefault()insideonWheel,onTouchStartandonTouchMovenow works. Document-level listeners for those types are passive by default, so Chrome was discarding it — measureddefaultPrevented: falsebefore,trueafter.stopPropagation()below the document no longer silences an element's own handler. Probed in Chrome: the button's ownonClickruns, the ancestor's does not, which is what the DOM promises.onDoubleClick→onDblClickThe prop has never fired, so nothing can be depending on it, and the name it should have had is the one the event actually has. Solid spells it the same way. Keeping the old spelling would mean carrying a permanent special case in the hot path for a prop that was born broken.
Cost, measured
Attaching is not free the way writing a slot was. In Chrome, ns per handler, arms interleaved round-robin, 31 reps × 20 000 elements, order rotated per rep:
addEventListenerwith a freshly built nameThe gap between the last two is not the parsing — that is 20 ns. It is the browser atomising a string it has not seen as that object before; memoising the event name per prop name hands back the same object and returns about a fifth of the path.
Mapinstead of an object, cachingcapturealongside the name, splitting into two objects, derivingcapturefrom the name length: all measured, all within noise of each other on speed, all 6–17 B larger.js-framework-benchmark, 15 iterations per arm, back to back on an idle machine:
The 10k case is a genuine cost and it is not the attach path — the arithmetic there accounts for about 4 ms of the 18. The rest is the browser's own bookkeeping for twenty thousand live listeners. Set against it: dispatch is roughly four times cheaper than the deleted dispatcher (423–488 ns of handler-attributable work against 1808–1905), so one dispatched event pays back seven attaches. Ten thousand rows each carrying a handler is a benchmark shape; events happening is an application shape.
Size
createElementPropertySetteralso gains/* @__NO_SIDE_EFFECTS__ */. It was called at module root without it, so the bundler had to keep the call and everything it reached — importingvalue$alone was shipping thechecked$andselected$implementations. That one line is worth 285 B on its own, and it was true before this change too.All four pins come down.
Tests
Six added: a non-bubbling event fires, capture ordering runs outer-capture → target-capture → target-bubble,
onDblClickreachesdblclick,onGotPointerCaptureis not mistaken for a capture handler, a handler dispatched from inside an effect does not subscribe that effect, and a control binding coexists with a handler for the same event in either key order. 109 pass; the five fixes are also verified against the built dist in Chrome 151.