fix: support per-stat highlighting for Plotly single-path box plots (#535)

[jooyoungseo]

Plotly renders each box plot as a single element containing
whiskers, IQ box, and median line, whereas MAIDR expects separate SVG
elements for each statistical component. This caused within-box stat
navigation and per-stat visual highlighting to fail for Plotly box plots.

Add detection for single-path box plots (where all BoxSelector fields
resolve to the same DOM element) and create synthetic SVG line overlays
at data-driven positions using bounding box coordinate mapping. The
existing multi-element code path (matplotlib, etc.) is preserved and
only activates when 2+ non-null selectors resolve to different elements.

Changes:

• Add isSinglePathBox() detection method to BoxTrace
• Add createDataDrivenOverlays() for data-to-SVG coordinate mapping
• Add Svg.createPositionedLineElement() utility for positioned overlays
• Add Plotly-style vertical box plot example

https://claude.ai/code/session_012wiRFDsjMCepsrpfVaPJza

[claude]

PR Review: fix: support per-stat highlighting for Plotly single-path box plots

Summary: The approach is well-reasoned and creative. Detecting single-path boxes and synthesizing overlay elements via data-driven coordinate mapping is a sound strategy for the Plotly case. Below are issues ranging from bugs to suggestions.

---

Bugs / Correctness Issues

1. Coordinate mapping assumes bbox fully spans data range (may misfire)

createDataDrivenOverlays maps data values linearly onto the bounding box of the single <path>:

const y = bbox.y + ((dataMax - value) / dataRange) * bbox.height;

This assumes the path's bounding box exactly spans from point.min to point.max with no padding. In the example SVG this holds, but if Plotly (or another renderer using single-path boxes) adds any internal padding, the overlays will be mispositioned. At minimum, a comment documenting this assumption explicitly would prevent future confusion.

2. dataRange === 0 falls back incorrectly when q1/q2/q3 vary

const dataRange = dataMax - dataMin;

if (dataRange === 0) {
  return [lowerOutliers, createEmptyElement, ...];
}

If point.min === point.max (a degenerate box) but the statistical internals (q1, q2, q3) are non-trivially different, every stat gets an empty line element. A more robust fallback might use a small epsilon or fall back to placing all lines at the single-value coordinate.

3. Multiple insertAdjacentElement('afterend', ...) on same reference reverses insertion order

Inside createPositionedLineElement, the line is always inserted immediately after referenceElement:

referenceElement.insertAdjacentElement(Constant.AFTER_END, line);

When called 5 times for the same reference element (min, q1, q2, q3, max), each insertion goes between the reference and the previously inserted sibling. The DOM order ends up reversed: [ref, max_line, q3_line, q2_line, q1_line, min_line]. For visibility toggling this is harmless since each element is addressed directly, but it is unexpected and could confuse debugging.

---

Architecture / Design

4. isSinglePathBox with exactly one non-null selector

if (elements.length <= 1) {
  return false;
}

If only one selector field resolves (e.g., only iq is non-null), the method returns false and falls into the standard path. This is the right default, but it means a Plotly box that only specifies one of the four fields will silently skip synthetic overlays. Worth a code comment explaining the intent.

5. isSinglePathBox could have false positives

If a chart author deliberately points two different fields at the same element (to highlight both stats together), the detection would treat it as a single-path box and replace highlighting with synthetic overlays. This is unlikely but worth noting since there is no explicit user signal—the detection is purely structural.

6. window.getComputedStyle called at construction time

createPositionedLineElement calls window.getComputedStyle(referenceElement) during the trace constructor. If the SVG element is not yet attached to a rendered layout (e.g., hidden container), style.stroke will be an empty string, producing '' for the stroke rather than the '#000000' fallback, because:

line.setAttribute(Constant.STROKE, style.stroke || '#000000');

style.stroke returns '' (falsy) on unrendered elements, so the fallback does kick in here—which is a good guard. However, the existing createLineElement doesn't have this fallback:

line.setAttribute(Constant.STROKE, style.stroke);  // no fallback

Consider applying the same || '#000000' guard to createLineElement for consistency (or extract to a shared helper).

---

Test Coverage

7. No unit or E2E tests for the new code paths

isSinglePathBox and createDataDrivenOverlays contain non-trivial logic (element identity comparison, coordinate math). Both are good candidates for unit tests:

• isSinglePathBox: test with all same, all different, mix of null, and exactly-one-non-null inputs
• createDataDrivenOverlays: test coordinate math for vertical/horizontal, degenerate range

A Playwright E2E test for examples/boxplot-plotly-vertical.html analogous to boxplotVertical.spec.ts would verify the full user-facing highlighting flow works end-to-end.

---

Minor / Nitpick

8. createPositionedLineElement vs createLineElement — near-duplication

Both methods share ~10 lines of identical setup (namespace, x1/y1/x2/y2 setAttribute, stroke, stroke-width, visibility, insertAdjacentElement). A private shared helper accepting coordinates and reference element would reduce duplication and make the fallback fix in point 6 easier to apply consistently.

9. Version bump in package-lock.json

The lock file bumps version 3.50.0 → 3.51.0. If versioning is handled through a separate release workflow, this should be consistent with that process. No issue if this is intentional.

---

What's Working Well

• The two-phase approach (collect originals first, then clone/create) already in the codebase is correctly preserved for the Plotly path.
• Using point from this.layer.data (original, unreversed) while selectors are also in original order is correct and the comment confirms the intent.
• The fallback when referenceElement is null (all-empty sections) is safe.
• The getBBox() + sibling insertion keeps coordinate spaces consistent since both the reference and new lines share the same parent <g>.

Overall the approach is sound for the described Plotly rendering model. Addressing the coordinate-assumption documentation, the reversed-insertion order, and adding at least basic tests for the new logic would make this production-ready.

[claude]

PR Review: fix: support per-stat highlighting for Plotly single-path box plots (#535)

Thanks for a well-documented fix for a real accessibility gap. The core idea — detecting Plotly's single-path rendering and synthesizing positional overlays — is practical. Here are my observations:

---

Architecture / Design

Mild MVVC boundary concern in BoxTrace

createDataDrivenOverlays calls svg.getBBox() inside /src/model/box.ts. Per CLAUDE.md, the Model layer should contain "ONLY data and navigation logic — NO UI concerns." DOM queries belong in a service or utility. This is consistent with pre-existing DOM touches in box.ts, but the new method pushes further by querying layout geometry. Consider moving DOM-coordinate logic into a Svg utility method, or at least noting it as a known trade-off.

---

Potential Bugs

1. getBBox() assumes bounding box spans exactly min→max

createDataDrivenOverlays maps data values to SVG coordinates by treating bbox.x / bbox.x+bbox.width (horizontal) or bbox.y / bbox.y+bbox.height (vertical) as the SVG positions of dataMin and dataMax.

This works for the examples in this PR, but it could silently misplace highlights if a chart's path uses different coordinate extents (e.g., an axis that doesn't start at the whisker, or a padding offset). A comment explaining the assumption would help future maintainers:

// Assumes the bounding box of the single-path element spans from
// the SVG coordinate of dataMin to dataMax (i.e. whisker ends are
// the leftmost/rightmost or topmost/bottommost rendered pixels).

2. isSinglePathBox can return false on partial DOM failures

If exactly one of {min, max, iq, q2} resolves to a non-null element (e.g., one selector fails), elements.length <= 1 causes an early return false. This falls through to the multi-element code path with a mix of missing selectors, which may render incorrectly without any warning. Consider logging a warning for the partial-match case.

3. Redundant null-guard after isSinglePathBox

When isSinglePathBox returns true, the function has already verified at least 2 non-null elements, so referenceElement from original.iq ?? original.min ?? ... will always be non-null. The if (referenceElement) check is dead code. Not a bug, but misleading.

---

Test Coverage

No tests are added for the new code paths:

• isSinglePathBox detection (true/false cases)
• createDataDrivenOverlays coordinate mapping
• Svg.createPositionedLineElement

The new example HTML files are great for manual verification, but unit tests for the detection/mapping logic and an E2E test using those examples would significantly reduce regression risk for future box plot changes.

---

Code Quality

Svg.createPositionedLineElement refactor is clean

Extracting the shared line-creation logic and making it public static is a good, minimal refactor. The || '#000000' stroke fallback is good defensive coding.

dataRange === 0 guard is appreciated — returning empty elements for degenerate input is the right call.

Inline createLineAtValue closure

The closure is clear and readable, but point.min and point.max are also dataMin/dataMax. Calling createLineAtValue(point.min) and createLineAtValue(point.max) should produce lines exactly at the bounding box edges (y = bbox.y + 0 and y = bbox.y + bbox.height). This is mathematically correct, but a brief comment would help:

// min/max map to the bbox edges; q1..q3 are interpolated between them

---

Minor

• In both new example HTML files, <link rel="stylesheet"> is placed inside <body> rather than <head>. Technically valid, but unconventional and may cause a flash of unstyled content.

---

Summary

Area	Verdict
Core algorithm	✅ Correct for Plotly-style single-path boxes
Existing code path preserved	✅ Unchanged multi-element path
Architecture fit	⚠️ DOM geometry in Model layer (consistent with existing pattern but worth noting)
Robustness	⚠️ Silent fallback on partial selector failure; bounding-box assumption undocumented
Test coverage	❌ No new tests for new logic
Code clarity	✅ Well-commented, readable

Recommended before merge: Add at least one unit test for isSinglePathBox and one for the coordinate mapping, and document the bounding-box-alignment assumption in createDataDrivenOverlays.

[claude]

Code Review

Thanks for tackling the Plotly single-path box plot issue — this is a meaningful accessibility improvement. Here are my findings:

---

Bugs / Correctness Issues

1. getBBox() can fail on hidden or detached elements (medium severity)

In createDataDrivenOverlays() (src/model/box.ts), getBBox() is called on the reference element unconditionally:

const bbox = svg.getBBox();

getBBox() throws in Firefox when the element has display:none or is detached, and silently returns {x:0, y:0, width:0, height:0} in Chromium. There's no guard for the zero-bbox case — if the element hasn't been laid out yet, all overlays will stack at the SVG origin. Recommend adding a guard:

const bbox = svg.getBBox();
if (bbox.width === 0 && bbox.height === 0) {
  return [lowerOutliers, ...Array(5).fill(Svg.createEmptyElement('line')), upperOutliers];
}

Or wrapping in a try/catch.

2. package.json version not bumped

package-lock.json shows a version bump from 3.50.0 → 3.51.0, but package.json is not in the changed files. These should be in sync — please run npm version minor (or equivalent) so both files are updated together.

---

Design / Architecture

3. Fragile non-null assertion after isSinglePathBox

const referenceElement = (original.iq ?? original.min ?? original.max ?? original.q2)\!;

The \! is safe today because isSinglePathBox guarantees ≥ 2 non-null elements, but this contract is implicit. Consider documenting the invariant in a comment, or extracting the non-null element inside isSinglePathBox and returning it (e.g., returning SVGElement | false) so the guarantee is encoded in the type system.

4. Coordinate mapping assumption is fragile for stroked paths

From the JSDoc:

Assumption: The bounding box of the single-path element spans exactly from the data min to the data max.

getBBox() returns the geometric bounding box (excluding stroke width). For Plotly paths with stroke-width: 1.5, the visible whisker ends extend ±0.75px beyond the returned bbox, so overlays at min/max will be slightly inset relative to the visible whisker caps. This is probably acceptable visually but worth noting in the JSDoc as a known limitation.

---

Code Quality

5. console.warn in production code

isSinglePathBox() emits a console.warn for an edge case (exactly 1 selector resolves). Per the debugging guide, production code should prefer conditional logging or a logger utility. Consider removing or gating it behind a debug flag.

6. No unit tests for new methods

isSinglePathBox() and createDataDrivenOverlays() contain non-trivial logic (element identity checks, coordinate math for both orientations, dataRange === 0 edge case). These would benefit from unit tests. The existing test infrastructure should make this straightforward.

---

Positive Notes

• The detection logic in isSinglePathBox() is clean and correctly handles 0, 1, and N-element cases.
• Extracting createPositionedLineElement as a reusable public static method is the right refactoring — avoids duplication and follows existing patterns.
• The existing multi-element code path (matplotlib) is completely untouched, so there's no regression risk for existing users.
• The coordinate math for both orientations is correct: vertical uses (dataMax - value) / dataRange (SVG y-down), horizontal uses (value - dataMin) / dataRange.
• The dataRange === 0 guard is good defensive coding.
• The stroke fallback style.stroke || '#000000' prevents invisible overlays on elements with empty computed stroke.
• JSDoc comments clearly explain the design rationale and assumptions.

---

Summary

The approach is sound and the implementation is well-structured. The main items to address before merging are: the getBBox() zero-bbox edge case (#1), syncing package.json with package-lock.json (#2), and adding test coverage for the new methods (#6). The rest are lower priority suggestions.