Fit the Page map list inside its sheet and read six nightly fixtures at the layer's current contracts - #236
Conversation
…inside the sheet, and read the nightly fixtures as the layer now draws them
|
This run was dispatched for the older run 33698883459 and reached the same seven cases independently. This PR covers all of them and is green, so I am not opening a competing one — its ring-walk change and the
The second I took the other branch — one entry, one frame — and it makes The dispatcher-side alternative--- a/skills/leaf/assets/runtime/keyboard/return-stack.js
+++ b/skills/leaf/assets/runtime/keyboard/return-stack.js
@@ -62,7 +62,15 @@ export function createReturnStack({ focused, paintHere, readingBlock }) {
: null;
const result = run();
prune();
- if (frame?.active()) frames.push({ ...frame, origin });
+ // One entry, one frame. A row whose surface the reader is already standing in has
+ // entered nothing: `g L` pressed inside the leaves tray only re-lands focus in it,
+ // and a second frame there makes one entry cost two Escapes — the first restoring
+ // the workspace that entry displaced, which is the tray still standing. The row
+ // names the entry, and the held frame must still be live: a surface the reader
+ // opened by pointer, or left and came back to, still gets its rung. `prune` trims
+ // only the tail, so a buried frame this row has already given up cannot block one.
+ if (frame?.active() && !frames.some((held) => held.id === row.id && held.active()))
+ frames.push({ ...frame, id: row.id, origin });
return result;
}Full nightly on that branch, rebased on It is not the whole answer either — it leaves One more thing from my own run, not this PR's: |
# Conflicts: # tests/test_site.py
|
Merged The one conflict was The Problem section still describes seven cases on the failing run; six of them are fixed here now, and the seventh is fixed on Resolution detailMerge base
|
leaf-agent
left a comment
There was a problem hiding this comment.
Reviewed the merged tree at 912c99d. The change itself reads right and I could not fault it: the sheetReturning flag is set only on the dispatcher's own way out and consumed by the one close it queues, the sheet is the only surface that restores focus from a close handler (the keyboard reference does its restore synchronously inside setOpen, so the same task boundary does not bite there), and the test_site.py merge resolution is not just defensible but forced — bf9dd47 deleted says from AFTER_THE_DRAG, so carrying the branch's side would have raised KeyError on label["says"].
I ran the changed fixtures on the merged tree rather than reading them. The six that were red pass, plus both parameters of test_the_small_screen_map_is_a_complete_accessible_sheet. The ring walk I had to take apart, because it now fails before it reaches anything this PR changed — see below — but with ship-review held out it walks 540 stops with page-map-search lit and zero ring faults, which is both halves of the claim: the walk reaches the sheet's earlier stops through the crossing, and the sheet's list is inside the card that holds it (ring_faults is what would report the search box's ring drawn outside the sheet, and it reports nothing).
One thing the description no longer says truly, and it is worth knowing before merging rather than after. ## Testing is a nightly run on c370784; the base merge in 912c99d brought bf9dd47, and that commit reddened the nightly on its own. test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus now fails at .lf-margin-marker[data-lf-kinds~="comment"] did not open a thread card on ship-review, before the walk loop this PR rewrites ever runs — and it fails identically on bf9dd47 and on today's main tip e0a0549, so it is not this PR's. Merging this will not turn the nightly green: bf9dd47's own run leaves seven other cases red on top of the six here.
What I ran, and the split between the two causes
All on the merged tree 180441e (refs/pull/236/merge), host Chromium headless shell.
tests/test_render_margin.py::test_the_small_screen_map_is_a_complete_accessible_sheet— 2 passedtest_the_g_chord_reaches_panels_and_document_lists,test_escape_lets_go_of_the_ask_the_reader_is_standing_on,test_a_closed_leaf_clears_itself_off_the_tray,test_the_leaves_tray_takes_the_keyboard,test_a_widgets_label_takes_a_comment_inside_the_control_it_labels— 5 passedtest_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus— fails atship-review; identical failure onbf9dd47and one0a0549. Withship-reviewremoved fromRING_WALK_EXAMPLES,assert not unseen,assert not errorsandassert not faultsall pass,stopsis 540, andlitcontainspage-map-search. The remaining failure in that patched run is the expected one — the scopes whose only corpus wasship-reviewwalked nothing.
Run 33714432132 on bf9dd47 is the whole picture. Six of its thirteen are the ones this PR reads — test_a_widgets_label_takes_a_comment_inside_the_control_it_labels, test_a_closed_leaf_clears_itself_off_the_tray, test_the_leaves_tray_takes_the_keyboard, test_the_g_chord_reaches_panels_and_document_lists, test_escape_lets_go_of_the_ask_the_reader_is_standing_on, and the ring walk's original cause. The seventh case in the description, test_site.py::test_the_label_is_chrome_rather_than_words_to_quote, is gone because bf9dd47 deleted the assertion rather than fixing it, which is why that hunk had nowhere to land. The other seven are bf9dd47's: test_every_passage_in_a_real_page_can_be_quoted[feature-gallery], test_ask_addresses_follow_the_feature_gallery_s_visible_margin_controls[1200] and [390], test_the_shipped_long_thread_opens_beside_its_source_in_the_right_margin, test_a_pick_states_the_whole_set, test_selected_reactions_keep_neutral_button_furniture[dark], test_a_comment_on_a_wrapped_diff_line_names_the_line_an_unwrapped_one_names, and the ring walk's new one.
Nothing here asks for a change to this PR — the ship-review card and the six beside it are a separate commit's to answer, and folding them in would put two unrelated causes through one review. The description's ## Testing line is the one thing worth restating against the current base.
|
Restated The split between the two causes checks out from main's own run rather than from the merge ref alone: run 33714432132 on |
|
#239 takes the six nightly cases this branch does not, and one of them is the thread-card scope of Composed locally — this branch's head The list runs 33px past the sheet's bottom and 1.4px over its top. Same box this branch refits, and the refit's measurement was taken at 760×900 while that stop is the 1200×900 walk. Left here rather than duplicated in #239, since |
|
Two readings from run 33722764498 (
The page map sheet fault does not reproduce on a newer composition. Which of |
# Conflicts: # skills/leaf/assets/runtime/living-margin.js # tests/test_render_margin.py # tests/test_render_navigation.py
leaf-agent
left a comment
There was a problem hiding this comment.
Reviewed the merged tree at 91da9d3. The base merge is what changed here: e71fa110 took over the Escape fix, so this branch is now the .lf-page-map-list refit, the six fixture readings, and the paragraph that explains the focus-return rule. The refit is the pattern the same stylesheet already uses for the keyboard reference — .lf-help.open { display: flex; flex-direction: column; } over .lf-help-results { min-height: 0; overflow-y: auto; } — and the [open] guard is needed for the reason the comment gives, since the author display would otherwise beat the UA's dialog:not([open]) rule. max-height on the sheet is already a border-box measurement (.lf-ui sets box-sizing: border-box), so capping the list against the box rather than the window is the right depth for it. The ring walk's third answer is sound: __lfSeen is reset immediately before the loop, so "seen" can never be the first read, and empty = 0 is only reachable after a stop was recorded.
One thing the merge left behind, inline. The paragraph was written against this branch's two-flag design — sheetActivation for the activation path, sheetReturning for the dispatcher's — and main collapsed both into the single sheetCloseOwnsFocus. Its closing sentence still describes the two-flag world.
The paragraph was written against this branch's two-flag design; main's single sheetCloseOwnsFocus is also raised by activate and by the map action's forwarding onclick, so the dispatcher is not its only holder.
… from the press (#241) ## Problem The nightly leg of `ci` failed on [run 33726571939](https://github.com/max-sixty/leaf/actions/runs/33726571939) (`e71fa11`) with ten cases. Nine are already owned by open PRs — four by #236 (`test_a_widgets_label_takes_a_comment_inside_the_control_it_labels`, `test_a_selection_around_a_targets_buttons_does_not_deaden_them`, `test_a_closed_leaf_clears_itself_off_the_tray`, `test_the_leaves_tray_takes_the_keyboard`), four by #239 (`test_every_passage_in_a_real_page_can_be_quoted[feature-gallery]`, both parameters of `test_ask_addresses_follow_the_feature_gallery_s_visible_margin_controls`, `test_the_shipped_long_thread_opens_beside_its_source_in_the_right_margin`), and the ring walk by #239's `ship-review` precondition ahead of #236 and #240. The tenth is unowned and is what this takes: `test_render_widgets.py::test_a_wait_the_reader_would_notice_says_so_and_a_short_one_says_nothing`, first seen on this run and absent from the three nightlies before it ([33722764498](https://github.com/max-sixty/leaf/actions/runs/33722764498), [33718675826](https://github.com/max-sixty/leaf/actions/runs/33718675826), [33714432132](https://github.com/max-sixty/leaf/actions/runs/33714432132)). ``` AssertionError: a wait worth noticing said nothing: [0.702361, 0.642718, 0.583432, 0.523789, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5] ``` The test took `performance.now()` just before the press and asked that every frame past 400ms had settled at 0.5. The rule it reads is `[aria-busy="true"] { animation: … 140ms linear 200ms both; }` — 340ms in all — but that clock starts at the first frame the busy attribute is *painted* in, not at the press. `#inFlight` sets the attribute synchronously and then renders the controls and updates the margin in the same task, so the animation's start time is the first frame after all of that. Measured here, the fade begins at 233ms and completes at 383ms: 17ms inside the deadline. The reported sample is that whole timeline slipped about 120ms, which is what a loaded nightly worker costs — the frames past 400ms are still mid-fade rather than absent. ## Solution Read the fade on the rule's own clock. Each sampled frame now records the animation's `currentTime` and `playState` beside the opacity. The delay is the frames before `currentTime` reaches 150ms, including the ones before the animation exists at all; the fade is the frames the animation itself reports `finished`. Both halves of the claim survive intact — nothing paints while the wait is too short to notice, and the wait that is long enough ends at 0.5 — and neither is a deadline the machine has to beat. This is `tests/CLAUDE.md`'s **A wait consumes a fact the system states** applied to a reading that was inferring from elapsed time, using the `getAnimations()` route that section names. The sampling window goes 500ms → 700ms so a start that slips still leaves frames on the far side of the fade; it is a bounded `setTimeout` inside the page, not an awaited `animation.finished`. ## Testing `uv run pytest tests/test_render_widgets.py --run-nightly` is green on this branch (82 passed) and `uv run pytest tests` was green (808 passed, 6 skipped) on the commit the run failed on. `ruff check` and `ruff format` at the pinned `v0.16.1` pass on the changed file. Not vacuous, checked from both sides against the rule in `chrome-style.js`: <details><summary>The two negations</summary> | rule changed to | what fails | | --- | --- | | `… 140ms linear 0ms both` (delay removed) | `the wait was announced before it was one: {1, 0.94, 0.88, …}` — the early half | | selector changed so nothing matches | `a wait worth noticing said nothing: []` — the late half | </details> <details><summary>The frame samples, before and after</summary> Sampled locally under three spinning cores, on the original wall clock (`ms since t0`, opacity): ``` (200.3, 1), (217.0, 1), (223.8, 0.999939), (240.5, 0.940296), … (357.1, 0.523868), (373.7, 0.5), (390.5, 0.5), (407.1, 0.5) ``` The fade runs 223→383. The assertion's threshold is 400, so the margin the reading had was one frame. On the animation's clock the same run reads `currentTime` 0 at the frame the attribute paints in, 200 where the fade starts and 340 where it ends and holds — the same numbers the rule is written with. Worth a maintainer's eye rather than a fix here: about 120ms of main thread between the press and the frame the busy attribute paints in is a lot for `SUGGESTION_PAGE`, which is a small fixture. `#inFlight` calls `#renderControls` and `this.#margin?.update()` after setting the attribute, and the animation cannot start until both have finished. The rule is designed for exactly the heavy-page case, so it still does the right thing for the reader — but the cost is the layer's, and it is not visible from any assertion that reads the rule alone. </details> --- Automated fix for [failed run](https://github.com/max-sixty/leaf/actions/runs/33726571939) Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
## Problem The nightly leg of `ci` failed on [run 33735071949](https://github.com/max-sixty/leaf/actions/runs/33735071949) (`a57ae29`) with seven cases. Six are already owned by open PRs: `test_a_widgets_label_takes_a_comment_inside_the_control_it_labels`, `test_a_closed_leaf_clears_itself_off_the_tray` and `test_the_leaves_tray_takes_the_keyboard` by #236; the ring walk by #239's `ship-review` precondition standing ahead of #240's `lf-shotframe` fault; and both of `test_page_fixture_renders[pr-walkthrough]` (the 4px sideways scroll on `pr-scale-groups`) and `test_nested_command_projections_stop_at_their_own_boundary` (`§ inner-worker`) by #243. The seventh is new with `a57ae29` and nobody's yet: ``` FAILED tests/test_render_conversations.py::test_a_coined_class_cannot_reach_the_chromes_rules AssertionError: the document-level class surface changed: widen the shared vocabulary on purpose Extra items in the left set: 'lf-workspace' ``` #242 wrote the open workspace's tint as `.lf-btn.on, .lf-btn.lf-workspace[aria-expanded="true"]` in `chrome-style.js`, at document level. `lf-workspace` is worn by three controls and all three are the chrome's own — the Threads toggle built at [`leaf.js:817`](https://github.com/max-sixty/leaf/blob/064e67d5/skills/leaf/assets/leaf.js#L817), and All leaves and Asks, which `trayIs` marks at [`trays.js:194`](https://github.com/max-sixty/leaf/blob/064e67d5/skills/leaf/assets/runtime/trays.js#L194). Every one of them is inside `chromeRoot`, the `.lf-chrome` container the private stylesheet is scoped to. So the name reached document level without being shared vocabulary, which is the seam the pinned set exists to catch — the same one #152 answered for the keycap rules. ## Solution Move the selector into the `@scope (.lf-chrome)` block, beside `.lf-banner-more[data-lf-news]`, which paints the same accent face for the same kind of banner state. `.lf-btn.on` keeps its document-level rule, unchanged and now on its own: `on` is worn out in the page and is the reason that line was there before #242. Nothing about the paint moves. Specificity is unchanged, and the one rule that could contend — `.lf-banner-menu > .lf-btn { border-color: transparent }`, for a workspace control folded into the overflow menu — is `(0,2,0)` against this selector's `(0,3,0)` on both sides of the move, so scope proximity never gets a say. Fixing it at the pinned set instead would have widened the shared vocabulary by a class no widget can wear, which is what the assertion's own message asks you not to do. ## Testing On this branch, rebased on `fdba9e1d`: - `uv run pytest tests` — 808 passed, 6 skipped - `test_a_coined_class_cannot_reach_the_chromes_rules` and both parameters of #242's `test_workspaces_replace_each_other_and_name_the_open_one` pass - `uv run pytest tests/test_render_conversations.py tests/test_render_controls.py --run-nightly` — 141 passed, and the three failures are #236's two tray cases and the ring walk, all reproducing identically on `main` Not vacuous: deleting the scoped rule fails the workspace test at both 500 and 1200 with the resting face (`rgb(238, 234, 222)` where the active face is `rgb(255, 254, 251)` under `--chip`) rather than passing by construction. `chrome-style.js` is served rather than bundled, so there is nothing to re-vendor, and no user-visible pixel changes, so there is no before/after shot to hand over. --- Automated fix for [failed run](https://github.com/max-sixty/leaf/actions/runs/33735071949) Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
Problem
The nightly leg of
cifailed with seven cases on run 33702158485 (c370784). All seven reproduced locally on every run, so none was the load-shaped race thatrunning-tendwarns about; each was a nightly fixture reading a contract #232 and #233 replaced, or a defect their new surfaces brought with them.Three of the seven are no longer this branch's to answer.
mainreached them from the other side while this PR was open, and the merge below takesmain's version of each:Restore Page map chord grammar and focus) landed the Page map's Escape fix — the sameleavePageMaproute this branch had, under one flag (sheetCloseOwnsFocus) rather than two — and read its two fixtures. It also restoredg m Nas the numbered location prefix andg Mas the complete sheet, reversing the grammar change this branch had adapted its fixtures to. Sotest_render_projection.py::test_escape_lets_go_of_the_ask_the_reader_is_standing_onreverts tomain'sg m 2, which is correct again.test_site.py::test_the_label_is_chrome_rather_than_words_to_quoteby dropping the key-line reading rather than restating it.Solution
What remains is one product fix and six fixture readings.
The sheet's list did not fit in the sheet.
.lf-page-map-listwas capped atcalc(100vh - 150px)while the card it sits in stops atmin(720px, 100vh - 24px). Measured on the corpus at 760×900: the sheet ran172→892and its list270→1020— 128px of the map hanging out of the rounded corner and off the bottom of the window, with its own focus ring drawn outside the card. The open sheet is a column now and the list takes what the head and search box leave: same measurement,270→877.Six fixtures read the layer as it was. #232's return stack means
g Lgives back the reading place rather than the tray's button, and a secondg Linto a standing tray is its own frame — two fixtures intest_render_controls.py. Three sibling widget-label drags intest_render_anchors.pyreadlf-pendingstraight off the mouseup, when the capture is queued behind it; all three now wait for the paint that says the gesture was read. And the ring walk'sRING_NEW_STOPtold "nothing focused" and "stood on this already" apart with one boolean, and the walk ended on either — a scope it joins part-way down its own order keeps its earlier stops on the far side of the tab order's crossing out of the document and back in, so ending there leftpage-map-searchunwalked and unlit. Only a repeat ends a walk now; two empty reads in a row still stop one that never starts.Testing
On the merge commit
91da9d3:uv run pytest tests— 808 passed, 6 skipped.test_the_small_screen_map_is_a_complete_accessible_sheetand the two Page map fixtures the merge takes frommain— all pass under--run-nightly.The ring walk still fails, identically on
main.test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpustrips on a scope precondition —.lf-margin-marker[data-lf-kinds~="comment"] did not open a thread card on ship-review— before the loop this PR rewrites ever runs. Measured on both91da9d3ande71fa110(main's tip at merge time), same assertion, same message. That cause is not this branch's; folding it in would put two unrelated causes through one review.So merging this will not turn the nightly green on its own.
The seven original cases and where each went
test_render_navigation.py::test_the_g_chord_reaches_panels_and_document_listsmain, e71fa11test_render_projection.py::test_escape_lets_go_of_the_ask_the_reader_is_standing_ong m 2— restored by e71fa11, somain's version is correct againmain, e71fa11test_site.py::test_the_label_is_chrome_rather_than_words_to_quotecwith nothing in hand names the pagemain, 991c64ctest_render_controls.py::test_the_leaves_tray_takes_the_keyboardtest_render_controls.py::test_a_closed_leaf_clears_itself_off_the_traytest_render_controls.py::test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus.lf-page-map-search:focus-visible; the walk ended at the tab order's crossing back through the document and never reached itship-reviewprecondition abovetest_render_anchors.py::test_a_widgets_label_takes_a_comment_inside_the_control_it_labelsAutomated fix for failed run