fix(visimer): anchor edge-label overlay on the arrow, not a sibling label (PRD-7866) (#3345) - #21
Merged
Merged
Conversation
…abel (PRD-7866) (#3345)
* fix(visimer): anchor edge-label overlay on the arrow, not a sibling label
Adding a label to a freshly-created arrow put the text-input over a
NEIGHBORING edge's label (and hid that neighbor for the duration of
editing) — the input only snapped to its real position on blur, once
Mermaid re-rendered.
Root cause: `editEntityLabel` used
`correlation.edgeLabels.get(id) ?? correlation.edges.get(id)` as the
overlay anchor for edges. A fresh unlabeled edge does have a
`<g class="edgeLabel">` correlated to it — but the group is EMPTY (no
text descendants). `openOverlayEditor` calls `findTextTarget`, which
finds nothing inside the empty group, widens the search to the parent
`.edgeLabels` container, and returns the first text leaf — some other
edge's label. `openOverlayEditor` then reads THAT sibling's rect and
hides THAT sibling's visibility.
Fix (two moving parts):
1. `editEntityLabel` — for an unlabeled edge/transition/relation, prefer
the edge `<path>` over the empty `.edgeLabel` group, so
`findTextTarget` has no siblings-of-empty-group to wander into.
Labeled edges still prefer the rendered label (the natural anchor
for the glyphs). Same fix applied to flowchart edges, state
transitions, class relations, and ER relations — identical code
shape, identical bug.
2. `openOverlayEditor` — when the anchor arrives as an SVG `<path>` with
no text descendants (the case above), skip `findTextTarget` entirely
and anchor at the path's midpoint via `getPointAtLength(len / 2)`
through a new `pathMidpointRect` helper. Also skip the "hide the
original" step, since there IS no existing label to hide — hiding
the path itself would just be spurious. The blur re-render still
places the committed label at Mermaid's real anchor.
Tests: two new jsdom cases in `packages/dom/test/inplace.test.ts` set
up a flowchart with a labeled sibling (`B -->|Yes| C`) alongside an
unlabeled fresh edge (`A --> B`), then trigger `editEntityLabel` on the
fresh one and assert (a) the overlay opens, (b) the sibling label's
`visibility` stays unchanged (pre-fix, it went to `hidden`).
Also: add the four `@codemirror/*` peers to `apps/site` so the site
dev server starts cleanly — pnpm workspace hoisting kept them at the
workspace root only, and Vite's `optimizeDeps.include` couldn't resolve
them.
* chore(visimer): changeset for edge-label overlay-position fix
* fix(visimer): typecheck + review polish on edge-overlay position
- Narrow `SVGPathElement` inline in the `openOverlayEditor` branch so
the `pathMidpointRect` call type-checks. CI's tsc caught this: the
narrowing lived in a separate `const emptyEdgePath = ...` and didn't
carry through the reassignment site.
- Probe `getScreenCTM` (`typeof … === 'function'`) before calling it,
matching the `getTotalLength` guard. jsdom sets neither on the
prototype; calling one before checking would throw a TypeError that
the null-check couldn't see.
- Fix the axis asymmetry: `top: screen.y` was a midpoint but the
consumer applies it as the top-edge with no vertical transform, so
the overlay rendered half its height BELOW the arrow. Shift up by
`height / 2`. Also scale `height` with the live zoom so `lineHeight`
and the (zoom-scaled) `fontSize` stay in the same units.
- Extract the flowchart edge / state transition / class relation / ER
relation "labeled → edgeLabel else path" preference into a private
`edgeAnchor(entityId, hasLabel)` helper — the four call sites carried
the same ternary + same comment. Reasoning now lives in one place.
- Drop the "opens the overlay even when the edge has no rendered label"
test — pre-fix code created the overlay too (widening picked a
neighbor's rect but still produced a `.mw-inplace-editor`), so the
assertion doesn't fail against the bug it claims to pin. The
sibling-visibility test above is the load-bearing regression guard.
* test(visimer): close remaining review threads on edge-overlay fix
- Drop the dead `querySelector('text, tspan, span, p, div') === null`
sub-condition from the empty-edge-path detection. `correlation.edges`
only ever holds bare `<path>` elements per correlate.ts, and Mermaid
`<path>` never carries element children. `instanceof SVGPathElement`
alone identifies the case; comment now documents the correlation
contract so a future reader doesn't reinstate the defensive check.
- Test: Enter on a fresh-edge overlay dispatches setEdgeLabel with the
typed value. Pins that the commit branch fires for the unlabeled-edge
case (where `inlineHidden === null`) — the commit path was previously
only exercised by node-editing tests, where the anchor IS a rendered
label and `inlineHidden` gets set.
- Test: Escape on a fresh-edge overlay closes without touching a
nonexistent hidden element. Pins that the fresh-edge teardown path
doesn't throw when `inlineHidden === null` — the code guards with an
`if (this.inlineHidden)` check but there was no test firing without
it, so a future refactor dropping the guard could silently crash the
editor on Escape from a bare arrow.
GitOrigin-RevId: 3c0812b7148004ea73467add0ab253df4f41ae3e
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
1 participant
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.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.