Repository navigation
Run JSX listeners deepest-first by binding them to their own node - #2
SantosNr22 wants to merge 1 commit into
Conversation
bindNodeListener delegated every bubbling JSX listener to the document body, gated by containsNode. The engine runs a node's listeners in registration order, so the body ran nested handlers in the order they were bound, not deepest first. A list row rebuilt by a re-render binds after the ancestors that were bound once at mount, so with an onPointerDown on both the row and an ancestor the order flipped from row-then-ancestor to ancestor-then-row after the first re-render, and stopPropagation could not hold the ancestor back. Register the listener on the node itself, as scroll already is. The engine's Tree::dispatchEvent walks from the hit node up its parents, so descendants run first, stopPropagation and non-bubbling events work, and the listeners are released with the node (releaseRareData). Runtime-header change only. The emitted-set gate names the same moved programs with and without it (its baselines do not reproduce on the machine this was written on, so they are not re-taken here). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the pull request. Before it can be merged, please read the GeaStack Contributor License Agreement and sign it by posting a comment here with exactly: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesNode listeners
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Listeners now register on their target nodes. Available evidence does not establish a regression, though the external engine’s propagation and teardown behavior remains unverified and should be confirmed in supported native testing. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.3)src/targets/cpp/runtime/gea_runtime.hast-grep timed out on this file Comment |
|
I have read the CLA Document and I hereby sign the CLA |
Problem
On the native (macOS) target, when a keyed list row and one of its ancestors both have
onPointerDown(or any other bubbling listener), the order in which they run flips after thelist has re-rendered once:
IA)AI)Downstream effect (Daymark week grid): a block's
onPointerDownguards onmode === "none"and thegrid's
onPointerDownstarts a "create" gesture. After the first gesture the grid ran first, setmode = "create", and the block's handler returned at its guard. It looked like "the block's handlernever fires"; it fires, but second.
stopPropagation()has no effect between handlers on differentnodes for the same reason.
The web target is not affected (see "Web target" below).
Cause
bindNodeListenerregistered every bubbling listener on the document body, gated byTree::containsNode(target, event.targetId).Tree::dispatchEventruns the listeners of one node inregistration order, so the body ran nested handlers in the order they were bound, not deepest first.
At mount, a list's rows are built (
reactiveListApplycalls the row thunk once) before the listcontainer's own
onPointerDownis bound, so the row is bound first and the order happens to be right.On every later re-render the rows are rebuilt and bound after the ancestors, so they run after them.
The comments above
addNodeListenersay the engine "does not walk ancestors". That is not true ofany published
@geastack/engine(0.1.5, 0.1.6, 0.1.7 all start atevent.targetIdand walkparent, honourevent.bubblesandpropagationStopped), so body delegation is no longer neededfor bubbling.
Fix
Register the listener on the node itself (what the
scrollbranch already does), for every event thatis not document-level.
Tree::dispatchEventthen does the bubbling: descendants first, thenancestors;
stopPropagationstops the walk; non-bubbling events stay on their target. The engine dropsa node's listeners when the node is released (
releaseRareData, also when its id is reused bycreateNode), so a rebuilt row never leaves a stale generation of its handler behind, which is what theold body-delegation comment was protecting against.
The
scrollspecial case and therecordNodeSubscriptionrelease for body listeners becomeunnecessary and are removed; comments updated to match.
Reproduction
The reproduction app's source is inlined at the bottom of this description. A ReactiveComponent with an
ancestor
onPointerDown, keyed rows with their ownonPointerDown, and a release handler thatreassigns
this.items. The status line printsorder.Build and run:
npx gea build --target macos, thenopen dist/macos/listrepro/Listrepro.app, and clickItem A three times with a real mouse (CGEvents; synthetic NSEvents skip the gesture recognizer).
orderafter 3 pressesIAAIAI(press 1 correct, presses 2 and 3 reversed)IAIAIA(every press row-then-ancestor),itemDowns=3 ancestorDowns=3itemDowns=3after 3 presses on the patched build also shows a rebuilt row runs its handler once, notonce per generation.
Second check, the actual Daymark week-grid spike (blocks with a nested resize handle, drag to move,
drag empty grid to create), workaround removed, real-mouse drags, same pinned install:
Versions
@geastack/apple0.2.11 (re-checked on 0.2.12, below),core0.1.28,cli0.1.86,targets0.1.83,engine0.1.6,compiler1.0.18 (exactnpm cifrom the spike's lockfile), macOS 26 (Darwin 25.6), Apple Silicon.The fix was checked two ways: by applying the same edit to the 1.0.18 header in a pristine
npm ciinstall, and by building the repro against this branch's
gea_runtime.h.Re-checked against newer releases
@geastack/apple0.2.12 (changes macOS hit-testing: text labels now participate in hit tests, and theroot press bridge hit-tests in the root's own coordinate space) does not change the ordering bug.
Same repro, same real-mouse protocol, everything else at the pins above:
orderafter 3 pressesIAAIAI(bug persists)gea_runtime.hIAIAIA@geastack/compiler1.0.19 andorigin/main(no commits beyond the base of this branch) stilldelegate listeners to the body, so the patch applies unchanged. The week-grid drag run was not repeated
on 0.2.12. The press target can now be a text node inside a block; the fix relies on the engine walking
parents, so that case bubbles correctly, but I only exercised it through the repro's
<span>rows.Web target
Not affected, but for a different reason worth knowing: the web runtime installs one document
listener per event type and calls only the nearest node that has a handler, then returns. So on web
the row handler runs and the ancestor's does not run at all (
order=III,ancestorDowns=0; theancestor fires only for a press that lands on it directly). After this change the native target
bubbles (row, then ancestor), which is what
event.bubbles/stopPropagationdescribe but is not whatweb does. I did not change web; flagging it because app code that relies on either behavior will
differ between targets.
How it was verified, and what was not
Ran:
gea_runtime.hbuilds and behaves the same in the repro app.node scripts/check-runtime-header.mjs(both wchar widths) andnpx tsc -p tsconfig.json --noEmitpass on the branch.
Not verified / limits:
compile-onlyand pin emitted C++, and this change is in the runtime header, so a fixture cannotfail on this bug.
check-runtime-header.mjscompiles the header withoutGEA_HOST_DECLARED, so it doesnot compile the changed code either (the app builds above do). A test that dispatches
Tree::dispatchEventover a row rebuilt after its ancestor needs the engine, e.g. besidepackages/core/test/test_gea_engine_rare_data_main.cppingeastack/core.npm run gatefails on this machine with 166 programs "moved" on unmodified HEAD too; the set isidentical with and without this commit, so I cannot show byte-identical emitted output from the
gate, only that it does not change what the gate reports. Baselines were not re-taken.
runtime.cpp->Tree::dispatchEventpath, but I did not build them.keydown/input/clicklisteners take the same per-node path; I exercised pointer events only.npm test,scripts/run-runtime-tests.mjs.npm installof the spike resolvesengine0.1.7 and fails to build(
elements/ui/virtual_list.cpp:itemCounton aunique_ptr), soelements0.1.2 andengine0.1.7do not match. The lockfile pins avoid it.
Repro source (
pr-repro/src/Repro.tsx)Summary by CodeRabbit