[charts-pro] Fix geometry not handling gestures in specific scenarios #18651
Conversation
|
Deploy preview: https://deploy-preview-18651--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: ▼-6.1KB(-0.05%) - Total Gzip Change: ▼-1.89KB(-0.05%) Show details for 100 more bundles (22 more not shown)@mui/x-charts-pro/ChartZoomSlider parsed: 🔺+29B(+0.04%) gzip: 🔺+13B(+0.05%) |
CodSpeed Performance ReportMerging #18651 will not alter performanceComparing Summary
|
Which part of the meaning is an issue? |
I thought it was going to allow the elements to overflow the drawing area, but we clip them regardless 😆 Will use the original one instead then. |
|
threw in a test to prevent regressions 😄 |
|
I don't get why we need the |
It is to make sure the slider handlers work. Else they interfere with the charts pan handlers |
|
Long story short-ish, in the chart we release pointer capture in some instances to ensure touch users can move the finger and get multiple tooltips. This can cause interference with both the zoom&pan and the sliders. We can't use So we need a way to know that the current target element is inside the drawing area. |
|
An alternative is to have the data-attributes on the sliders instead. As was the original solution here: #17480 (comment) |
| @@ -81,7 +81,7 @@ function BarPlot(props: BarPlotProps) { | |||
| const classes = useUtilityClasses(); | |||
|
|
|||
| return ( | |||
| <BarPlotRoot className={classes.root}> | |||
| <BarPlotRoot data-drawing-container className={classes.root}> | |||
There was a problem hiding this comment.
Should this be set on the parent? Otherwise when using composition data-drawing-container will always be included, even if you don't want it.
There was a problem hiding this comment.
Why wouldn't you want it?
There was a problem hiding this comment.
I'm not sure, but couldn't this cause a breaking change if someone isn't expecting this behavior to change? From now on, everything inside BarPlot, LinePlot and AreaPlot will be considered inside the drawing area, right?
Also, it's different from the pattern set for MarkPlot and ScatterPlot.
What was preventing them from working when using |
|
Also, I have a small question regarding For example, in |
|
@bernardobelchior some of your questions were answered in this: #18651 (comment) But to be more exact:
Initially I had it as a partial, though I ran into some troubles which are probably my own fault 😅
It is only the SVG when you grab the svg, otherwise it is the bar, or the mark, or anything that can capture a pointer. |
Ah, you're right. In the case of the area, it seems it's trying to check whether the area is entirely within the drawing area, which it isn't because it's zoomed in. That's the reason this bug exists, right? Can't we check if the pointer itself is within the drawing area? Why do we need to check the element instead? |
We can't use isPointInside(pointer.x/y) because it would disable interactions where we move the pointer outside, eg: pan/pinch |
I see, makes sense 👍 thanks for the explanation. |
|
If I resume what I understood:
For the second point, I don't get the reason why we need this I don't get what is a "section" and how to reproduce this bug |
It mostly doesn't do much when zoom in enabled, unless you already panned to the zoom border. On regular charts it allows you to slide your finger over the tooltips With the Screen.Recording.2025-07-02.at.15.32.56.movWithout Screen.Recording.2025-07-02.at.15.33.26.mov |
|
Ok, because the pointer get capputred, it can not enter/leave any of the item. I got confused because I was doing tests on the zoom-and-pan docs page 🙈 This solution still have some hard edges. For example let's pick a bar chart with zoom.
On the first example I still do not understand why zoom panning is still working outside of the chart even though the pointer capture got removed Another example is If you pointer down on a chart, you can not interact with another chart before doing pointer up. In my opinion the root cause is that we capture pointer event in case where it's not needed. |
Modern browsers implicitly capture the element in pretty much every occasion as far as I remember, only the |
|
I've removed the If there is a regression the tests should pick it up at least. |
The boundaries of some elements were outside of the drawing area, which made the
isElementInsideconsider them outside.