Line a held-back cascade step's message up with its number - #1681
Open
dqnykamp wants to merge 6 commits into
Open
Line a held-back cascade step's message up with its number#1681dqnykamp wants to merge 6 commits into
dqnykamp wants to merge 6 commits into
Conversation
A `<problem>`/`<task>`/`<part>` that a `<cascade>` is holding back shows exactly one child, its `<cascadeMessage>`, but `firstVisibleChild` treated the whole step as showing nothing: it returned `null` whenever `hideChildren` was set. The message therefore led nothing, so the item dropped out of the numbering grid it uses for every other item and the message kept the top margin that drew it a line below its own number. `childrenToHide` is the whole test on its own. It holds every child of a held-back step except the message — the one child whose hiding rule is inverted — so dropping the `hideChildren` test makes the message the lead while the step is held back and changes nothing once it is revealed, where the message is the hidden child and the content leads. `hideChildren` is no longer requested: `childrenToHide` and `childIndicesToRender` are both computed from it and both answer differently across the transition, so the lead still follows a cascade advancing. That direction is asserted in the new test. Closes Doenet#1680. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
dqnykamp
force-pushed
the
fix/cascade-message-list-item-lead
branch
from
August 10, 2026 17:54
4fee7ce to
bf38e15
Compare
…test Review-cycle follow-up to the previous commit; no behavior change. `firstVisibleChild`'s comment said `childrenToHide` "holds every child" of a held-back step with no message. It holds every *component* child: a string child is never in it, and is kept off the screen by being dropped from `childIndicesToRender` instead. Say both, so the reason such a step delegates to nobody is the actual one. The staleness note is replaced with the argument that holds for every shape of section rather than for two of them. Requesting `hideChildren` would not keep the lead any fresher because `childrenToHide` is computed from `hideChildren`: marking it stale marks `firstVisibleChild` stale, and `markUpstreamDependentsStale` compares no values on the way, so it does not matter whether the remaining dependencies' values happen to differ across the transition. Verified by sweeping the transition over every shape of held-back step — strings only, strings around the message, no children, configuration children only, a title, a composite child with and without a message, a message after the content, a message alone, a non-rendering child ahead of the content, a hidden message, a blank string, and `asList` — checking that the lead is never a hidden child and never stale afterwards. Also: the pre-existing cascade test's comment credited the deleted `hideChildren` test with suppressing the delegation, which it no longer does, and the new Cypress row check used `lessThan(2)` where the value must be 0 and any negative offset would be just as wrong. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… the layout A six-line comment defending a dependency that is not there costs more than the fact it records: the reasoning is now one clause of the doc comment above `firstVisibleChild`, which already had to explain that a held-back step shows its `<cascadeMessage>`. Also assert the layout the held-back item rejoins with the helper every other untitled, unboxed item in `problem.cy.js` is held to, rather than only its positional outcome; and qualify Doenet#1677's still-unreleased changeset, which says flatly that a `<cascadeMessage>` no longer takes the lead — true only of a hidden one once this entry lands beside it in the same CHANGELOG. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
Review-cycle follow-up; no behavior change. The Cypress guard held back a single step, which cannot tell "every held-back step leads with its own message" from "the step the cascade stopped at does". Its cascade now has three steps, so two are held back at once — the shape any cascade longer than two has from the start — and each is checked for its own message on its own first row, with all three numbers held to one decimal column across the two numbering layouts in play. Read off the screen against builds with and without the deleted condition: the pre-fix column of the PR's table is a measurement now rather than an estimate (and its post-fix item height was 38px, not 34px); a step that is `boxed` or titled renders identically before and after, since the `nonBoxedListItemWithoutTitle` gate excludes it and such a step draws its number in a heading of its own with the message below — so the changeset now says that, rather than promising every held-back message a shared row. The live reveal was watched in one page load: step 2's number moves onto the row of the content replacing its message while step 3 stays on its message. Also reflow a comment line left ragged by the previous commit. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Review-cycle follow-up; no behavior change. The third problem and the loop around it bought nothing. `firstVisibleChild` is computed per section and knows nothing about its position in the cascade, so a second held-back step traverses exactly the same path as the first — about twenty lines of scaffolding for a duplicate traversal on a two-line fix. The changeset sentence promising every held-back step the same treatment goes with it: the entry already says "a held-back cascade step" without qualification. `verifyListItemNumbersAlign` goes too, and its comment was why: it claimed the held-back item "numbers itself through a different layout than the revealed one beside it, and the two have to agree at the decimal". That was true before the fix and is false after it — both items are grid items now — and the check cannot fail for Doenet#1680 either way (measured: content starts at x=60 for every item in both builds, because the pre-fix hanging indent puts the number in the same column). `verifyUntitledUnboxedListItemUsesGridLayout` already pins the number's column, and the file has three dedicated Doenet#1482 alignment tests. Re-derived rather than inherited, against builds with and without the deleted condition: all eight numbers in the PR's table hold (block/38px/16px/60px → grid 32px 818px/0px/0px/38px), as do 16 pass + 1 fail for `cascade.test.ts` and 24 + 1 for `problem.cy.js` pre-fix, and 93 / 25 / 30 / 13+3 post-fix. A titled and a boxed held-back step were measured to draw their number in a heading with the message below, and the reveal was driven in the page. One claim did not hold: a cascade's first step is *not* unreachable as a held-back step. `numCompleted` counts leading children whose credit is 1 or absent, so a plain `<p>` ahead of the first step drags it along, but a *scoring* non-section child does not — `<cascade><answer>x</answer><problem> <cascadeMessage/>…` holds problem 1 back, shows its message, and (pre-fix) showed the bug. The fix handles it: the lead is the message and the grid is on. The PR body's "unreachable" paragraph is gone; the shape joins the swept list instead. Also say both halves of why a held-back step with nothing to show still lands on `null`: `childrenToHide` holds its component children, and `childIndicesToRender` drops its strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
Review-cycle follow-up; no behavior or assertion change. Both comments claimed more than the file holds. The Cypress guard said its grid-layout helper is what "every other untitled, unboxed item in this file" is held to, but the four `<problem>`s in the Doenet#1482 alignment tests are untitled and unboxed and are checked with `verifyListItemNumbersAlign` instead; the claim is now about the helper's job rather than its coverage. The worker test pointed at "the test two below", which any inserted test breaks silently — it names the test instead. Also "every child it has" -> "every component child it has", since the strings are dropped by `childIndicesToRender`, not by `childrenToHide` — the split the state variable's own comment draws two lines later. Re-derived rather than inherited: all eight numbers in the PR's table hold against builds with and without the deleted condition (block/38px/16px/60px -> grid `32px 818px`/0px/0px/38px at a 1000px viewport), as do 16 pass + 1 fail for `cascade.test.ts` and 24 + 1 for `problem.cy.js` pre-fix, and 93 / 25 / 30 / 13+3 post-fix. Schema regenerates with no diff. The Cypress guard is left at one held-back step. A second one measured bit-identical to the first in both builds — `display`, height, content-start x, `::before` column, and its message's margin and row offset all agree — so it is a duplicate traversal, and the fact it was meant to pin (every held-back step shows its own message, not only the next one) is already asserted at every `numCompleted` by "continuation messages inside sections". `verifyListItemNumbersAlign` stays out for the reason it was cut: content starts at x=60 for every item in both builds, so it cannot fail for Doenet#1680 in either direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
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.
Closes #1680.
Rebased onto
mainnow that #1677 (its predecessor, for #1673) has landed, so this diff is its own change alone: the fix, a changeset, two tests, and one qualifying sentence in #1677's still-unreleased changeset.The bug
A
<problem>/<task>/<part>that a<cascade>is holding back has its number drawn a line above the message telling the reader what to finish first:Problem 2 while problem 1 is unanswered, in headless Chrome at a 1000px-wide viewport. Both columns are measurements, not estimates: the
beforecolumn comes from a build of this branch with the deleted condition put back.displayblockgrid(32px 818px)margin-topCause
SectioningComponent'sfirstVisibleChildreturnednulloutright wheneverhideChildrenwas set. But a held-back step shows exactly one child: its<cascadeMessage>— the child whose hiding rule is inverted, hidden precisely when everything else is shown, and the reason thechildrenToHidetest on the next line exists. So the message led nothing: no lead means nouseListItemGridLayout, and the item fell back to the layoutsection.tsxkeeps for a number-only item, where the number is an inline-block hung into the start indent by a negative margin and whatever follows it is a block on the next line. The message, keeping its own top margin, was that block.#1677 already named that test as too broad and pointed at #1680 rather than touching it, because it is a different defect in a different layout path. This PR removes it.
Fix
One condition deleted.
childrenToHideis the whole test on its own:childrenToHideholds every component child of the step but its title and its message, so the message is the lead, the item numbers itself with the grid layout, and the message's top margin is suppressed;childrenToHideand is skipped, so the content behind it leads, exactly as before.A held-back step that really shows nothing still delegates to nobody without the deleted test:
childrenToHidecatches its component children, andchildIndicesToRenderdrops its strings.hideChildrenis no longer requested either. Both remaining dependencies,childrenToHideandchildIndicesToRender, are computed fromhideChildren, andmarkUpstreamDependentsStalerecords a potential change on every upstream dependency without comparing values — so a<cascade>holding a step back or letting it go marks this variable stale whichever way the values work out. The new test asserts the transition in both directions rather than leaving it to the argument.The blast radius is that one line:
firstVisibleChildis gated behindnonBoxedListItemWithoutTitle(isListItem && !boxed && !collapsible && titleChildName === null), which is the same conditionsection.tsxemits the hanging-number grid under, and all four of its consumers —childrenToRenderInlineForListItem,firstVisibleChildAdjustedForListItem,useListItemGridLayout, andfirstChildListItemAlignmentthrough the second of those — require that flag too. Within such an item,childrenToHideleaves out only the title (excluded by that gate) and<cascadeMessage>, so the only child a held-back step can newly lead with is a message it is actually showing. A step that is titled orboxedis outside the gate, so it is untouched by construction; measured, it draws its number in a heading of its own — in front of the title, or inside the box header — with the message a line below, which is the ordinary layout for such an item rather than the bug being fixed here.The suppression lands on the right element in every state, including a
<cascadeMessage>that is not its step's first child: the children ahead of it render nothing while the step is held back, so the message is both the core's lead (it getsrenderInlineForListItem) and the content wrapper's first element (it gets the> :first-childrule), and the two agree.No renderer change:
cascadeMessage extends P(Aliases.js), so it already declares alistItemInlineAlignmentand already consumesrenderInlineForListItemfor its margin.Tests
cascade.test.ts"gives a held-back step's lead to the cascadeMessage it shows": the held-back state (lead is the message,useListItemGridLayout,firstChildListItemAlignmentisbaseline, margin suppressed), then the same assertions after answering the previous step, where the lead moves to the content and the message becomes the hidden child. It is the mirror of the existing "does not give a list item's lead to a hidden cascadeMessage".problem.cy.js"a held-back step's cascadeMessage shares the row with its number": the rendered outcome —margin-top: 0pxand the message's top edge on the item's own first row — thenverifyUntitledUnboxedListItemUsesGridLayout, the helper this file checks an ordinary untitled unboxed item's numbering grid with, so the grid the item rejoined is pinned and not just its outcome.Both fail with the condition restored, and nothing else does:
cascade.test.ts16 pass / 1 fail,problem.cy.js24 pass / 1 fail. With the fix:cascade/sectioning/listsworker suites 93 pass,problem.cy.js25,list.cy.js30,sectioning.cy.js13 (+3 pre-existing pending). Schema regenerates with no diff.Beyond the two tests, the transition was swept over every shape of held-back step — no children at all, strings only, strings around the message, configuration children only, a title, a composite (
<repeat>) child with and without a message, a message after the content, a message alone, a non-rendering<solveEquations>ahead of the content, ahideden message,asList, and a cascade's first step held back, which is reachable when a scoring non-section child such as a bare<answer>precedes it — checking that the lead is never a child whosehiddenis true and never stale after the cascade advances. No shape produces either. The reveal was also watched live in a browser: answering step 1 in the page moves step 2's number onto the row of the content that replaces its message, with nothing left stale.The one line changed outside this fix
.changeset/list-item-hidden-first-child.mdis #1677's and is still unreleased, so its entry and this one land in the same CHANGELOG. It said flatly that "a<cascadeMessage>no longer takes the lead", which this release makes false in the held-back half: the sentence is now qualified to the hidden case and points at this entry for the other half.🤖 Generated with Claude Code