Skip to content

feat: support plotly plots#577

Merged
nk1408 merged 2 commits intomainfrom
fix/plotly-highlight
Mar 13, 2026
Merged

feat: support plotly plots#577
nk1408 merged 2 commits intomainfrom
fix/plotly-highlight

Conversation

@nk1408
Copy link
Collaborator

@nk1408 nk1408 commented Mar 12, 2026

Pull Request

Description

  • Add Plotly chart support for MAIDR highlighting across all trace types
  • Plotly renders SVG differently from matplotlib/seaborn (different element types, attributes, and DOM structures), so each trace type's SVG-to-data mapping is adapted to handle both renderers
  • A new Plotly adapter normalizes the DOM before MAIDR initialization, keeping renderer-specific logic isolated from core navigation and accessibility features

Changes Made

src/adapter/plotly.ts New adapter that detects Plotly plots and normalizes DOM structure (subplot wrapping, CSS layout, modebar, focus delegation)

src/index.tsx Call Plotly adapter before initMaidr() when a Plotly plot is detected

src/model/scatter.ts Parse transform="translate(x, y)" as fallback when x/y attributes are absent (Plotly uses with transform, not with x/y)

src/model/bar.ts Align SVG elements to data points when Plotly omits zero-height bars from the DOM; insert invisible placeholders for missing bars

src/model/box.ts Support direct q1/q3 CSS selectors for Plotly, which renders Q1/Q3 as separate elements instead of deriving them from IQR rect edges

src/model/heatmap.ts Detect Plotly's single heatmap and generate transparent overlay elements so individual cells can be highlighted

src/model/line.ts Accept comma-separated path coordinates; interpolate highlight positions when Plotly simplifies paths by removing collinear points

src/model/segmented.ts Make zero-value skipping conditional — only skip when DOM count < data count (Plotly histograms), map 1:1 when counts match (Plotly stacked bars)

src/type/grammar.ts Add optional q1 and q3 fields to BoxSelector interface

src/util/svg.ts Use appendChild() instead of insertAdjacentElement('afterend') for correct DOM ordering in Plotly SVG

Checklist

  • I have read the Contributor Guidelines.
  • I have performed a self-review of my own code and ensured it follows the project's coding standards.
  • I have tested the changes locally following ManualTestingProcess.md, and all tests related to this pull request pass.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation, if applicable.
  • I have added appropriate unit tests, if applicable.

@nk1408 nk1408 requested a review from jooyoungseo March 12, 2026 03:38
@claude
Copy link

claude bot commented Mar 12, 2026

test

@claude
Copy link

claude bot commented Mar 12, 2026

Code Review for PR 577: feat: support plotly plots

CRITICAL ISSUES

  1. src/util/svg.ts - createCircleElement DOM change affects ALL chart types: The diff changes insertAdjacentElement(afterend) to appendChild(). These have different semantics - afterend inserts the element as a sibling after parent.parentElement, while appendChild inserts it as a last child inside parent.parentElement. Since createCircleElement is used by all chart types, this silently changes DOM structure for existing matplotlib/seaborn scatter charts. A regression test or explanation is needed.

  2. src/adapter/plotly.ts - MutationObservers never disconnected (memory leak): Both setupStrokeMirror and setupLayoutObserver create MutationObserver instances that are never stored or disconnected. MAIDR uses the Disposable pattern precisely for this. When Controller.dispose() fires on focusout, these observers keep running, accumulating on every focus/blur cycle. normalizePlotlySvg should return a cleanup function or integrate with Disposable.

  3. src/index.tsx - Unsafe double type cast: normalizePlotlySvg(plot as unknown as SVGSVGElement, maidr) - isPlotlyPlot returns true when plot.closest('.plotly-graph-div') !== null, so plot could be any HTMLElement, not an SVGSVGElement. The double cast bypasses TypeScript safety and risks a runtime error. Consider widening the parameter type or querying for svg.main-svg inside plot before passing it.

HIGH SEVERITY

  1. src/adapter/plotly.ts - Global document.querySelector won't scale to multiple charts: fixModebarTabOrder, setupLayoutObserver, and setupClickToFocus all query globally. With two Plotly charts on a page, only the first modebar gets fixed and the layout observer targets the wrong react-container. Scope all queries to the chart container.

  2. src/adapter/plotly.ts - Infinite requestAnimationFrame loop: In setupLayoutObserver, if MAIDR fails to create the article element, observe() calls requestAnimationFrame(observe) indefinitely. Add a max retry count or timeout.

  3. src/adapter/plotly.ts - Accessibility regression (outline: none !important): The injected CSS removes focus outline on the MAIDR focusable div. MAIDR is an accessibility tool - this violates WCAG 2.4.7 Focus Visible. Please use box-shadow or another visible alternative instead of removing the outline.

  4. src/adapter/plotly.ts - Click capture phase intercepts Plotly interactions: setupClickToFocus uses capture=true, firing before Plotly's own handlers. Clicking a data point for a Plotly tooltip will be intercepted and redirected to MAIDR focus. Consider bubble phase or checking click target before stealing focus.

  5. src/model/heatmap.ts - Overlay rects never cleaned up: createOverlayRects appends rect elements with no cleanup. On repeated focus/blur cycles (new Controller each time), the heatmap parent accumulates duplicate rects causing rendering artifacts and stale highlight targets.

MEDIUM SEVERITY

  1. src/model/line.ts - Interpolation assumes monotonically increasing x: Range derivation uses coordinates[0].x as min and coordinates[last].x as max; segment search assumes left-to-right path. Non-monotonic lines will produce wrong highlight positions.

  2. src/model/bar.ts - Zero-value bar assumption is fragile: Alignment logic assumes every zero-value point was omitted by Plotly. Plotly does render zero-height bars in some configs (e.g., with a base offset). A clarifying comment would help prevent future regressions.

  3. src/adapter/plotly.ts - Schema selector parsing fragile: Extracting axes id via sel.match(/id="..."/) is fragile. Single-quoted attributes or different attribute ordering silently fails.

POSITIVE OBSERVATIONS

  • The src/adapter/plotly.ts module is the right architectural choice; renderer-specific logic is cleanly isolated.
  • The regex fix in line.ts ([\s,]+ instead of \s+) correctly handles comma-separated SVG path coordinates and benefits all renderers.
  • The skipZeros conditionality in segmented.ts is a clean improvement.
  • The optional q1/q3 selectors in grammar.ts are backwards-compatible and well-documented.
  • Inline comments throughout the adapter are clear and helpful.

@nk1408 nk1408 merged commit acd6f89 into main Mar 13, 2026
7 checks passed
@nk1408 nk1408 deleted the fix/plotly-highlight branch March 13, 2026 03:26
@xabilitylab
Copy link
Collaborator

🎉 This PR is included in version 3.58.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@xabilitylab xabilitylab added the released For issues/features released label Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released For issues/features released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants