fix(xychart): auto-flip point labels to avoid line collision#7551
fix(xychart): auto-flip point labels to avoid line collision#7551DominicBurkart wants to merge 9 commits intomermaid-js:developfrom
Conversation
🦋 Changeset detectedLatest commit: d1d7946 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
Type: Bug / Error
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7551 +/- ##
==========================================
- Coverage 3.34% 3.32% -0.03%
==========================================
Files 524 536 +12
Lines 55256 56430 +1174
Branches 795 820 +25
==========================================
+ Hits 1850 1876 +26
- Misses 53406 54554 +1148
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
knsv
left a comment
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this work, @DominicBurkart — per-point labels with collision avoidance is a really thoughtful feature addition. The QA screenshots on #7550 are excellent, and the stacked PR model (#7550 → #7551 → #7552) is a clean way to decompose this. Let's get these across the finish line.
Note: This PR is stacked on #7550 — the diff here includes changes from both PRs. I'm reviewing the combined changeset.
What's working well
🎉 [praise] The collision detection algorithm in linePlot.ts is well-designed. The approach — compute the line segment's y-range within the label's x-range, then check for overlap — correctly handles the case where a steep line passes through the label bounding box even when neither endpoint is inside it. The incremental offset retry (1x, 2x, 3x) is a practical fallback strategy.
🎉 [praise] Excellent test coverage. 6 new parser unit tests covering labeled points, mixed labels, labels with spaces, decimal values with labels, and bar data with labels. 8 new e2e visual regression tests covering basic labels, mixed labels, horizontal orientation, multiple lines, and all three collision scenarios (steep descent, gentle slope, zigzag). This is exactly the level of coverage we want.
🎉 [praise] Clean grammar extension — dataPoint rule returning { value, label } objects is a backwards-compatible change that naturally supports mixed labeled/unlabeled points. Existing unlabeled syntax continues to work via the label: '' default.
🎉 [praise] Two appropriate changesets: minor for the feature, patch for the collision fix. Documentation uses MERMAID_RELEASE_VERSION placeholder correctly.
🎉 [praise] The separation of computeLabelPlacementVertical and computeLabelPlacementHorizontal as pure functions with clear interfaces (LabelPlacement) makes the collision logic testable in isolation if needed later.
Things to address
🟡 [important] Labels not sanitized at DB layer. In xychartDb.ts, axis titles and band categories are run through textSanitizer() (which calls sanitizeText()), but point labels are stored as raw strings from the parser. While D3's .text() method (used at xychartRenderer.ts:242) is safe (it uses textContent, not innerHTML), and DOMPurify sanitizes the final SVG output, the inconsistency means labels and axis titles could render differently for the same input (e.g., markdown formatting, entity handling). It would be great to sanitize labels the same way for consistency:
// In setLineData, after extracting labels:
const labels = data.map((d) => textSanitizer(d.label));→ xychartDb.ts:164-165
🟡 [important] Hardcoded font size and offset. The label font size (12px) and offset (14px) are hardcoded constants in linePlot.ts:247-248. Other xychart text elements derive their font sizes from the chart config (e.g., xAxis.labelFontSize, titleFontSize). Worth considering making these configurable via XYChartConfig — even if that's deferred to a follow-up, it would be good to at least use the theme's data label font size rather than a magic number. The docs also call out the 12px hardcoding as a note, which is transparent, but users may expect consistency with other text sizes.
→ linePlot.ts:247-248
🟢 [nit] Generated docs edited directly. docs/syntax/xyChart.md is in the auto-generated directory — only packages/mermaid/src/docs/syntax/xyChart.md should be edited. The build process (pnpm --filter mermaid docs:build) will regenerate the top-level docs/. The manual edit won't cause issues but will be overwritten.
🟢 [nit] Bar data silently accepts labels. The shared dataPoint grammar rule means bar [10 "A", 20 "B"] parses successfully but labels are ignored (bar rendering doesn't use pointLabels). This is harmless but could confuse users who expect labels on bars too. Worth either documenting this limitation or parsing bar data without the label option.
🟢 [nit] CHAR_WIDTH_FACTOR = 0.7 is a rough heuristic for estimating text width. It's adequate for collision detection (where false positives just mean slightly more flipping), but it won't be accurate for all fonts, proportional characters, or CJK text. A comment noting this approximation would be helpful for future maintainers.
💡 [suggestion] The collision detection functions (lineYAtX, doesSegmentIntersectBox, etc.) are pure geometry utilities with no xychart dependency. If other diagram types ever need similar collision avoidance, these could be extracted to a shared utility. No action needed now — just noting the opportunity.
Security
No XSS or injection issues identified. Labels flow through D3's .text() method (which uses textContent, not HTML parsing) and are further protected by DOMPurify sanitization on the final SVG output. The 🟡 above about sanitizeText consistency is a defense-in-depth recommendation, not an exploitable vulnerability.
Summary
| Severity | Count |
|---|---|
| 🔴 blocking | 0 |
| 🟡 important | 2 |
| 🟢 nit | 3 |
| 💡 suggestion | 1 |
| 🎉 praise | 5 |
Self-check
- At least one 🎉 praise item
- No duplicate comments
- Severity tally: 0 🔴 / 2 🟡 / 3 🟢 / 1 💡 / 5 🎉
- Verdict matches criteria: COMMENT (2 🟡, no 🔴)
- No inline comments used
- Tone check: collaborative and constructive
Allow annotating individual data points on xychart line plots with custom text labels. The syntax extends the existing data array format: line [540 "PaLM", 65 "LLaMA", 34 "Llama 2", 7 "Mistral"] Each number can optionally be followed by a quoted string label. Labels are rendered above (vertical) or to the right (horizontal) of the data point, using the line's stroke color. Backward-compatible: existing syntax without labels works unchanged. Addresses mermaid-js#5326. https://claude.ai/code/session_01CgvREhkQ3tVYCagq1mHmjV
Document the new per-point text label syntax for line charts, including syntax examples and rendering behavior notes. https://claude.ai/code/session_01WErEVQAYaNmC3rpbevFwMg
…labels - Add minor changeset for the new point labels feature - Enhance xyChart.md docs with mermaid-example block for mixed labels and a note about fixed font size - Add 4 Cypress e2e snapshot tests covering: all-labeled, mixed-labels, horizontal orientation, and multi-line scenarios - Add MMLU to cspell dictionary https://claude.ai/code/session_011g3A6y2kdJnZh8YB4uJaxD
- Pass point labels through textSanitizer() in setLineData, matching the defense-in-depth pattern already used for axis titles/bands - Add note to docs that labels apply to line plots only; bar syntax is accepted but labels are currently ignored
When a line segment has a steep slope, labels placed above a data point can collide with the line. This adds collision detection that estimates each label's bounding box and checks for intersection with adjacent line segments using y-range overlap. When a collision is detected, the label is flipped to the opposite side of the point (below for vertical charts, left for horizontal charts). https://claude.ai/code/session_01UCV1QCaRsRtQjRspaVrqUp
…padding - Replace shouldFlipLabelVertical/Horizontal with computeLabelPlacement functions that verify both above and below positions before deciding where to place each point label - Try increasing offsets (1x, 2x, 3x baseOffset) if both positions collide at the default offset — handles zigzag patterns - Expand collision bounding boxes by strokeWidth/2 to account for the visual width of the rendered line - Increase default labelOffset from 10 to 14 for more breathing room - Add reviewer's exact failing example to e2e tests and demo page
DominicBurkart
force-pushed
the
fix/7549_xychart-label-line-collision
branch
from
b0f93a3 to
ca720b9
Compare
3 participants
📑 Summary
When a line chart has steep slopes, per-point text labels placed above a data point collide with the line segment. This adds collision detection that auto-flips the label to the opposite side of the point when an intersection is detected.
Stacked on #7550 — please merge #7550 first.
📋 Tasks
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.