Skip to content

Reveal reaction removal before undoing - #542

Merged
max-sixty merged 4 commits into
mainfrom
codex/reveal-reaction-removal
Sep 10, 2026
Merged

Reveal reaction removal before undoing#542
max-sixty merged 4 commits into
mainfrom
codex/reveal-reaction-removal

Conversation

@max-sixty

@max-sixty max-sixty commented Sep 10, 2026

Copy link
Copy Markdown
Owner

A standing reaction on a passage looked like a persistent receipt but acted as an immediate eraser, making an exploratory press destructive. This changes the first press into a disclosure that preserves the existing neutral emoji face and reveals a separately named negative Remove control; only that control sends the undo. Outside presses and Escape close the disclosure, and keyboard focus follows whichever real or proxied margin control is visible.

The same two-step path remains reachable when a reaction has spilled into Page Map: the map stays open, reveals and focuses Remove, and forwards that action to the source control. Static exports retain the reaction as a non-interactive mark without dangling disclosure attributes.

The feature gallery and public How it works page now explain the interaction, and the generated corpus is refreshed. Browser coverage verifies the unchanged face, non-mutating first press, pointer dismissal, keyboard focus/escape route, Page Map route, static export, and final undo. The complete local suite passes with 1002 tests; eight focused nightly browser cases pass across the responsive gallery widths, and all eight product-page checks pass.

This was written by Codex on behalf of max-sixty

@leaf-agent leaf-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three nightly browser gates that pass at the merge base (992bad90) fail on this head. The mark's margin-element key moved from take-back:<id> to reaction:<id>:open and its accessible name from <token> — take it back to <token> reaction actions, and three tests outside the diff still address it the old way:

  • tests/test_render_margin.py::test_the_feature_gallery_keeps_its_real_actions_reachable — all four widths; the page-map row [data-lf-map-margin-element$=":take-back:<id>"] no longer resolves.
  • tests/test_render_margin.py::test_a_reaction_receipt_keeps_an_unided_selected_blocks_visual_coordinate — filters the cluster on get_by_role("button", name=re.compile(r"^keep — take it back$")), now zero elements.
  • tests/test_render_pages.py::test_a_shipped_log_replays_its_example_state — the corpus sweep finds neither [data-lf-margin-element-key="take-back:<id>"] nor its :proxy, falls into the spill branch, and times out clicking a .lf-margin-spill that isn't there.

All three live in test_render_* modules, so only --run-nightly selects them and this PR's test job does not. The 1002 tests the description names are the everyday selection — uv run pytest tests collects 1002 of 2402 with 1400 nightly deselected — so tests/test_render_reactions.py itself was outside that run too. Selected explicitly, its new coverage does pass; the two test_comment_response_choices_expand_in_place[*-1280] failures I saw in that file reproduce identically at the merge base, so they are not yours.

The reachability one is more than a rename, which is why I'd rather flag it than suggest a test edit. makeSheetAction in page-map.js closes the sheet and then calls control.click(); that press used to withdraw the reaction. It now toggles record.expanded, and the Remove control carrying the undo is not in the sheet the press just closed — on the crowded gallery target, where the mark reached the map by spilling in the first place, the reader sees no result and has to reopen the map to find the Remove row. The same dispatch is a synthetic click, so event.detail is 0 and a pointer press through the map row — or through the margin's own lf-margin-option-proxy, which forwards the same way — takes the keyboard branch and tries to move focus to a control that may not be visible. What the two-step gesture should mean from the page map looks like the decision to settle before the tests get repinned.

Separately, the export path drops the new relation attributes on the floor. render-checks/standalone.js rebuilds each .lf-react-mark as a role="img" span by copying every attribute and then removing only tabindex, data-lf-offer, title, and type. aria-expanded="false" and aria-controls="lf-reaction-remove-<id>" survive that copy, while .lf-react-remove is dropped with the other offers — exporting the panel fixture with one standing reaction gives a mark carrying both attributes and zero .lf-react-remove elements in the file. So every exported page with a standing passage reaction ships aria-expanded on a role="img" and an aria-controls naming an id the file does not contain. Adding those two names to that removal list is the whole fix; I'm happy to push it if you want it in this PR.

Smaller, and a judgment call rather than a defect: the new .lf-react-mark[data-lf-behavior="disclosure"] rule restores the 2px ring, --ink, and lower shadow that margin-elements.js reserves for action — "An action's heavier ring and lower shadow say this press acts now; a disclosure's firmer single ring says it opens context". The mark is the one control on the page that stopped acting on press, and it now wears the family's acts-now face while the neighbours that do act wear the same one. Holding the face steady through the press doesn't require holding the pre-PR face at rest: the disclosure face is equally unchanged by expansion, and the proxy the living margin builds for this mark paints as a plain disclosure regardless, so the two faces already disagree wherever the mark is presented indirectly.

@leaf-agent leaf-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/how-it-works.html still tells readers that the margin token "paints as an emoji in the margin and a faint wash on the words, opens no thread, and comes off with a press on the emoji". That press now opens the disclosure, so the site's own account of the passage reaction describes a page that no longer exists — and docs/ is what https://leaf.page/how-it-works/ publishes, so this is the one reader-facing surface still carrying the old gesture. examples/corpus.html and the feature gallery both moved to the two-step wording; this sentence didn't come with them. I'm happy to push the edit if you'd rather not respin the branch for it.

The three nightly gates the previous review named — both tests/test_render_margin.py cases and test_a_shipped_log_replays_its_example_state — pass at this head, so that finding is closed.

@max-sixty
max-sixty merged commit 62de1ca into main Sep 10, 2026
7 checks passed
@max-sixty
max-sixty deleted the codex/reveal-reaction-removal branch September 10, 2026 07:25
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