fix(xychart): prevent flipped point label from clipping x-axis#7552
fix(xychart): prevent flipped point label from clipping x-axis#7552DominicBurkart wants to merge 19 commits into
Conversation
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
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
🦋 Changeset detectedLatest commit: 5b183bf 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 #7552 +/- ##
==========================================
- Coverage 3.34% 3.32% -0.03%
==========================================
Files 524 535 +11
Lines 55256 56456 +1200
Branches 795 820 +25
==========================================
+ Hits 1850 1876 +26
- Misses 53406 54580 +1174
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…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
When shouldFlipLabelVertical() recommends placing a label below its data point (to avoid line collision), check that the flipped position stays within yAxis.getRange() — the inner boundary of the plot area. If the flipped position would collide with the x-axis, keep the label above instead. Exposes getRange() on the Axis interface (already implemented on BaseAxis) so linePlot.ts can query the plot boundary.
DominicBurkart
force-pushed
the
fix/7549_xychart-label-axis-clipping
branch
from
9cbc71e to
fc44f99
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Hey @DominicBurkart — this is a really well-crafted feature contribution! The stacked PR structure is appreciated, the collision avoidance algorithm is solid, and the test coverage is thorough. Let's work through a couple of items to get this across the finish line.
Note: This review covers the full stacked diff (#7550 + #7551 + #7552). Since #7550 and #7551 need to merge first, the items below apply across the stack.
File Triage
| Tier | Count | Files |
|---|---|---|
| Tier 2 (diff + context) | 7 | axis/index.ts, linePlot.ts, interfaces.ts, xychart.jison, xychart.jison.spec.ts, xychartDb.ts, xyChart.spec.js |
| Tier 3 (diff only) | 5 | 3 changesets, 2 docs files |
| Tier 3 (minor) | 3 | .cspell, .gitignore, demos/xychart.html |
What's working well
🎉 [praise] The collision avoidance algorithm in linePlot.ts is excellent. The doesSegmentIntersectBox function correctly handles the tricky case where a steep line enters from above and exits below a bounding box without any endpoint or sample falling inside. The bounded retry loop (3 offsets × 2 directions, then fallback) is a clean design that won't degrade on edge cases.
🎉 [praise] Test coverage is comprehensive — 6 new parser unit tests covering labels with spaces, mixed labeled/unlabeled, decimals, and bar data passthrough, plus 8 E2E visual regression tests covering vertical, horizontal, mixed collision, and steep descent scenarios. This is exactly the level of coverage the project needs.
🎉 [praise] Clean interface extension — getRange() was already implemented on BaseAxis and used internally by both BandAxis and LinearAxis. Promoting it to the Axis interface is the right call and adds no risk.
Things to address
🟡 [important] Hardcoded font size and label offset
linePlot.ts:269-270 — fontSize = 12 and labelOffset = 14 are hardcoded constants. The existing xychart config already has labelFontSize on the axis configs, and the renderer already handles configurable font sizes for other text elements.
It would be great to derive label font size from config (or at least from theme variables) rather than hardcoding — this ensures labels scale consistently with the rest of the chart when users customize their config. If there's no suitable config key today, adding one to XYChartConfig (with 12px as default) would be a small addition.
This doesn't need to block the initial merge, but it would be good to address before the feature ships to avoid a follow-up breaking change to label sizing.
🟡 [important] Character width estimation may cause false collision/non-collision
linePlot.ts:4 — CHAR_WIDTH_FACTOR = 0.7 estimates text width as fontSize * labelText.length * 0.7. This works reasonably for monospace but not for proportional fonts (which mermaid uses by default). A label like "WWWW" will be significantly wider than estimated, potentially colliding with the line despite the algorithm saying it's safe. Conversely "iiii" will be narrower, causing unnecessary flips.
The codebase already has TextDimensionCalculatorWithFont (used in textDimensionCalculator.ts, imported by the axis components) which measures actual rendered text dimensions via a temporary SVG element. Using this for label width measurement would give accurate bounding boxes for collision detection. It would require passing the calculator into LinePlot or computing dimensions in the getDrawableElement method.
🟢 [nit] Generated docs file committed
docs/syntax/xyChart.md — This is auto-generated from packages/mermaid/src/docs/syntax/xyChart.md (which is correctly updated). CI/autofix handles regeneration, so this file doesn't need to be in the PR diff. Not a problem — CI will sort it out — but it can be dropped from the commit to keep the diff focused.
🟢 [nit] Bar labels silently dropped
The grammar change to dataPoints applies to both line and bar plot data, so bar [10 "A", 20 "B"] is valid syntax. But setBarData in xychartDb.ts only extracts value and ignores labels. This is fine behavior, but it would be worth noting in the docs that labels are currently line-only, so users don't wonder why their bar labels don't render.
💡 [suggestion] Consider reusing existing text measurement infrastructure
If adopting TextDimensionCalculatorWithFont feels too heavy for this feature, an intermediate approach would be to use a slightly more conservative CHAR_WIDTH_FACTOR (e.g., 0.85) to reduce false negatives with wide characters, while documenting the known limitation. The current 0.7 factor may underestimate by 20-30% for worst-case proportional text.
Security
No XSS or injection issues identified. Label text flows from the JISON parser (STR token) → xychartDb.ts → LinePlotData.pointLabels → TextElem.text → rendered via D3's .text() method (xychartRenderer.ts:242), which safely sets textContent (not innerHTML). The final SVG output is still sanitized by DOMPurify in the standard pipeline.
Self-check
- At least one 🎉 [praise] item exists (3)
- No duplicate comments
- Severity tally: 0 🔴 blocking / 2 🟡 important / 2 🟢 nit / 1 💡 suggestion / 3 🎉 praise
- Verdict matches criteria: COMMENT (2 🟡, no 🔴)
- Not a draft — COMMENT is appropriate given no blocking issues
- No inline comments used
- Tone check: collaborative and appreciative ✓
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-axis-clipping
branch
from
8b03b9b to
0eb5849
Compare
DominicBurkart
force-pushed
the
fix/7549_xychart-label-axis-clipping
branch
from
63d19c0 to
4f7a521
Compare
|
❌ Lockfile Validation Failed The following issue(s) were detected: Please address these and push an update. Posted automatically by GitHub Actions |
When shouldFlipLabelVertical() recommends placing a label below its data point (to avoid line collision), check that the flipped position stays within yAxis.getRange() — the inner boundary of the plot area. If the flipped position would collide with the x-axis, keep the label above instead. Exposes getRange() on the Axis interface (already implemented on BaseAxis) so linePlot.ts can query the plot boundary.
DominicBurkart
force-pushed
the
fix/7549_xychart-label-axis-clipping
branch
3 times, most recently
from
7718e0e to
0f27fec
Compare
…collision bounds Replace the CHAR_WIDTH_FACTOR = 0.7 width approximation with measured text dimensions via TextDimensionCalculatorWithFont. This matches what the renderer actually draws, so collision detection correctly flips labels whose real rendered width would overlap an adjacent line segment. BasePlot now owns a TextDimensionCalculatorWithFont instance (constructed from tmpSVGGroup) and passes it into LinePlot, which calls getMaxDimension per label. Placement helpers take a Dimension instead of reconstructing width from a char-count heuristic.
DominicBurkart
force-pushed
the
fix/7549_xychart-label-axis-clipping
branch
from
8f216b2 to
0ea8d7b
Compare
…ping' into fix/7549_xychart-label-axis-clipping # Conflicts: # docs/syntax/xyChart.md # packages/mermaid/src/diagrams/xychart/chartBuilder/components/plot/linePlot.ts # packages/mermaid/src/diagrams/xychart/xychartDb.ts # packages/mermaid/src/docs/syntax/xyChart.md
3 participants
📑 Summary
When a label is auto-flipped to avoid line collision, it can clip into the axis area if the data point is near a plot boundary. This adds full bounds checking to both placement functions, covering all four plot boundaries for both chart orientations.
Stacked on #7550 and #7551. Please merge those first.
[existing] top placement (default):
[existing] bottom placement (avoid intersecting with the line graph at acute angles):
[new] forces intersection with line instead of intersecting with the x axis:
📏 Design Decisions
getRange()on theAxisinterface — was already implemented onBaseAxisbut not part of the public interface. Provides the inner plot boundaries needed for bounds checks.computeLabelPlacementVerticalandcomputeLabelPlacementHorizontalnow accept aPlotBoundsparameter. A candidate position is rejected if it clips any axis boundary, not just the x-axis bottom.Coverage
Changes
axis/index.tsgetRange()toAxisinterfacelinePlot.tsPlotBoundsinterface; integrate bounds checks intocomputeLabelPlacementVerticalandcomputeLabelPlacementHorizontaldemos/xychart.html.gitignoredemos/mermaid.esm.mjsbuild artifact📋 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:.