From b1d6290234f486d4e65a574e46c58678a3834a1f Mon Sep 17 00:00:00 2001 From: dangreen Date: Thu, 20 Aug 2026 17:09:52 +0400 Subject: [PATCH] fix(nanoviews): fire every `on*` prop, not just the bubbling ones 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. --- packages/nanoviews/.size-limit.json | 8 +- .../nanoviews/src/elements/controls.spec.ts | 42 +++++++ packages/nanoviews/src/elements/controls.ts | 21 ++-- .../src/internals/elements/attributes.ts | 25 +++-- .../src/internals/elements/element.spec.ts | 106 +++++++++++++++++- .../src/internals/elements/events.ts | 54 --------- .../nanoviews/src/internals/elements/index.ts | 2 - .../nanoviews/src/internals/elements/utils.ts | 4 - .../src/internals/types/dom/events.ts | 4 +- packages/nanoviews/src/mount.ts | 7 +- 10 files changed, 179 insertions(+), 94 deletions(-) delete mode 100644 packages/nanoviews/src/internals/elements/events.ts delete mode 100644 packages/nanoviews/src/internals/elements/utils.ts diff --git a/packages/nanoviews/.size-limit.json b/packages/nanoviews/.size-limit.json index 13bbffe1..2275901e 100644 --- a/packages/nanoviews/.size-limit.json +++ b/packages/nanoviews/.size-limit.json @@ -4,25 +4,25 @@ "gzip": true, "path": "dist/index.js", "import": "*", - "limit": "7.6 kB" + "limit": "7.45 kB" }, { "name": "All publics (Brotli)", "path": "dist/index.js", "import": "*", - "limit": "6.75 kB" + "limit": "6.6 kB" }, { "name": "Average usage (Gzip)", "gzip": true, "path": "dist/index.js", "import": "{ fragment, div, form, input, button, label, classList$, if_, for_, value$, $$children, effect }", - "limit": "4.25 kB" + "limit": "3.8 kB" }, { "name": "Average usage (Brotli)", "path": "dist/index.js", "import": "{ fragment, div, form, input, button, label, classList$, if_, for_, value$, $$children, effect }", - "limit": "3.95 kB" + "limit": "3.55 kB" } ] diff --git a/packages/nanoviews/src/elements/controls.spec.ts b/packages/nanoviews/src/elements/controls.spec.ts index 686acc08..991dc338 100644 --- a/packages/nanoviews/src/elements/controls.spec.ts +++ b/packages/nanoviews/src/elements/controls.spec.ts @@ -11,6 +11,10 @@ import { } from '@nanoviews/testing-library' import { userEvent } from '@testing-library/user-event' import { signal } from 'kida' +import { + input, + fragment +} from '../index.js' import * as Stories from './controls.stories.js' import type { Indeterminate } from './controls.js' @@ -74,6 +78,44 @@ describe('nanoviews', () => { expect(textarea.value).toBe('user input') }) + + it('should run alongside a handler for the same event, in either order', () => { + const seen: string[] = [] + const $bound = signal('') + const $reversed = signal('') + const { container } = render(() => fragment( + input({ + value$: $bound, + onInput: () => seen.push(`bound:${$bound()}`) + }), + input({ + onInput: () => seen.push(`reversed:${$reversed()}`), + value$: $reversed + }) + )) + const [bound, reversed] = Array.from(container.querySelectorAll('input')) + + fireEvent.input(bound, { + target: { + value: 'a' + } + }) + fireEvent.input(reversed, { + target: { + value: 'b' + } + }) + + // both listeners run whatever the key order was; only what the + // handler sees in the signal follows it, and the DOM value is + // current either way + expect($bound()).toBe('a') + expect($reversed()).toBe('b') + expect(seen).toEqual([ + 'bound:a', + 'reversed:' + ]) + }) }) describe('selected$', () => { diff --git a/packages/nanoviews/src/elements/controls.ts b/packages/nanoviews/src/elements/controls.ts index 4da52887..b178e40b 100644 --- a/packages/nanoviews/src/elements/controls.ts +++ b/packages/nanoviews/src/elements/controls.ts @@ -38,6 +38,7 @@ type ComboboxElement = HTMLSelectElement type FileElement = HTMLInputElement +/* @__NO_SIDE_EFFECTS__ */ function createElementPropertySetter( eventName: string, getValue: (control: E) => V, @@ -51,13 +52,11 @@ function createElementPropertySetter( setValue(control, $value()) }) - effect(() => { - const eventListener = () => $value(getValue(control)) - - control.addEventListener(eventName, eventListener) - - return () => control.removeEventListener(eventName, eventListener) - }) + // The registration dies with the element, so the binding needs no + // teardown - and no effect node to carry one. It reads the DOM and writes + // a signal, and a write subscribes nobody, so it needs no tracking barrier + // either + control.addEventListener(eventName, () => $value(getValue(control))) } } @@ -172,13 +171,7 @@ function filesEffectAttribute( control: FileElement, $value: Files ) { - effect(() => { - const eventListener = () => $value(Array.from(control.files!)) - - control.addEventListener(onChangeEvent, eventListener) - - return () => control.removeEventListener(onChangeEvent, eventListener) - }) + control.addEventListener(onChangeEvent, () => $value(Array.from(control.files!))) } /** diff --git a/packages/nanoviews/src/internals/elements/attributes.ts b/packages/nanoviews/src/internals/elements/attributes.ts index b4214fca..3772cd29 100644 --- a/packages/nanoviews/src/internals/elements/attributes.ts +++ b/packages/nanoviews/src/internals/elements/attributes.ts @@ -1,7 +1,8 @@ import { isAccessor, isFunction, - effect + effect, + untracked } from 'kida' import type { PrimitiveAttributeValue, @@ -9,7 +10,6 @@ import type { } from '../types/index.js' import { isEmpty } from '../utils.js' import { effectAttributes } from './effectAttribute.js' -import { delegateEvent } from './events.js' type AttributeValue = PrimitiveAttributeValue | TargetEventHandler @@ -37,13 +37,24 @@ function isEventHandler(key: string, value: unknown): value is TargetEventHandle return key.startsWith('on') && isFunction(value) } -function setEventListener(element: Element, name: string, value: TargetEventHandler) { - const eventName = name.slice(2).toLowerCase() +// Building the event name allocates a string the browser has to atomize on +// every `addEventListener`; keyed by the prop name, the same string object is +// handed over every time +const eventNames: Record = {} - delegateEvent(eventName) +function setEventListener(element: Element, name: string, value: TargetEventHandler) { + // `onGotPointerCapture` and `onLostPointerCapture` end with `Capture` + // themselves, and are ordinary bubbling events + const capture = name.endsWith('Capture') && !name.endsWith('PointerCapture') - // @ts-expect-error Inject event listener into element - element[`__${eventName}`] = value + element.addEventListener( + eventNames[name] ??= name.slice(2, capture ? -7 : undefined).toLowerCase(), + // A handler is user code: it must not subscribe whatever effect happens to + // be running when the event is dispatched synchronously from inside one - + // `autoFocus$` calls `focus()` from an effect, and that is not exotic + event => untracked(() => (value as EventListener).call(element, event)), + capture + ) } /** diff --git a/packages/nanoviews/src/internals/elements/element.spec.ts b/packages/nanoviews/src/internals/elements/element.spec.ts index 660886f5..bae5b68c 100644 --- a/packages/nanoviews/src/internals/elements/element.spec.ts +++ b/packages/nanoviews/src/internals/elements/element.spec.ts @@ -10,7 +10,10 @@ import { screen, fireEvent } from '@nanoviews/testing-library' -import { signal } from 'kida' +import { + signal, + effect +} from 'kida' import * as Stories from './element.stories.js' import { createElement } from './element.js' @@ -118,6 +121,107 @@ describe('nanoviews', () => { expect(onClick).toHaveBeenCalled() }) + it('should handle events that do not bubble', () => { + const onFocus = vi.fn() + const onPlay = vi.fn() + const { container } = render(() => createElement('div')( + createElement('input', { + onFocus + })(), + createElement('video', { + onPlay + })() + )) + + container.querySelector('input')!.dispatchEvent(new Event('focus')) + container.querySelector('video')!.dispatchEvent(new Event('play')) + + expect(onFocus).toHaveBeenCalledTimes(1) + expect(onPlay).toHaveBeenCalledTimes(1) + }) + + it('should handle capture events ahead of bubbling ones', () => { + const calls: string[] = [] + const { container } = render(() => createElement('div', { + onClickCapture: () => calls.push('outer capture'), + onClick: () => calls.push('outer') + })( + createElement('button', { + onClickCapture: () => calls.push('button capture'), + onClick: () => calls.push('button') + })() + )) + + fireEvent.click(container.querySelector('button')!) + + expect(calls).toEqual([ + 'outer capture', + 'button capture', + 'button', + 'outer' + ]) + }) + + it('should spell the double click prop the way the DOM spells the event', () => { + const onDblClick = vi.fn() + const { container } = render(() => createElement('button', { + onDblClick + })()) + + fireEvent.dblClick(container.querySelector('button')!) + + expect(onDblClick).toHaveBeenCalledTimes(1) + }) + + it('should not read a pointer capture event as a capture handler', () => { + const calls: string[] = [] + const { container } = render(() => createElement('div', { + onGotPointerCaptureCapture: () => calls.push('capture') + })( + createElement('button', { + onGotPointerCapture: () => calls.push('bubble') + })() + )) + + container.querySelector('button')!.dispatchEvent( + new Event('gotpointercapture', { + bubbles: true + }) + ) + + expect(calls).toEqual([ + 'capture', + 'bubble' + ]) + }) + + it('should not subscribe the effect an event was dispatched from', () => { + const $tick = signal(0) + const reads: number[] = [] + let runs = 0 + const { container } = render(() => createElement('button', { + onClick: () => reads.push($tick()) + })()) + const button = container.querySelector('button')! + const stop = effect(() => { + runs++ + $tick() + button.click() + }) + + expect(runs).toBe(1) + expect(reads).toEqual([0]) + + $tick(1) + + // the handler read `$tick`, but it read it for itself: the effect + // that dispatched the click is woken by its own dependency only + expect(runs).toBe(2) + expect(reads).toEqual([0, 1]) + + stop() + }) + it('should render children', () => { const { container } = render(Children()) diff --git a/packages/nanoviews/src/internals/elements/events.ts b/packages/nanoviews/src/internals/elements/events.ts deleted file mode 100644 index 9389a11d..00000000 --- a/packages/nanoviews/src/internals/elements/events.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { untracked } from 'kida' -import { defineProtoProp } from './utils.js' - -type Target = EventTarget & { - disabled?: boolean - __mp?: ParentNode -} - -function eventHandler(event: Event) { - const key = `__${event.type}` - let node: Target | undefined - - Object.defineProperty(event, 'currentTarget', { - configurable: true, - get: () => node - }) - - const path = event.composedPath() - - // A handler is user code: it must not subscribe whatever effect happens to - // be running when the event is dispatched synchronously from inside one - untracked(() => { - for (let i = 0, len = path.length - 4, handler; i < len; i++) { - node = path[i] - - // @ts-expect-error Get monkey defined property - if ((handler = node[key] as EventListener | undefined) !== undefined && !node.disabled) { - handler.call(node, event) - - if (event.cancelBubble) { - break - } - } - - if (node.__mp) { - break - } - } - }) - - node = undefined -} - -let delegatedEvents: Set | undefined - -export function delegateEvent(eventName: string) { - delegatedEvents ??= new Set() - - if (!delegatedEvents.has(eventName)) { - defineProtoProp(`__${eventName}`) - delegatedEvents.add(eventName) - document.addEventListener(eventName, eventHandler) - } -} diff --git a/packages/nanoviews/src/internals/elements/index.ts b/packages/nanoviews/src/internals/elements/index.ts index 2b222908..36a787d7 100644 --- a/packages/nanoviews/src/internals/elements/index.ts +++ b/packages/nanoviews/src/internals/elements/index.ts @@ -1,8 +1,6 @@ export * from './child.js' export * from './text.js' export * from './attributes.js' -export * from './events.js' export * from './effectAttribute.js' export * from './element.js' export * from './fragment.js' -export * from './utils.js' diff --git a/packages/nanoviews/src/internals/elements/utils.ts b/packages/nanoviews/src/internals/elements/utils.ts deleted file mode 100644 index 61f3e785..00000000 --- a/packages/nanoviews/src/internals/elements/utils.ts +++ /dev/null @@ -1,4 +0,0 @@ -export function defineProtoProp(name: string, value?: unknown) { - // @ts-expect-error Define property on prototype - Element.prototype[name] = value -} diff --git a/packages/nanoviews/src/internals/types/dom/events.ts b/packages/nanoviews/src/internals/types/dom/events.ts index f5ccc59e..01f13a02 100644 --- a/packages/nanoviews/src/internals/types/dom/events.ts +++ b/packages/nanoviews/src/internals/types/dom/events.ts @@ -190,8 +190,8 @@ export interface DOMAttributes { onClickCapture?: MouseEventHandler | undefined onContextMenu?: MouseEventHandler | undefined onContextMenuCapture?: MouseEventHandler | undefined - onDoubleClick?: MouseEventHandler | undefined - onDoubleClickCapture?: MouseEventHandler | undefined + onDblClick?: MouseEventHandler | undefined + onDblClickCapture?: MouseEventHandler | undefined onDrag?: DragEventHandler | undefined onDragCapture?: DragEventHandler | undefined onDragEnd?: DragEventHandler | undefined diff --git a/packages/nanoviews/src/mount.ts b/packages/nanoviews/src/mount.ts index b67f7e43..b68f1cfc 100644 --- a/packages/nanoviews/src/mount.ts +++ b/packages/nanoviews/src/mount.ts @@ -8,8 +8,7 @@ import { import { type Child, type MaybeDestroy, - mountChild, - defineProtoProp + mountChild } from './internals/index.js' /** @@ -19,10 +18,6 @@ import { * @returns A function to unmount the app */ export function mount(app: () => Child, target: ParentNode) { - defineProtoProp('__mp', false) - // @ts-expect-error Mark as mount point - target.__mp = true - let unmount: MaybeDestroy let scope!: DeferredScope