[charts] Allow customizing shape in scatter charts

[bernardobelchior]

Allow customizing shape in scatter charts by using the marker slot.

This is my first time using slots, so I'm not sure if I'm using them properly and whether the ownerState/additionalProps is well defined.

Still need to write the docs, but at the moment I'm only looking for feedback on the implementation.

[mui-bot]

Deploy preview: https://deploy-preview-16640--material-ui-x.netlify.app/

Updated pages:

• docs/data/charts/scatter/scatter.md

Generated by 🚫 dangerJS against 28dc87c

[codspeed-hq]

CodSpeed Performance Report

Merging #16640 will degrade performances by 22.58%

_{Comparing bernardobelchior:scatter-chart-shape (a76f62c) with master (7746150)}

Summary

❌ 1 regressions
✅ 5 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	ScatterChart with big data amount	382.9 ms	494.6 ms	-22.58%

[JCQuintas]

Left some comments, if there are other places you want input, let me know

[JCQuintas]

We can remove this and merge the props into the ScatterMarkerProps

[bernardobelchior]

This is used in Scatter.types.ts and on ScatterMarkerProps, so it's probably better to keep it to avoid duplication.

I'll remove it if we have only one usage.

[bernardobelchior]

The current implementation seems to work, but it's weird that the legend's marker is different from the chart's. I'll think of way to reuse the marker slot.

[JCQuintas]

The current implementation seems to work, but it's weird that the legend's marker is different from the chart's. I'll think of way to reuse the marker slot.

A lot of the times, the slots are passed to the top component, which means you should be able to pass it around to the legend component to be used as the marker there. Though a lot of parts will probably need to change 😆

[bernardobelchior]

I don't love it that markerSize is just a scale. I think it would be better if it were the width in pixels of a marker, for example.

[bernardobelchior]

CodSpeed Performance Report

Merging #16640 will degrade performances by 20.21%

Comparing bernardobelchior:scatter-chart-shape (a20991f) with master (ca5f2a0)

Summary

❌ 1 regressions ✅ 5 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
❌ ScatterChart with big data amount 383.2 ms 480.3 ms -20.21%

@JCQuintas @alexfauquette is there a procedure to analyze and fix these performance regressions? When do we just acknowledge them on CodSpeed?

When running the benchmarks locally against a real browser (pnpm run test:performance --browser in the test/performance-charts dir with 100 iterations), the results are the following:

master:

 ✓  chromium  tests/ScatterChart.bench.tsx > ScatterChart 1480ms
     name                                    hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · ScatterChart with big data amount  72.9820  11.9000  36.7000  13.7020  13.9000  21.3000  36.7000  36.7000  ±3.77%      100

This PR:

 ✓  chromium  tests/ScatterChart.bench.tsx > ScatterChart 1451ms
     name                                    hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · ScatterChart with big data amount  74.3329  12.0000  34.1000  13.4530  13.7000  18.7000  34.1000  34.1000  ±3.36%      100

---

By the way, I think there's some room for improvement to the way we benchmark. Here's some things I noticed:

1. It seems like we're running against the dev build, rather than the prod build. We can see that in the CodSpeed flame chart since there are function calls like runWithFiberInDEV and jsxDEVImpl;
2. We're calling cleanup() inside the benchmark, so the time it takes for React Testing Library to clean up after itself is impacting the benchmark results;
3. We're benchmarking using JSDOM, which might skew results when compared to a real browser.

I'll see if I find the time to fix these.

[JCQuintas]

1. It seems like we're running against the dev build, rather than the prod build. We can see that in the CodSpeed flame chart since there are function calls like runWithFiberInDEV and jsxDEVImpl;
2. We're calling cleanup() inside the benchmark, so the time it takes for React Testing Library
3. to clean up after itself is impacting the benchmark results;
   We're benchmarking using JSDOM, which might skew results when compared to a real browser.

These are improvements, but they are not necessary, we are checking relative performance, not total. We don't care about the seconds there, only the % change. If the relative performance changes then something is wrong.

We can still add these changes to a different issue though, so we pick them up.

[JCQuintas]

@JCQuintas @alexfauquette is there a procedure to analyze and fix these performance regressions? When do we just acknowledge them on CodSpeed?

We usually acknowledge it when we are ok with the changes. There is no hard rule. @alexfauquette worked on improving the circle marker performance, so he will probably have some points to share

[bernardobelchior]

If the relative performance changes then something is wrong.

Yeah, I agree, but if I can't replicate the 20% difference locally, doesn't that mean that the issue might not be in this PR but rather in how we run our benchmarks?

I think the local benchmarks are more realistic since they rely on a real browser rather than JSDOM.

We can still add these changes to a different issue though, so we pick them up.

Sure, created it here.

[bernardobelchior]

Managed to find the root cause of the failing test.

It is a consequence of two factors:

1. consumeSlots creates a new component on every render if InComponent isn't wrapped in a forwardRef;
2. useInteractionItemProps's onPointerLeave isn't called when a marker is unmounted.

Initially, I didn't wrap InComponent in a forwardRef and that caused a new component to be created on every render. Because of that, onPointerLeave was never called, and so the tooltip remained visible until onPointerEnter was called for another element.

I'll focus on fixing onPointerLeave not being called when a component is unmounted since that is already creating bugs in this implementation.

Next, I'll take a look at how to fix the issue with consumeSlots. I noticed that ChartsLegend's InComponent isn't wrapped in forwardRef, so we're creating a new ChartsLegend component on every render, which is unnecessary.

[bernardobelchior]

@JCQuintas what do you think of this change?

I'm proposing it to fix the issue of creating a new component on render. You can read more about it here.

Not sure if this only works for React 19 since ref is now a normal prop, but if you think the approach makes sense then I'll test it across different versions.

[JCQuintas]

Sure, also ensure it is working when passing both reffed and non-reffed components to the slots 👍

[bernardobelchior]

We're trying to detect if a ref is needed, but would it make sense to just forward it all the time?

In React 19 this isn't a problem because ref is a normal prop. In React <= 18, the user will see a warning to add forwardRef to the component if we're using a ref.

The issue with this is that if we start passing a ref to a component that uses consumeSlots, the users in React <= 18 whose slot isn't wrapped in forwardRef will start seeing a warning in dev. As long as behavior doesn't change with the lack of a ref, I don't think this is a breaking change, but it might be hard to detect that simply adding a ref might cause warnings in end users using React <= 18.

I tested this in React 18 and the missing forwardRef warning only shows up if we're actually using the ref. If we're forwarding it in consumeSlots, but we're not using it, then it won't trigger a warning.

A warning would be triggered here in React <= 18 if the slot isn't wrapped in forwardRef:

function Chart() {
  const props = { /* Props without a ref */ }
  const ref = useRef(null);

  return <ChartsLegend {...props} ref={ref} />
}

A warning would not be triggered here in React <= 18 even if the slot isn't wrapped in forwardRef:

function Chart() {
  const props = { /* Props without a ref */ }
  return <ChartsLegend {...props} />
}

If the slot is wrapped in forwardRef or if in React >= 19, there would also be no warning.

Basically, the affected users would be someone on React <= 18, whose slot isn't wrapped in forwardRef, and we add a ref to a component that accepts slots. The consequence of this would be a warning in dev. Is this acceptable, @JCQuintas @alexfauquette?

[JCQuintas]

This was added in this breaking change window, so we can still change it without a BK.

I'm ok with always passing the ref. We can remove checking the function arguments length altogether.

[bernardobelchior]

Also, it is a good idea to change the displayName of a user's component? Not sure if it's more confusing than helpful. I'm thinking of the use case of a user searching for their component in React DevTools and not being able to find it.

[JCQuintas]

The name change was mostly because of the ForwardRef which would wrap it into a new function, I just didn't check if a user component was passed.

Now we can probably remove it.

[bernardobelchior]

More info on the performance part: it's the logic we're running in ConsumeSlotsInternal that is making this slower. I removed part of it here and the results are similar to what we see in master.

I'm now trying to understand where the performance hit comes from and see if I can optimize consumeSlots. If it isn't possible, then we might need to go for another approach.

[bernardobelchior]

Performance seems to be unchanged, so I'll revert those changes.