Skip to content

Fix worksheet solution workspace order - #3154

Open
oscarlevin wants to merge 3 commits into
PreTeXtBook:masterfrom
oscarlevin:fix-worksheet-solution-workspace-order
Open

Fix worksheet solution workspace order#3154
oscarlevin wants to merge 3 commits into
PreTeXtBook:masterfrom
oscarlevin:fix-worksheet-solution-workspace-order

Conversation

@oscarlevin

Copy link
Copy Markdown
Member

This fixes a regression introduced in #3114 and the follow-up commits that caused workspace to be shown before solutions in printouts. This might partially fix the issues identified in #3151, so it would be good to merge this one first. I can then go back and look at the other fixes in #3151 and reconcile them.

By the way, as soon as both of these are merged, I'm going to refactor the add_on.js to pull out all this logic into its own file. It is getting big.

@rbeezer

rbeezer commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Read through; the design holds up and the comments carry their weight. Verified: all three commits are clean on whitespace, the sample article builds and the two added <solution>s raise nothing new in validation, and the DOM ordering the hoist depends on is real — the XSL does emit div.solutions before div.workspace inside an exercise. No browser was available to me, so what follows comes from reading the code paths rather than from watching a printout repaginate.

1. hideWidowedWorkspaces() suppresses with an inline style.display, and it sticks to whichever element was the row at the time.

Line 998 sets display:none on the stranded row. If that row is a bare .workspace and the reader then ticks "highlight workspace", toggleWorkspaceHighlight() wraps it (line 1564 selects every .workspace, hidden ones included) and the new .workspace-container becomes the row. After that, hideWidowedWorkspaces() only ever writes display on the container; the inner .workspace keeps display:none for the rest of the session, so hiding the solution again restores the container but not the writing space. That is the opposite of the contract in the function's own comment — "so that a later toggle which brings the group back onto one page brings the writing space back with it."

2. The same inline style makes a stranded workspace deletable.

createPrintoutPages() skips a row measuring offsetHeight === 0 unless it carries the class hidden (line 872), and the closing loop deletes every printout child that did not land in a .onepage (lines 937-940). A workspace with an inline display:none measures zero and has no such class, so a full recompute destroys it, and nothing in between resets the style. I could not construct a sequence that reaches this today — the only post-reveal full recompute is the settle tick at line 2128, and the initial reveal is chained after it — but it is one call site away, and the comment above that filter is there because this deletion has already eaten hidden solution content once.

Both of these go away if the suppression uses the hidden class instead of an inline style, applied and cleared on the workspace and its container together. moveDepthClass() already establishes that those two have to be kept in step when the row role moves.

3. The cleanup loop iterates a live collection while removing from it.

for (const child of printout.children) { ... printout.removeChild(child) } at line 937 skips an element on every removal. Pre-existing, but the new comment directly above it names the hazard and then leaves the loop alone, and this branch leans harder on that loop being exact. [...printout.children] belongs in the same change.

4. The justification for the group-id counter is inverted.

Line 666 explains the call-local groupSeq with "pagination is rebuilt from scratch on every recompute". It isn't — the second createPrintoutPages() finds the .solutions wrappers already gone, reissues nothing, and the first pass's data-block-group stamps persist. That persistence is what makes the scheme work; the stated reason is the opposite of what makes it safe.

5. Optional. depthClassForRowOut() returns subtask for a row out of a task, and both the stylesheet and the JS then spend a paragraph explaining why the name is one step deeper than the margin it selects. Stamping the owner's own class and letting one rule serve both would retire both paragraphs. Not a defect, just churn you may or may not think it worth.

Claude Opus 5, acting as a review assistant for Rob Beezer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants