feat: support Plotly SVG scatter point highlighting (#533)

[jooyoungseo]

Enhance scatter trace position extraction to support SVG elements that
use transform translate or cx/cy attributes instead of x/y attributes.
This fixes highlighting for Plotly scatter plots where markers use
transform="translate(x, y)" for positioning.

https://claude.ai/code/session_01G8aSXXDCkdRL7ym57XQueM

[claude]

PR Review: feat: support Plotly SVG scatter point highlighting (#533)

This is a focused, well-scoped fix that cleanly extends scatter highlight support to Plotly SVGs. The extraction of getElementPosition() is a good design decision — it isolates the multi-backend position logic and makes the primary mapToSvgElements easier to read.

---

Issues

1. Behavioral Change: Partial Coordinates Now Excluded

The original code handled x and y independently:

// Old: elements with ONLY x (no y) still went into xGroups
if (!Number.isNaN(x)) { xGroups.get(x)!.push(element); }
if (!Number.isNaN(y)) { yGroups.get(y)!.push(element); }

The new code skips elements entirely if getElementPosition returns null, which happens when neither x/y, cx/cy, nor transform yields a full coordinate pair. Any SVG element that previously had only an x attribute (but no y) would have been added to xGroups before but is silently dropped now.

This is almost certainly the correct behavior (a point without both coordinates is useless for grouping), but worth confirming no real-world matplotlib or Plotly SVGs emit partial attributes that this would silently discard.

2. No Tests for the New Code Path

getElementPosition has three distinct branches (x/y, cx/cy, transform) and zero tests. There is also no E2E spec for scatter plots at all — only bar, line, box, heatmap, and histogram have coverage.

At minimum, unit tests for getElementPosition should cover:

• Element with x/y attributes
• Element with cx/cy attributes
• Element with transform="translate(...)" (comma-separated and space-separated)
• Element with no recognized attributes (should return null)
• Edge values: zero, negative, scientific notation

3. Minor: transform Regex Does Not Handle matrix() Transforms

Plotly occasionally emits transform="matrix(1,0,0,1,tx,ty)" instead of translate(). The current regex silently returns null for such elements, which means those points would not be highlighted. This may be acceptable as a follow-up, but worth documenting as a known gap.

---

Positives

• Clean extraction: getElementPosition is a good single-responsibility helper — well-named, well-documented, and easy to test in isolation.
• Fallback ordering is correct: x/y → cx/cy → transform goes from most specific to most expensive to parse, which is sensible.
• Regex handles all valid SVG separator formats: translate(10 20) (space-only) works because the regex engine backtracks to let [,\s] consume the space rather than \s*. Comma, space, and mixed separators are all covered.
• Refactored mapToSvgElements is cleaner: Removing the duplicated if (!Number.isNaN(...)) guards and consolidating into one pos check is a net readability improvement.

---

Suggestions

1. Add a scatter plot E2E spec (or at minimum, unit tests for getElementPosition).
2. Add a code comment in getElementPosition noting that matrix() transforms are not currently supported, so future contributors know this is a known gap.

---

Overall, the change is correct and solves a real problem. The main ask before merging is test coverage for the new code paths.

[claude]

Follow-up Review: Coordinate System Bug in getPositionFromBoundingBox

The previous review comment noted a few gaps. On closer inspection, I want to correct one point and raise a new, more substantive concern.

---

Correction: matrix() IS handled

The previous review stated that matrix() transforms were not handled. This was incorrect — the code does include a matrix() regex:

const matrixMatch = transform.match(
  /matrix\(\s*[-\d.e+]+[,\s]+[-\d.e+]+[,\s]+[-\d.e+]+[,\s]+[-\d.e+]+[,\s]+([-\d.e+]+)[,\s]+([-\d.e+]+)\s*\)/,
);

This correctly extracts the e and f translation components. Apologies for the inaccuracy.

---

Bug: getPositionFromBoundingBox uses a different coordinate system

The three attribute-based methods (x/y, cx/cy, transform) all return SVG user space coordinates — the logical coordinate system defined by the SVG's viewBox. But getPositionFromBoundingBox uses getBoundingClientRect(), which returns browser viewport pixel coordinates:

const rect = element.getBoundingClientRect();
return {
  x: Math.round(rect.x + rect.width / 2),  // viewport pixels
  y: Math.round(rect.y + rect.height / 2),  // viewport pixels
};

These are completely different coordinate spaces. If an SVG element has no recognized attribute (x/y, cx/cy, transform) and falls through to the bounding box path, its coordinates will be in viewport pixels — potentially hundreds of pixels — while every other element was grouped using SVG user space coordinates (e.g. values 0–1 for a normalized scale). The two groups would never intersect correctly.

A real case: a <path d="M 50 75 ..."> with no transform attribute. It falls through all three attribute checks, lands in the bounding box path, and gets viewport coordinates like (350, 420) instead of SVG user space (50, 75).

Fix: Use element.getBBox() instead, which returns a bounding box in SVG user space:

private getPositionFromBoundingBox(element: SVGElement): { x: number; y: number } | null {
  if (\!(element instanceof SVGGraphicsElement)) {
    return null;
  }
  try {
    const bbox = element.getBBox();
    if (bbox.width > 0 || bbox.height > 0) {
      return {
        x: bbox.x + bbox.width / 2,
        y: bbox.y + bbox.height / 2,
      };
    }
  } catch {
    // getBBox() may throw for elements not in the DOM or with no geometry
  }
  return null;
}

getBBox() returns coordinates in the same SVG user space as x/y, cx/cy, and transform attributes, so grouping remains consistent.

---

Minor: Rounding inconsistency

The bounding box path applies Math.round() but the attribute-based paths do not. This means that if two elements happen to use different detection paths (e.g. one has a transform and another falls through to bounding box), the same logical position could be rounded differently and land in different groups. Using getBBox() in the fix above also removes the need for explicit rounding.

---

Summary

Issue	Severity	Fix
getBoundingClientRect() returns viewport coords, not SVG user space coords	High — can silently produce wrong groupings	Use getBBox() instead
matrix() is handled (previous review was wrong)	✅ No action needed	—
No unit tests for new methods	Medium	Add tests for each branch
Rounding inconsistency	Low — resolves when switching to getBBox()	Addressed by above fix

The translate and matrix regex logic looks correct. The attribute-based priority order is sensible. The main concern is the coordinate space mismatch in the fallback path.

[claude]

PR Review Update: Current State of the Diff

This is a follow-up pass on the current diff after the two previous review comments. Here's an updated assessment.

---

Status of Previously Raised Issues

✅ Coordinate system bug — already resolved in current diff

The second review comment raised a high-severity bug about getBoundingClientRect() returning viewport coordinates. Looking at the actual diff, the code already uses getBBox() (SVG user-space coordinates):

const bbox = (element as SVGGraphicsElement).getBBox();
return {
  x: bbox.x + bbox.width / 2,
  y: bbox.y + bbox.height / 2,
};

This is correct — consistent with the other three extraction paths. No action needed.

⚠️ No tests — still unresolved

Neither unit tests nor E2E specs exist for the three new methods. This remains the main concern before merging.

---

New Observations

1. Prefer instanceof check over as cast in getPositionFromBoundingBox

// Current
const bbox = (element as SVGGraphicsElement).getBBox();

The as cast is a compile-time assertion — it doesn't verify anything at runtime. If element is not actually an SVGGraphicsElement, calling getBBox() throws (caught by the outer try/catch, so not a crash, but the error is silently swallowed). An explicit check makes the intent clear:

private getPositionFromBoundingBox(element: SVGElement): { x: number; y: number } | null {
  if (\!(element instanceof SVGGraphicsElement)) {
    return null;
  }
  try {
    const bbox = element.getBBox();
    if (bbox.width > 0 || bbox.height > 0) {
      return { x: bbox.x + bbox.width / 2, y: bbox.y + bbox.height / 2 };
    }
  } catch {
    // getBBox() may throw for off-screen elements or those with no geometry
  }
  return null;
}

In practice the cast is safe (all CSS-selectable visible elements extend SVGGraphicsElement), but the pattern is still cleaner with the guard.

2. Regex character class [-\d.e+]+ accepts invalid numeric strings

The number pattern used across all three regexes:

/translate\(\s*([-\d.e+]+)[,\s]+([-\d.e+]+)\s*\)/

The class [-\d.e+]+ is more permissive than a valid float — it would match strings like ---, ..., or e+++. Number.parseFloat will return NaN for these, and the \!Number.isNaN guard catches it, so there is no incorrect result. But if a malformed transform attribute is encountered, the code silently falls through to the getBBox() path rather than the regex match failing cleanly. This is low-severity but worth noting. A tighter pattern like:

[-+]?(?:\d+\.?\d*|\.\d+)(?:[eE][-+]?\d+)?

would only match valid floating-point literals, though it's more complex.

3. Single-argument translate(x) with y=0 assumption

// Try translate(x) — single argument, y defaults to 0
const translateSingleMatch = transform.match(
  /translate\(\s*([-\d.e+]+)\s*\)/,
);
if (translateSingleMatch) {
  return { x: tx, y: 0 };
}

The SVG spec says translate(x) is equivalent to translate(x, 0), so defaulting y to 0 is correct. The JSDoc also documents this. No issue — just confirming this is spec-compliant.

4. Priority ordering and cumulative transforms

The method returns the first successful strategy, which is correct. One subtle case: an SVG element with both an x attribute AND a transform attribute (e.g., <use x="10" y="20" transform="translate(5,5)">) would have its final rendered position at (15, 25), but getElementPosition would return (10, 20) from the x/y path and ignore the transform. This is an inherent limitation of single-attribute extraction (it was the same before this PR) and is likely acceptable given real-world SVG structures, but worth documenting as a known limitation.

---

Summary

Item	Status
Coordinate system bug (getBBox vs getBoundingClientRect)	✅ Already fixed in current diff
matrix() transform support	✅ Handled correctly
instanceof vs as cast in getPositionFromBoundingBox	⚠️ Minor — consider fixing
Regex permissiveness	ℹ️ Low-severity, no incorrect behavior
No unit tests for new methods	⚠️ Medium — main ask before merge
No E2E spec for scatter plots	ℹ️ Nice-to-have follow-up

The logic is sound and the fix addresses a real gap in Plotly SVG support. The instanceof guard is a minor improvement worth making. Unit tests for getElementPosition (covering x/y, cx/cy, translate, matrix, and null cases) remain the primary ask before this is merge-ready.

[nk1408]

Will re-create new pr. There are quite a few new regeressions

cc: @jooyoungseo