From 36829c62bdc7417c64c65e3eceb7c696addf3f93 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 3 Aug 2026 11:45:02 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20close=20#222=20=E2=80=94=20track=20t?= =?UTF-8?q?he=20Elements=20Register=20and=20RadioGroup=20create?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RequestWriter.Register and the lazy radio/label creation behind RadioGroup called Request.NewElement directly, so they were the only widget helpers a ui.Template did not track. A template that updates registered a fresh set on every execution and unregistered none, leaving the browser's removal acknowledgement as the only cleanup — and that can name only ids which reached the DOM. Growth was unbounded for every shape where one does not: a $.Register whose returned Jid is discarded or printed as text, a RadioElement.Label rendered without its Radio (which leaves the radio Element created but never rendered), and an execution that fails before its markup is delivered. Stale Elements also keep answering tag lookups, so Dirty(updater) fans out to ids absent from the DOM, where the client throws. Both helpers now report their Elements to the writer's owner, so the Template that rendered them reclaims them like any other nested UI. Reporting happens at creation rather than after a successful render, and the hook is renamed elementCreated to say so. That is what covers the unrendered radio: the Element exists because its Jid supplies the group's name= and the label's for=, and it may never render. The cost is that an owner's set can hold an Element whose render failed and which NewUI already unregistered, which is free — Request.DeleteElements skips elements it finds unregistered and every rollback deletes the whole set at once. The hook loses its error return. Its sole implementation cannot fail, and neither new call site could propagate one: Register returns a jid.Jid and Radio/Label return template.HTML, so both would have had to swallow it or route it through MustLog, which panics when no logger is configured. Neither helper routes through NewUI, and the reason differs per helper, so each carries its own comment. NewUI renders, and Element.JawsRender appends a debug comment when Jaws.Debug is set; Register's documented usage puts the returned Jid inside an attribute, where that comment would corrupt the markup. Radio and Label instead return their HTML for the template to place, and need the Element before that render for the name= and for= attributes. Ownership follows the RequestWriter that RadioGroup was called on, which is the template whose body called it rather than the wrapper the markup lands in. Those differ only when RadioElement values cross a template boundary in the dot; RadioGroup's doc states the condition, and a test pins it by updating the nested wrapper alone and asserting the group survives, since an outer update cannot distinguish the two attributions. Docs updated where #221 recorded the exception: Template, Register, RadioGroup, RadioElement, the lib/ui README and the tracked skill. The characterization test that asserted the growth becomes TestTemplate_UpdateTracksRegisterAndRadioGroup, now covering the discarded-Jid and label-only shapes, alongside new tests for execution failing after either helper ran and for the attribution boundary. --- .agents/skills/jaws/SKILL.md | 17 ++- lib/ui/README.md | 36 +++--- lib/ui/radiogroup.go | 32 +++-- lib/ui/register.go | 17 +-- lib/ui/requestwriter.go | 30 +++-- lib/ui/template.go | 33 +++-- lib/ui/template_owned_test.go | 221 +++++++++++++++++++++++++++++----- 7 files changed, 281 insertions(+), 105 deletions(-) diff --git a/.agents/skills/jaws/SKILL.md b/.agents/skills/jaws/SKILL.md index 1403ce09..06f680ed 100644 --- a/.agents/skills/jaws/SKILL.md +++ b/.agents/skills/jaws/SKILL.md @@ -158,16 +158,13 @@ For clickable content rendering: - `ui.Template.JawsUpdate` re-renders the template data into the generated wrapper. - `ui.NewTemplate` returns a `*ui.Template`, which tracks the Elements its execution creates and therefore backs one live Element; construct a fresh one per render - (`$.Template` already does). A successful update unregisters the Elements the - previous execution created through `rw.NewUI` — the path every `RequestWriter` - widget helper except `$.Register` and `$.RadioGroup` takes — since `SetInner` - replaces the DOM holding them. -- Those two helpers create Elements with `Request.NewElement`, so no template owns - them and their cleanup falls to the browser, which reports the ids it removed from - the DOM as the wrapper's new content is applied. An Element whose id never reaches - the DOM stays registered until the request ends (a discarded `$.Register` Jid, or an - execution that failed before delivering its markup), so prefer the `rw.NewUI`-backed - helpers inside a template that updates. + (`$.Template` already does). A successful update unregisters every Element the + previous execution created through the writer it was given — the widget helpers, + `$.Register`, `$.RadioGroup` and nested `$.Template` alike — since `SetInner` replaces + the DOM holding them. Ownership is recorded at creation, so an Element that never + rendered is reclaimed as well. +- Call `$.RadioGroup` from the template that renders the group: its Elements belong to + the template whose body called it, not to the wrapper their markup lands in. - HTML getter paths must not mutate domain state, but they may call element update methods (`SetClass`, `RemoveClass`, `SetAttr`, `RemoveAttr`, etc.) on the passed-in `*Element` to co-ordinate wrapper class/attribute changes with the inner-HTML refresh. No custom `JawsUpdate` is needed for that case — the queued wrapper updates flush alongside the `SetInner` from `HTMLInner.JawsUpdate`. - Use a custom `JawsUpdate` only when the widget's update logic diverges from "render the getter again" — e.g. to compare against a stored last-value and skip the update (as the input widgets do). - `Element.SetAttr/RemoveAttr/SetClass/RemoveClass/SetInner/SetValue/Append/Order/Remove/Replace` are update-time operations; call them only from render/update processing. diff --git a/lib/ui/README.md b/lib/ui/README.md index 90ae9a3d..f080c383 100644 --- a/lib/ui/README.md +++ b/lib/ui/README.md @@ -29,26 +29,22 @@ already happened; it does not roll back partial output, queued messages, or application side effects. The tracked elements the failed execution registered are unregistered, since nothing will update them. -A template owns the elements created through `rw.NewUI(...)`, the path taken by -every `RequestWriter` widget helper except the two below — `{{$.Span ...}}`, -`{{$.Button ...}}`, a nested `{{$.Template ...}}`, and so on. A successful update -unregisters the ones the previous render left behind, along with the DOM that -`SetInner` replaces. On updates that `SetInner` is queued only after a complete -successful render, so a failed update leaves the browser DOM unchanged — and with -it the previous render's elements — while earlier server-side side effects from -that attempted render may remain. Treat template execution errors as application -bugs: validate data before rendering and keep template actions infallible once they -start emitting output or nested UI. - -`{{$.Register ...}}` and `{{$.RadioGroup ...}}` are the exceptions: they create -elements through `Request.NewElement` directly rather than `rw.NewUI`, so the -template neither owns nor unregisters them. Their cleanup falls to the browser, -which reports the JaWS ids it removed from the DOM as the wrapper's new content is -applied, and the request unregisters those elements. An element whose id never -reaches the DOM has nothing to report it and stays registered until the request -ends — one from an execution that failed before its markup was delivered, or a -`$.Register` whose returned Jid the template discards. Prefer the `rw.NewUI`-backed -helpers inside a template that updates. +A template owns every element created through the `RequestWriter` it is given — +`{{$.Span ...}}`, `{{$.Button ...}}`, `{{$.Register ...}}`, `{{$.RadioGroup ...}}`, +a nested `{{$.Template ...}}`, and so on. A successful update unregisters the ones +the previous render left behind, along with the DOM that `SetInner` replaces. +Ownership is recorded when an element is created rather than after it renders, so +an element that never reached the browser is reclaimed too. On updates that +`SetInner` is queued only after a complete successful render, so a failed update +leaves the browser DOM unchanged — and with it the previous render's elements — +while earlier server-side side effects from that attempted render may remain. Treat +template execution errors as application bugs: validate data before rendering and +keep template actions infallible once they start emitting output or nested UI. + +`$.RadioGroup` has one attribution condition: its radio and label elements belong to +the template whose body called it, not to the wrapper their markup lands in. Call it +from the template that renders the group; see `RequestWriter.RadioGroup` for what +happens when the two differ. You can also use explicit constructors through: diff --git a/lib/ui/radiogroup.go b/lib/ui/radiogroup.go index b2ed0e9e..4e2c2ccd 100644 --- a/lib/ui/radiogroup.go +++ b/lib/ui/radiogroup.go @@ -15,8 +15,10 @@ import ( // renders register no elements on the [jaws.Request]. Call each of Radio and // Label at most once. Render Label only when Radio is also rendered: Label emits a // for="..." referencing the radio's id, so a Label without its Radio points at an -// input that is absent from the document (and leaves an unrendered radio Element -// registered on the Request for the request's lifetime). +// input that is absent from the document. The radio Element it created is still +// unregistered by the [Template] that owns it (see [RequestWriter.RadioGroup]) when +// that template next replaces its content, or stays registered until the +// [jaws.Request] ends if the group was rendered outside a template. type RadioElement struct { st *radioState } @@ -44,7 +46,14 @@ type radioGroupState struct { // ordered before the label's regardless of which is rendered first. func (st *radioState) radioElem() *jaws.Element { if st.radio == nil { + // Create and report the Element rather than going through + // RequestWriter.NewUI: Radio and Label return their HTML for the template to + // place instead of writing it to the writer, and the Element has to exist + // before that render because its Jid supplies the group's name= and the + // label's for=. Reporting it here, at creation, also means a radio that is + // never rendered (a Label without its Radio) is still owned and reclaimed. st.radio = st.rw.Request.NewElement(NewRadio(st.nb)) + st.rw.trackElement(st.radio) if st.group.nameAttr == "" { st.group.nameAttr = `name="` + st.radio.Jid().String() + `"` } @@ -81,7 +90,9 @@ func (re RadioElement) Radio(params ...any) template.HTML { func (re RadioElement) Label(params ...any) template.HTML { radio := re.st.radioElem() if re.st.label == nil { + // Created and reported like the radio Element; see radioElem. re.st.label = re.st.rw.Request.NewElement(NewLabel(re.st.nb)) + re.st.rw.trackElement(re.st.label) } var sb strings.Builder forAttr := string(radio.Jid().AppendQuote([]byte("for="))) @@ -97,13 +108,16 @@ func (re RadioElement) Label(params ...any) template.HTML { // rendered radio in the group shares a name derived from the first created // radio Element's request-scoped [jaws.Jid]. // -// The radio and label Elements are created through [jaws.Request.NewElement] rather -// than [RequestWriter.NewUI], so a surrounding [Template] neither owns nor unregisters -// them when the template re-renders. Cleanup falls to the browser, which reports the -// JaWS ids it removed from the DOM as the surrounding wrapper's new content is applied; -// a rendered radio or label carries its own id, so it is reported. One that never -// reached the DOM stays registered until the [jaws.Request] ends, including the -// unrendered radio Element a [RadioElement.Label] without its Radio leaves behind. +// The radio and label Elements belong to the [Template] whose body called RadioGroup, +// which unregisters them when it next replaces its content. Ownership follows that call +// site rather than the wrapper the markup lands in, so passing [RadioElement] values +// into a nested wrapped template through its dot leaves them owned by the outer +// template: an update of the inner wrapper alone replaces their DOM without their owner +// reclaiming them, leaving that to the browser's removal acknowledgement for the ids +// that reached the DOM and to the outer template's next update for any that did not. +// Re-rendering them in the inner template is not an alternative, since [RadioElement] +// allows Radio and Label at most one render each. Call RadioGroup from the template +// that renders the group to avoid the condition entirely. func (rw RequestWriter) RadioGroup(nba *named.BoolArray) (rel []RadioElement) { group := &radioGroupState{} nba.ReadLocked(func(nbl []*named.Bool) { diff --git a/lib/ui/register.go b/lib/ui/register.go index a6ece9f8..07afae9e 100644 --- a/lib/ui/register.go +++ b/lib/ui/register.go @@ -33,13 +33,10 @@ func (u Register) JawsRender(elem *jaws.Element, w io.Writer, params []any) erro // The updater's [jaws.Updater.JawsUpdate] method will be called immediately to // ensure the initial rendering is correct. // -// Register creates its Element through [jaws.Request.NewElement] rather than -// [RequestWriter.NewUI], so a surrounding [Template] neither owns nor unregisters it -// when the template re-renders. Cleanup falls to the browser, which reports the JaWS -// ids it removed from the DOM as the surrounding wrapper's new content is applied. Use -// the returned [jid.Jid] as an element id for that to work: an Element whose id never -// reaches the DOM stays registered until the [jaws.Request] ends, and every re-render -// of the template adds another. +// A surrounding [Template] owns the Element and unregisters it when the template next +// replaces its content, so repeated updates do not accumulate registrations. Rendered +// outside a template — through a [RequestWriter] a caller built itself — the Element +// stays registered until the [jaws.Request] ends. // // Register does not call [jaws.Renderer.JawsRender]. The updater must therefore // be ready for JawsUpdate and event handling without render-time initialization. @@ -51,7 +48,13 @@ func (u Register) JawsRender(elem *jaws.Element, w io.Writer, params []any) erro // //
...
func (rw RequestWriter) Register(updater jaws.Updater, params ...any) jid.Jid { + // Create and report the Element rather than going through RequestWriter.NewUI, so + // a surrounding Template owns it without anything being rendered: NewUI calls + // JawsRender, which appends a debug comment when Jaws.Debug is set, and the + // documented usage puts the returned Jid inside an attribute + // (
), where that comment would corrupt the markup. elem := rw.NewElement(Register{Updater: updater}) + rw.trackElement(elem) elem.Tag(updater) // The wrapping Register element's UI is not the updater, so events reach the // updater only through the element's handler list, not the elem.UI() fallback. diff --git a/lib/ui/requestwriter.go b/lib/ui/requestwriter.go index e27d491c..a15bee1a 100644 --- a/lib/ui/requestwriter.go +++ b/lib/ui/requestwriter.go @@ -11,15 +11,23 @@ import ( type RequestWriter struct { *jaws.Request io.Writer - // elementRendered, when non-nil, is called by NewUI with each Element it - // successfully rendered, letting the widget that owns this writer track the - // Elements created through it. A returned error fails the NewUI call. + // elementCreated, when non-nil, is called with every Element created through + // this writer, letting the widget that owns the writer track them. It is called + // as soon as the Element exists, before it renders and whether or not it ever + // does, so an implementation must not assume rendered state. // // It is deliberately unexported: RequestWriter is embedded in [With], so an // exported func field would be callable from any template as - // {{call $.ElementRendered $.Element}}, letting a template make an Element own + // {{call $.ElementCreated $.Element}}, letting a template make an Element own // itself and have its own wrapper unregistered on the next update. - elementRendered func(elem *jaws.Element) (err error) + elementCreated func(elem *jaws.Element) +} + +// trackElement reports a newly created Element to the writer's owner, if any. +func (rw RequestWriter) trackElement(elem *jaws.Element) { + if rw.elementCreated != nil { + rw.elementCreated(elem) + } } // NewUI creates an element for ui and renders it to the underlying writer. @@ -28,12 +36,12 @@ type RequestWriter struct { // requirements documented by [jaws.UI]. func (rw RequestWriter) NewUI(ui jaws.UI, params ...any) (err error) { elem := rw.NewElement(ui) - if err = elem.JawsRender(rw, params); err == nil { - if rw.elementRendered != nil { - err = rw.elementRendered(elem) - } - } - if err != nil { + // Report the Element before rendering it, so the owner's set is complete even for + // one that fails. That set may then hold an Element already unregistered below, + // which costs nothing: Request.DeleteElements skips elements it finds + // unregistered, and every rollback path deletes the whole set at once. + rw.trackElement(elem) + if err = elem.JawsRender(rw, params); err != nil { // Unregister anything the failed Element already owns along with it, so no // widget can strand a subtree by not rolling back itself. deleteOwnedElements(rw.Request, []*jaws.Element{elem}) diff --git a/lib/ui/template.go b/lib/ui/template.go index b57eb388..8a1d08fb 100644 --- a/lib/ui/template.go +++ b/lib/ui/template.go @@ -28,20 +28,19 @@ import ( // helper. The referenced template must be a partial template, not a full HTML // document. // -// The Elements a template creates through [RequestWriter.NewUI] — the path taken by -// every RequestWriter widget helper except [RequestWriter.Register] and -// [RequestWriter.RadioGroup], including a nested [RequestWriter.Template] — belong to -// the Template that rendered them: when [Template.JawsUpdate] replaces the wrapper's -// content, those Elements are unregistered along with the DOM that held them, and a -// nested widget's own Elements go with it. +// Every Element a template creates through the [RequestWriter] it is given belongs to +// the Template that rendered it — the widget helpers, [RequestWriter.Register], +// [RequestWriter.RadioGroup] and a nested [RequestWriter.Template] alike. When +// [Template.JawsUpdate] replaces the wrapper's content, those Elements are +// unregistered along with the DOM that held them, and a nested widget's own Elements +// go with it. // -// Those two helpers create their Elements through [jaws.Request.NewElement] instead, -// so a Template neither tracks nor unregisters them. Their cleanup is left to the -// browser, which reports the JaWS ids it removed from the DOM as the wrapper's new -// content is applied, and the [jaws.Request] unregisters those Elements. An Element -// whose id never reaches the DOM has nothing to report it and stays registered until -// the Request ends: one from an execution that failed before its markup was -// delivered, or from a Register call whose returned Jid the template discards. +// Ownership is recorded when an Element is created rather than after it renders, so +// one that never reaches the browser is reclaimed too: an Element whose render failed, +// or a radio Element left unrendered by a [RadioElement.Label] without its +// [RadioElement.Radio]. See [RequestWriter.RadioGroup] for the one attribution +// condition, which applies when a group's markup ends up in a different wrapper than +// the RadioGroup call. // // Template execution is best-effort rather than transactional. Template actions // and nested JaWS helpers run as the template executes, so an execution error @@ -81,12 +80,12 @@ func (tmpl *Template) String() string { } // ownElement records child as created while tmpl's template executed. It is the -// [RequestWriter] element-rendered hook installed by execute. -func (tmpl *Template) ownElement(child *jaws.Element) (err error) { +// [RequestWriter] element-created hook installed by execute, so it is called as soon +// as the Element exists and makes no assumption about whether it rendered. +func (tmpl *Template) ownElement(child *jaws.Element) { tmpl.mu.Lock() tmpl.owned = append(tmpl.owned, child) tmpl.mu.Unlock() - return } // takeOwnedElements returns the Elements created by the most recent execution and @@ -137,7 +136,7 @@ func (tmpl *Template) execute(elem *jaws.Element, w io.Writer, lookedUp *templat // races on the shared io.Writer, and on the render as a whole. err = lookedUp.Execute(w, With{ Element: elem, - RequestWriter: RequestWriter{Request: elem.Request, Writer: w, elementRendered: tmpl.ownElement}, + RequestWriter: RequestWriter{Request: elem.Request, Writer: w, elementCreated: tmpl.ownElement}, Dot: tmpl.Dot, Auth: tmpl.auth(elem), }) diff --git a/lib/ui/template_owned_test.go b/lib/ui/template_owned_test.go index 583796a3..3ac81365 100644 --- a/lib/ui/template_owned_test.go +++ b/lib/ui/template_owned_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "html/template" + "slices" "strings" "sync" "testing" @@ -28,6 +29,12 @@ const ownedTestTemplates = ` {{define "owned-container"}}{{$.RequestWriter.Container "div" $.Dot.Container}}{{end}} {{define "owned-register"}}
{{end}} {{define "owned-radiogroup"}}{{range $.RequestWriter.RadioGroup $.Dot.Radios}}{{.Radio}}{{.Label}}{{end}}{{end}} +{{define "owned-register-discarded"}}registered as text: {{$.RequestWriter.Register $.Dot}}{{end}} +{{define "owned-labelonly"}}{{range $.RequestWriter.RadioGroup $.Dot.Radios}}{{.Label}}{{end}}{{end}} +{{define "owned-register-failafter"}}
{{$.Dot.Check}}{{end}} +{{define "owned-radiogroup-failafter"}}{{range $.RequestWriter.RadioGroup $.Dot.Radios}}{{.Radio}}{{.Label}}{{end}}{{$.Dot.Check}}{{end}} +{{define "owned-radio-outer"}}{{$.RequestWriter.Template "div" "owned-radio-inner" ($.Dot.Box $.RequestWriter)}}{{end}} +{{define "owned-radio-inner"}}{{range $.Dot.Elements}}{{.Radio}}{{.Label}}{{end}}{{end}} ` var errOwnedDotCheck = errors.New("owned dot check failed") @@ -40,6 +47,7 @@ type ownedDot struct { container *testContainer names []string radios *named.BoolArray + box *ownedRadioBox } func (d *ownedDot) Check() (string, error) { @@ -60,6 +68,33 @@ func (d *ownedDot) Names() []string { return d.names } func (d *ownedDot) Radios() *named.BoolArray { return d.radios } +// Box builds the radio group with the passed-in writer — the outer template's — and +// returns the box carrying the RadioElement values to the nested template, whose dot it +// becomes. It is a pointer so it is usable as a tag. +func (d *ownedDot) Box(rw RequestWriter) *ownedRadioBox { + d.box.rel = rw.RadioGroup(d.radios) + return d.box +} + +// ownedRadioBox carries RadioElement values across a template boundary, so a test can +// render them in a nested template while the outer template owns their Elements. +type ownedRadioBox struct { + rel []RadioElement + show bool + execs int // executions of the nested template that read Elements +} + +// Elements returns the group to render, or nothing once show is cleared. It counts +// calls so a test can tell an update that ran and rendered nothing from one that never +// executed. +func (b *ownedRadioBox) Elements() []RadioElement { + b.execs++ + if !b.show { + return nil + } + return b.rel +} + // JawsUpdate makes ownedDot usable as the $.Register updater. Register tags its // Element with the updater, so the dot still tags the whole subtree. func (d *ownedDot) JawsUpdate(*jaws.Element) {} @@ -324,30 +359,35 @@ func TestTemplate_RenderClosingWriteFailureDeletesOwnedElements(t *testing.T) { } } -// TestRequestWriter_NewUIElementRenderedFailureDeletesOwnedElements checks that a -// failing element-rendered hook unregisters the Element and everything it owns. -func TestRequestWriter_NewUIElementRenderedFailureDeletesOwnedElements(t *testing.T) { +// TestRequestWriter_NewUIReportsElementBeforeRendering checks that NewUI reports the +// Element it created even when the render then fails, and unregisters it regardless. +// Reporting at creation is what lets an owner reclaim an Element that never rendered. +func TestRequestWriter_NewUIReportsElementBeforeRendering(t *testing.T) { _, rq := newOwnedRequest(t) - hookErr := errors.New("hook refused the element") - var seen int + var seen []*jaws.Element var sb strings.Builder rw := RequestWriter{ Request: rq, Writer: &sb, - elementRendered: func(elem *jaws.Element) error { - seen++ - return hookErr + elementCreated: func(elem *jaws.Element) { + seen = append(seen, elem) }, } - if err := rw.Template("div", "owned-parent", &ownedDot{}); !errors.Is(err, hookErr) { - t.Fatalf("NewUI error = %v, want %v", err, hookErr) + // The template renders nested UI and then fails, so this writer creates one + // Element (the wrapper) and the nested writer inside it creates its own. + dot := &ownedDot{fail: errOwnedDotCheck} + if err := rw.Template("div", "owned-failafter", dot); !errors.Is(err, errOwnedDotCheck) { + t.Fatalf("render error = %v, want %v", err, errOwnedDotCheck) + } + if len(seen) != 1 { + t.Fatalf("hook calls = %d, want 1 (only Elements created directly through this writer)", len(seen)) } - if seen != 1 { - t.Fatalf("hook calls = %d, want 1 (only Elements created through this writer)", seen) + if !seen[0].Deleted() { + t.Error("the reported Element was not unregistered after its render failed") } if got := countRegistered(t, rq); got != 0 { - t.Fatalf("registered elements after hook failure = %d, want 0", got) + t.Fatalf("registered elements after failed render = %d, want 0", got) } } @@ -459,43 +499,43 @@ func TestPageTemplate_RenderFailureDeletesOwnedElements(t *testing.T) { } } -// TestTemplate_UpdateDoesNotTrackRegisterOrRadioGroup pins the documented -// exclusion: Register and RadioGroup create their Elements through -// Request.NewElement rather than RequestWriter.NewUI, so a Template does not own -// them and each update registers another set. +// TestTemplate_UpdateTracksRegisterAndRadioGroup covers the two helpers that create +// their Elements through Request.NewElement rather than RequestWriter.NewUI: they +// report them to the writer's owner, so the counts stay flat across updates with no +// client attached to acknowledge DOM removals. // -// The growth measured here is what the server does on its own. In a live session the -// browser reports the ids it removes as the wrapper's new content is applied and the -// Request unregisters those Elements; there is no client here to send that -// acknowledgement, which is also the state of an Element whose id never reaches the -// DOM. Should either helper start reporting through the element-rendered hook, these -// counts become constant — update this test along with the Template, Register, -// RadioGroup, README and skill docs stating the exclusion. -func TestTemplate_UpdateDoesNotTrackRegisterOrRadioGroup(t *testing.T) { - radios := named.NewBoolArray(false) - radios.Add("1", "one") +// The label-only case is why ownership is recorded at creation: RadioElement.Label +// creates the radio Element for its for= attribute without ever rendering it, so an +// after-render notification would miss it and it would accumulate. +func TestTemplate_UpdateTracksRegisterAndRadioGroup(t *testing.T) { + newRadios := func() *named.BoolArray { + nba := named.NewBoolArray(false) + nba.Add("1", "one") + return nba + } for _, tt := range []struct { name string template string dot *ownedDot - perRound int // Elements the helper adds per execution + perRound int // Elements the helper creates per execution }{ {"register", "owned-register", &ownedDot{}, 1}, - {"radiogroup", "owned-radiogroup", &ownedDot{radios: radios}, 2}, // input and label + {"register discarded jid", "owned-register-discarded", &ownedDot{}, 1}, + {"radiogroup", "owned-radiogroup", &ownedDot{radios: newRadios()}, 2}, // input and label + {"radiogroup label only", "owned-labelonly", &ownedDot{radios: newRadios()}, 2}, // unrendered radio, label } { t.Run(tt.name, func(t *testing.T) { _, rq := newOwnedRequest(t) tmpl := NewTemplate("div", tt.template, tt.dot) elem := renderOwned(t, rq, tmpl) - want := 1 + tt.perRound // the wrapper, plus the first execution's + want := 1 + tt.perRound // the wrapper, plus this execution's if got := countRegistered(t, rq); got != want { t.Fatalf("registered elements after render = %d, want %d", got, want) } for round := 1; round <= 3; round++ { tmpl.JawsUpdate(elem) - want += tt.perRound if got := countRegistered(t, rq); got != want { t.Fatalf("registered elements after %d update(s) = %d, want %d", round, got, want) } @@ -504,6 +544,125 @@ func TestTemplate_UpdateDoesNotTrackRegisterOrRadioGroup(t *testing.T) { } } +// TestTemplate_UpdateFailureKeepsRegisterAndRadioGroupGeneration covers the remaining +// leak mode: execution failing after either helper has already created its Elements. +// The failed execution's output is discarded, so its Elements must go and the previous +// generation must stay live to match the unchanged DOM. +func TestTemplate_UpdateFailureKeepsRegisterAndRadioGroupGeneration(t *testing.T) { + newRadios := func() *named.BoolArray { + nba := named.NewBoolArray(false) + nba.Add("1", "one") + return nba + } + + for _, tt := range []struct { + name string + template string + dot *ownedDot + perRound int + }{ + {"register", "owned-register-failafter", &ownedDot{}, 1}, + {"radiogroup", "owned-radiogroup-failafter", &ownedDot{radios: newRadios()}, 2}, + } { + t.Run(tt.name, func(t *testing.T) { + jw, rq := newOwnedRequest(t) + logger := new(templateLogger) + jw.Logger = logger + + tmpl := NewTemplate("div", tt.template, tt.dot) + elem := renderOwned(t, rq, tmpl) + want := 1 + tt.perRound + if got := countRegistered(t, rq); got != want { + t.Fatalf("registered elements after render = %d, want %d", got, want) + } + first := registeredJids(t, rq) + + tt.dot.setFail(errOwnedDotCheck) + tmpl.JawsUpdate(elem) + if len(logger.errors) != 1 || !errors.Is(logger.errors[0], errOwnedDotCheck) { + t.Fatalf("logged errors = %v, want one %v", logger.errors, errOwnedDotCheck) + } + if got := registeredJids(t, rq); !slices.Equal(got, first) { + t.Fatalf("registered jids after failed update = %v, want the previous generation %v", got, first) + } + + // The previous generation must still be owned, so the next success reclaims + // it rather than leaking it. + tt.dot.setFail(nil) + tmpl.JawsUpdate(elem) + if got := countRegistered(t, rq); got != want { + t.Fatalf("registered elements after recovery = %d, want %d", got, want) + } + if got := registeredJids(t, rq); slices.Equal(got, first) { + t.Fatal("the recovering update did not replace the previous generation") + } + }) + } +} + +// TestRadioGroup_OwnedByTheTemplateThatCalledIt pins where ownership is attributed +// when a group's markup lands in a different wrapper than the RadioGroup call: the +// outer template calls $.RadioGroup and passes the RadioElement values to a nested +// wrapped template through its dot. +// +// Updating the outer template alone would not distinguish the two candidates, since +// the cleanup walk recurses into the nested Template's own owned set either way. The +// discriminating step is updating the nested Element on its own: with the radios no +// longer rendered and no client to acknowledge the DOM removal, they must still be +// registered, which is what fails if the nested template owned them. +func TestRadioGroup_OwnedByTheTemplateThatCalledIt(t *testing.T) { + _, rq := newOwnedRequest(t) + + radios := named.NewBoolArray(false) + radios.Add("1", "one") + box := &ownedRadioBox{show: true} + dot := &ownedDot{radios: radios, box: box} + outer := NewTemplate("div", "owned-radio-outer", dot) + outerElem := renderOwned(t, rq, outer) + + // outer wrapper, nested wrapper, radio, label + const want = 4 + if got := countRegistered(t, rq); got != want { + t.Fatalf("registered elements after render = %d, want %d", got, want) + } + inner := rq.GetElements(box) + if len(inner) != 1 { + t.Fatalf("nested wrapper elements = %d, want 1", len(inner)) + } + + // The nested template updates on its own and stops rendering the group. Counting + // its executions keeps the assertion below from passing vacuously: the radios must + // survive an update that ran and dropped them, not one that never happened. + box.show = false + execsBefore := box.execs + inner[0].JawsUpdate() + if box.execs != execsBefore+1 { + t.Fatalf("nested template executions = %d, want %d: the nested update did not run", + box.execs, execsBefore+1) + } + if got := countRegistered(t, rq); got != want { + t.Fatalf("registered elements after the nested update = %d, want %d: the radio and label "+ + "belong to the template that called RadioGroup, so the nested update must not reclaim them", got, want) + } + + // The owner reclaims them, along with the nested wrapper it also owns. + outer.JawsUpdate(outerElem) + if got := countRegistered(t, rq); got != 2 { + t.Fatalf("registered elements after the outer update = %d, want 2 (both wrappers, no group)", got) + } +} + +// registeredJids returns the Jids still registered in rq, in ascending order. +func registeredJids(t *testing.T, rq *jaws.Request) (jids []jaws.Jid) { + t.Helper() + for jid := jaws.Jid(1); jid <= maxProbedJid; jid++ { + if rq.GetElementByJid(jid) != nil { + jids = append(jids, jid) + } + } + return +} + // TestTemplate_UpdateReclaimsManyTaggedDescendants exercises the batched // unregister with a generation large enough to span several tag entries. func TestTemplate_UpdateReclaimsManyTaggedDescendants(t *testing.T) { From ebe8c0add8c9f9092708fa9eacc64c3ba6891245 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 3 Aug 2026 11:52:29 +0200 Subject: [PATCH 2/2] docs: do not promise request-lifetime registration without a template owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register and RadioElement said an Element with no template owner stays registered until the Request ends. Losing the owner only removes one cleanup path: the browser still reports the JaWS ids it removes when an ancestor's content is replaced, and Element.Remove unregisters a managed child outright. Say that cleanup falls to the ordinary DOM-removal handling in that case, and that only an Element no removal ever reports — one whose Jid never becomes an element id — necessarily lasts until request teardown. For the radio Element a Label leaves behind, that condition is met by construction: it has no DOM node for any removal to report, so the original claim holds there and now says why. --- lib/ui/radiogroup.go | 4 ++-- lib/ui/register.go | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/ui/radiogroup.go b/lib/ui/radiogroup.go index 4e2c2ccd..969d8868 100644 --- a/lib/ui/radiogroup.go +++ b/lib/ui/radiogroup.go @@ -17,8 +17,8 @@ import ( // for="..." referencing the radio's id, so a Label without its Radio points at an // input that is absent from the document. The radio Element it created is still // unregistered by the [Template] that owns it (see [RequestWriter.RadioGroup]) when -// that template next replaces its content, or stays registered until the -// [jaws.Request] ends if the group was rendered outside a template. +// that template next replaces its content. With no template owner it has no DOM node +// for a removal to report either, so it stays registered until the [jaws.Request] ends. type RadioElement struct { st *radioState } diff --git a/lib/ui/register.go b/lib/ui/register.go index 07afae9e..1689d53e 100644 --- a/lib/ui/register.go +++ b/lib/ui/register.go @@ -34,9 +34,13 @@ func (u Register) JawsRender(elem *jaws.Element, w io.Writer, params []any) erro // ensure the initial rendering is correct. // // A surrounding [Template] owns the Element and unregisters it when the template next -// replaces its content, so repeated updates do not accumulate registrations. Rendered -// outside a template — through a [RequestWriter] a caller built itself — the Element -// stays registered until the [jaws.Request] ends. +// replaces its content, so repeated updates do not accumulate registrations. With no +// template owner — rendered through a [RequestWriter] a caller built itself — cleanup +// falls to the ordinary DOM-removal handling: the browser reports the JaWS ids it +// removes when an ancestor's content is replaced, and [jaws.Element.Remove] unregisters +// a managed child outright. Only an Element no removal ever reports, such as one whose +// returned [jid.Jid] never becomes an element id, necessarily stays registered until the +// [jaws.Request] ends. // // Register does not call [jaws.Renderer.JawsRender]. The updater must therefore // be ready for JawsUpdate and event handling without render-time initialization.