Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[charts] fix x-axis tick label overflow #16709

Merged

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Feb 24, 2025

Part of #10928.

Fix x-axis tick label overflow by applying an ellipsis for overflowing labels.

This PR does not change the algorithm for tick label visibility, so that logic should remain the same. The only difference is if those label would cause an overflow, they no longer will. This is especially noticeable for angles !== 0 and for tick labels at the extremities of charts.

Before

image

After

image

Checklist

  • Test multi-line labels
    image

  • Test multiple anchors/baselines

    localhost_3001_playground_long-x-axis-ticks_ (2)

  • Test with zoom

    Screen.Recording.2025-03-04.at.09.42.23.mov
  • Test ellipsis with unicode characters
    image

Changelog

Tick labels in the x-axis of cartesian charts will now have an ellipsis applied to prevent overflow.
If your tick labels are being clipped sooner than you would like, you can increase the x-axis size by increasing its height property.
The default line-height has also been changed to 1.25, so if you aren't customizing the line height for x-axis tick labels, make sure to double check if the result is desirable.

@mui-bot
Copy link

mui-bot commented Feb 24, 2025

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

Updated pages:

Generated by 🚫 dangerJS against 1949237

@bernardobelchior bernardobelchior added the component: charts This is the name of the generic UI component, not the React module! label Feb 24, 2025
@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 8e61dfe to 961878d Compare February 24, 2025 15:51
@bernardobelchior
Copy link
Member Author

It seems the tick labels are hidden in visual tests:

image

The reason is simple, but also unfortunate. We're spreading the style of MUI's caption onto the tick label, which has a line-height of 1.66. By default, the X axis has a height of 25px and the tick size is by default 6px. This means that there's 19px for the tick label to take up. The default font size of the tick label is 12px, which multiplied by 1.66 is 19.92px, meaning that it's 0.92px above the limit, so the label is hidden.

Options:

  1. Increase default axis height
  2. Decrease default tick size
  3. Decrease tick label line height

In my opinion, number 3 is best because a line height of 1.66 seems to big for this use case.

This is a line height of 1.25:
image

This is a default line height of 1.66:

image

Is a default line height of 1.25 acceptable?

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 2b2dc4c to acd8b68 Compare February 25, 2025 08:36
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from a1d7350 to ba2165a Compare March 3, 2025 08:04
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 3, 2025
Copy link

codspeed-hq bot commented Mar 3, 2025

CodSpeed Performance Report

Merging #16709 will not alter performance

Comparing bernardobelchior:fix-axis-tick-label-overflow (1949237) with master (8d75836)

Summary

✅ 7 untouched benchmarks

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 7c0b2c2 to 87d9dfe Compare March 3, 2025 11:04
@bernardobelchior
Copy link
Member Author

There's a small conflict with overflow + text-anchor next to the limit of the chart.

In this case (circled blue), we have 60px to the left of the X axis to render the tick label. However, the text-anchor is middle, so we must center label. Given that we only have 60px to the left, the maximum width can only be 120px for the label to remain centered. However, setting the max width to 120px creates a void between the first and second tick labels:

image

There are at least these two options:

  1. Live with this limitation for now. This will only impact axes with band scale whose labels are centered and too long to display. In this case, my suggestion would be to rotate the label using the angle and this problem would be solved.
  2. Treat the text-anchor: middle as a suggestion, and override it in this case, to be able to display more of the tick label. The consequence is that the label would seem like pointing somewhere else;
  3. Integrate the ellipsis algorithm in the tick label visibility algorithm to more accurately define which labels to show. This would probably allow us to provide better results, at the cost of more development time.

Obviously, number 2 would be preferable, but I'm worried it might increase the scope of this PR too much since it might create more questions. One I can think of is:

  1. if we can add an ellipsis, should we try to show all labels with an ellipsis or show as many labels without ellipsis as possible?

@alexfauquette @JCQuintas what do you think?

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch 3 times, most recently from 885c737 to 3472b18 Compare March 4, 2025 08:09
@alexfauquette
Copy link
Member

Is a default line height of 1.25 acceptable?

Seems good

Live with this limitation for now. This will only impact axes with band scale whose labels are centered and too long to display. In this case, my suggestion would be to rotate the label using the angle and this problem would be solved.

I'm good with this solution. It's a realy nich issue

Comment on lines 40 to 54
do {
lastLength = newLength;
newLength = Math.floor(text.length * by);

ellipsizedText = sliceUntil(text, newLength).trim();
const fits = doesTextFitInRect(ellipsizedText + ELLIPSIS, config);
step += 1;

if (fits) {
longestFittingText = ellipsizedText;
by += 1 / 2 ** step;
} else {
by -= 1 / 2 ** step;
}
} while (newLength !== lastLength);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this algorithm is to search for the point where text is longest and fits.

Let's say you have a string with 32 characters. The algorithm will first check if the 32 characters fit. If not, it'll check if 16 characters fit, then:

  • If 16 characters don't fit, it'll check if 8 characters fit
  • If the 16 characters fit, it will check if 24 characters fit

The idea is to split the search space in two in every iteration. The algorithm will eventually stop when it can no longer split the search space.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 5, 2025
Copy link

github-actions bot commented Mar 5, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 9eed1a4 to d276110 Compare March 5, 2025 13:38
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 5, 2025
@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch 2 times, most recently from f24a704 to dcb8a3f Compare March 5, 2025 14:45

let count = 0;

// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/naming-convention,no-underscore-dangle
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to disable all of these, otherwise CI will fail 🤣

segments is an iterator, so we need to iterate it to count. I could do Array.from() but didn't want to allocate an array just to obtain its length.

As such, the _unused var is useless, but I can't name it _ or unused because that will cause different lint errors to fire.

image

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 9d09700 to 3aefeeb Compare March 5, 2025 16:50

const tickLabels = isClient
? shortenLabels(visibleLabels, drawingArea, tickLabelsMaxHeight, axisTickLabelProps.style)
: new Map();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should no labels or all labels when server-side rendering.

All labels would make more sense for the cases when there's no overlap, but they wouldn't look good for cases with an overlap 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All labels would make more sense for the cases when there's no overlap

I agree on this one, especially if devs don't care at all about SSR, they might use the responsive charts wich do not render on server because they don't have access to their height/width. And if they care, they can filter values by themself to remove overflow

@bernardobelchior bernardobelchior force-pushed the fix-axis-tick-label-overflow branch from 8cc3c1c to 1d4d31b Compare March 11, 2025 13:59
@bernardobelchior
Copy link
Member Author

There's this hydration issue that's causing the axis labels to be incorrectly positioned.

For some reason, Next isn't updating the y value of the labels after hydrating:

Screen.Recording.2025-03-11.at.14.58.36.mov

@bernardobelchior
Copy link
Member Author

There's this hydration issue that's causing the axis labels to be incorrectly positioned.

For some reason, Next isn't updating the y value of the labels after hydrating:

Screen.Recording.2025-03-11.at.14.58.36.mov

I have this fix, but I'm not too happy about it because we're applying a transform that might not make sense to apply if the user is customizing the component. @alexfauquette @JCQuintas do you see any other option?

@JCQuintas
Copy link
Member

There's this hydration issue that's causing the axis labels to be incorrectly positioned.
For some reason, Next isn't updating the y value of the labels after hydrating:
Screen.Recording.2025-03-11.at.14.58.36.mov

I have this fix, but I'm not too happy about it because we're applying a transform that might not make sense to apply if the user is customizing the component. @alexfauquette @JCQuintas do you see any other option?

Is this happening because getStringSize(label, axisLabelProps.style).height can't detect the correct height?

@bernardobelchior
Copy link
Member Author

There's this hydration issue that's causing the axis labels to be incorrectly positioned.
For some reason, Next isn't updating the y value of the labels after hydrating:
Screen.Recording.2025-03-11.at.14.58.36.mov

I have this fix, but I'm not too happy about it because we're applying a transform that might not make sense to apply if the user is customizing the component. @alexfauquette @JCQuintas do you see any other option?

Is this happening because getStringSize(label, axisLabelProps.style).height can't detect the correct height?

Yes. getStringSize returns { width: 0, height: 0 } on the server. Even if we use an heuristic for calculating the string size, there's always the possibility hydration mismatches.

@JCQuintas
Copy link
Member

There's this hydration issue that's causing the axis labels to be incorrectly positioned.
For some reason, Next isn't updating the y value of the labels after hydrating:
Screen.Recording.2025-03-11.at.14.58.36.mov

I have this fix, but I'm not too happy about it because we're applying a transform that might not make sense to apply if the user is customizing the component. @alexfauquette @JCQuintas do you see any other option?

Is this happening because getStringSize(label, axisLabelProps.style).height can't detect the correct height?

Yes. getStringSize returns { width: 0, height: 0 } on the server. Even if we use an heuristic for calculating the string size, there's always the possibility hydration mismatches.

Would changing the anchor solve the issue, instead of calculating the line height?

@bernardobelchior
Copy link
Member Author

There's this hydration issue that's causing the axis labels to be incorrectly positioned.
For some reason, Next isn't updating the y value of the labels after hydrating:
Screen.Recording.2025-03-11.at.14.58.36.mov

I have this fix, but I'm not too happy about it because we're applying a transform that might not make sense to apply if the user is customizing the component. @alexfauquette @JCQuintas do you see any other option?

Is this happening because getStringSize(label, axisLabelProps.style).height can't detect the correct height?

Yes. getStringSize returns { width: 0, height: 0 } on the server. Even if we use an heuristic for calculating the string size, there's always the possibility hydration mismatches.

Would changing the anchor solve the issue, instead of calculating the line height?

Yeah, it could work, but how would you go about it?

I tried changing the dominant-baseline and transform-origin, but they didn't seem to work. Not sure if I did something wrong.

@JCQuintas
Copy link
Member

JCQuintas commented Mar 11, 2025

Yeah, it could work, but how would you go about it?

I tried changing the dominant-baseline and transform-origin, but they didn't seem to work. Not sure if I did something wrong.

It seems you can try to get funky with dominantBaseline: position === 'bottom' ? 'text-after-edge' : 'text-before-edge' instead of the boring old position === 'bottom' ? 'hanging' : 'auto'

hanging -> top of text
ideographic -> bottom of text
text-after-edge -> magic stuff, long boring documentation, mostly translates to "the baseline is the top of the text-area"
text-before-edge -> "baseline is the bottom of the text area"

@JCQuintas
Copy link
Member

It seems you can try to get funky with dominantBaseline: position === 'bottom' ? 'text-after-edge' : 'text-before-edge' instead of the boring old position === 'bottom' ? 'hanging' : 'auto'

Disclaimer, test on different browsers, not sure how they will react to this 😆

@alexfauquette
Copy link
Member

@bernardobelchior I pushed the fix from JC suggestion.

Comment on lines +28 to 33
const isClient = useIsClient();

const wordsByLines = React.useMemo(
() => getWordsByLines({ style, needsComputation: text.includes('\n'), text }),
[style, text],
() => getWordsByLines({ style, needsComputation: isClient && text.includes('\n'), text }),
[style, text, isClient],
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed similar hydration issues when using \n in axes labels. SO I added this fix that ignore the size computation during the hydration phase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, technically isClient is not correct, because the hydration is on the client. It's more isHydrationDone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, technically isClient is not correct, because the hydration is on the client. It's more isHydrationDone

I'll rename it in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #16937

);

let startDy: number;
switch (dominantBaseline) {
case 'hanging':
case 'text-before-edge':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adde dthis one and the text-after-edge because it gives better result.

Makes me realise the hanging and auto are not 100% correctly computed but nobody complained about it

@bernardobelchior
Copy link
Member Author

It seems you can try to get funky with dominantBaseline: position === 'bottom' ? 'text-after-edge' : 'text-before-edge' instead of the boring old position === 'bottom' ? 'hanging' : 'auto'

Disclaimer, test on different browsers, not sure how they will react to this 😆

I found a solution that works for almost all browsers, but unfortunately it doesn't work on Safari <= 18.3 (i.e,. in Safari, it only works in the current beta, which is 18.4).

The problem is that the line-height unit (lh) doesn't seem to work on Safari <= 18.3 when using it as part of an SVG attribute, e.g., <tspan dy="1lh" />.

Here's an example CodeSandbox.

Screen.Recording.2025-03-12.at.10.27.46.mov

@bernardobelchior
Copy link
Member Author

@bernardobelchior I pushed the fix from JC suggestion.

Thank you!

@bernardobelchior bernardobelchior merged commit cb56bfd into mui:master Mar 12, 2025
19 checks passed
@bernardobelchior bernardobelchior deleted the fix-axis-tick-label-overflow branch March 12, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants