Skip to content

Conversation

Jayesh-11
Copy link
Contributor

@Jayesh-11 Jayesh-11 commented Sep 6, 2025

Fixes #44211
Fixes #40692

Root cause -
When the scroll happened the calculation for the position was not accounting for the width/height of the tab

The fix includes ltr, since rtl has lot of issues that needs to fixed alongside the current fix

Original issue -

issue

Resolved -

fix

Please do share any feedback
Thanks!


Before: https://stackblitz.com/edit/pzatxsan-kb1xsyjh

After: https://stackblitz.com/edit/pzatxsan

@mui-bot
Copy link

mui-bot commented Sep 6, 2025

Netlify deploy preview

https://deploy-preview-46869--material-ui.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/material 🔺+66B(+0.01%) 🔺+27B(+0.02%)
@mui/lab 0B(0.00%) 0B(0.00%)
@mui/system 0B(0.00%) 0B(0.00%)
@mui/utils 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 186b877

@Jayesh-11 Jayesh-11 changed the title Fix/tabs not scrolling to correct tab issue Fix : tabs not scrolling to correct tab issue Sep 6, 2025
@ZeeshanTamboli ZeeshanTamboli added the scope: tabs Changes related to the tabs. label Sep 8, 2025
@ZeeshanTamboli ZeeshanTamboli changed the title Fix : tabs not scrolling to correct tab issue [Tabs] Not scrolling to correct tab after refresh Sep 8, 2025
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I added the "Before" and "After" reproductions in the PR description. It seems to work well.

The Argos CI fails for RTL Vertical tabs demo.

Could you please add a test case?

@Jayesh-11
Copy link
Contributor Author

Thanks @ZeeshanTamboli , let me do that

@Jayesh-11
Copy link
Contributor Author

Jayesh-11 commented Sep 8, 2025

Hi @ZeeshanTamboli I've added related regressions PTAL
I was not really sure how to fix failing CI test for RTL
since the both regression screenshot seems correct here

the "Tab C" is center aligned in the new change, which seems to be in good direction
Screenshot 2025-09-09 at 1 10 35 AM

before

Screenshot 2025-09-09 at 1 13 14 AM

after

Screenshot 2025-09-09 at 1 12 17 AM

please do suggest the way forward, thanks!

@Jayesh-11
Copy link
Contributor Author

Jayesh-11 commented Sep 12, 2025

Hi @ZeeshanTamboli can you please review the pr and suggest changes if any?

Comment on lines 637 to 638
const nextScrollStart = tabsMeta[scrollStart] + (tabMeta[end] - tabsMeta[end]);
const nextScrollStart =
tabsMeta[scrollStart] + (tabMeta[end] - tabsMeta[end]) + tabMeta[size];
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Sep 13, 2025

Choose a reason for hiding this comment

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

Should we consider the similar size like here if left-side button is out of view as well, in the earlier above if condition?

@Jayesh-11
Copy link
Contributor Author

Hi @ZeeshanTamboli, thanks for suggestion
sorry about the delay, it took some time to reproduce issue in RTL
I've attached the fix with the video

Thanks!

output.mov

@Jayesh-11
Copy link
Contributor Author

Hi @ZeeshanTamboli can you please review the updated pr?

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@Jayesh-11 I think this is not a proper fix. Considering the tab's width/height (± tabMeta[size]) could allow overscroll in some layouts.

The problem that's happening with scrollButtons="auto" without your fix is that:

  • On first render, the selected tab is outside the viewport . We compute “just enough” scroll to bring its edge into view.
  • As soon as we scroll, overflow is now clearly true. It shows the scroll buttons, which shrink the visible viewport.
  • Because our scroll target didn't account for those buttons, the selected tab ends up partially clipped.

With scrollButtons={true}, the buttons are present during the measurement, so the math is correct and the tab lands fully in view.

So the solution is to recompute the scrollSelectedIntoView once the scroll buttons are available when scrollButtons="auto".

@Jayesh-11
Copy link
Contributor Author

Jayesh-11 commented Sep 25, 2025

Thanks for the feedback,
the insight regarding viewport is helpful
let me check and get back

@Jayesh-11
Copy link
Contributor Author

Screen.Recording.2025-09-27.at.2.06.15.PM.mov

I see what you meant
the scroll was working correctly but because of scroll button the issue was present
recomputing scrollSelectedIntoView based on scroll buttons presence solved the issue
Thanks!

@ZeeshanTamboli ZeeshanTamboli changed the title [Tabs] Not scrolling to correct tab after refresh [Tabs] Not scrolling to correct tab after refresh when auto scrollable Sep 28, 2025
@ZeeshanTamboli ZeeshanTamboli changed the title [Tabs] Not scrolling to correct tab after refresh when auto scrollable [Tabs] Fix not scrolling to correct tab after refresh when auto scrollable Sep 28, 2025
@ZeeshanTamboli ZeeshanTamboli added the type: bug It doesn't behave as expected. label Sep 28, 2025
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Thanks @Jayesh-11 for your contribution! Nice work. I made a few tweaks. This also fixes #40692.

@ZeeshanTamboli ZeeshanTamboli merged commit 46a8506 into mui:master Sep 28, 2025
22 checks passed
@Janpot
Copy link
Member

Janpot commented Oct 1, 2025

This PR causes the tabs to animate scroll on initial render. This in turn has made the screenshot testing flaky.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Oct 1, 2025

This PR causes the tabs to animate scroll on initial render. This in turn has made the screenshot testing flaky.

Just checked. It is expected that it will animate even on the first render in the case of scrollButtons="auto". Even if you remove the effect completely, it will animate on the first render when scrollButtons="auto" (it's just that the selected tab would be in the wrong position, that's what this fix is for). The visual regression tests for Tabs with auto scrollButtons added in this PR started to be flaky after merge. I am not sure how we can test this without these visual regression tests. Can we capture the screenshot after a few milliseconds just for these 4 tests? We just want to check the selected tab is completely in view.

I even tried #25469 for this particular effect.

Edit: Actually I think the scrollSelectedIntoView in this effect does not run on first render, but on subsequent renders.

@Jayesh-11
Copy link
Contributor Author

Jayesh-11 commented Oct 2, 2025

@ZeeshanTamboli Please do let me know your thoughts

we rely on width/visibility of scroll buttons to determine the position of the tab in viewport, for that we could possibly initially consider the width of scroll buttons when determining which position selected tab in viewport, unless initally selected tab is first or last tab(since if we consider during these there will be unwanted space of either left or right so here not including width makes sense, only for last and first tab).

so on component mount, let's say the tab is selected number 6/10 and outside of viewport on the right side, in that case we do the above and the position of the tab is +scrollButton width(towards left) from right boundary

@Janpot
Copy link
Member

Janpot commented Oct 2, 2025

Just checked. It is expected that it will animate even on the first render in the case of scrollButtons="auto".

This comment suggests the intention is for the tabs to not animate when mounting. If it does animate, I conclude there must be a bug.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Oct 2, 2025

@ZeeshanTamboli Please do let me know your thoughts

we rely on width/visibility of scroll buttons to determine the position of the tab in viewport, for that we could possibly initially consider the width of scroll buttons when determining which position selected tab in viewport, unless initally selected tab is first or last tab(since if we consider during these there will be unwanted space of either left or right so here not including width makes sense, only for last and first tab).

so on component mount, let's say the tab is selected number 6/10 and outside of viewport on the right side, in that case we do the above and the position of the tab is +scrollButton width(towards left) from right boundary

Isn't that already what's happening here? The effect calls scrollSelectedIntoView once the scroll buttons are rendered. That function scrolls the selected tab into view based on the tab’s dimensions and the container’s dimensions, which already account for the scroll button widths.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Oct 2, 2025

Just checked. It is expected that it will animate even on the first render in the case of scrollButtons="auto".

This comment suggests the intention is for the tabs to not animate when mounting. If it does animate, I conclude there must be a bug.

I had edited comment #46869 (comment), I think the animation is not occurring on "first render" but subsequent renders once the condition is satisfied (the scroll buttons are in view).

@Janpot
Copy link
Member

Janpot commented Oct 2, 2025

I think the animation is not occurring on "first render" but subsequent renders once the condition is satisfied (the scroll buttons are in view).

yes, technically it's not the first render. I assume the spirit of the comment is "when initially shown on screen, not as a result of user interaction".

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Oct 2, 2025

I dug into this further and unfortunately we'll need to revert this PR.

The effect also runs when the scroll buttons are clicked, which causes incorrect behavior. See the demo:

Screen-Recording.mp4

For the following example code:

import * as React from 'react';
import Tabs from '@mui/material/Tabs';
import Tab from '@mui/material/Tab';
import Box from '@mui/material/Box';

export default function ScrollableTabsButtonAuto() {
  const [value, setValue] = React.useState(4);
  const handleChange = (event: React.SyntheticEvent, newValue: number) => {
    setValue(newValue);
  };

  return (
    <Box sx={{ maxWidth: { xs: 320, sm: 480 }, bgcolor: 'background.paper' }}>
      <Tabs
        value={value}
        onChange={handleChange}
        variant="scrollable"
        scrollButtons="auto"
        aria-label="scrollable auto tabs example"
      >
        <Tab label="Item One" />
        <Tab label="Item Two" />
        <Tab label="Item Three" />
        <Tab label="Item Four" />
        <Tab label="Item Five" />
        <Tab label="Item Six" />
        <Tab label="Item Seven" />
      </Tabs>
    </Box>
  );
}

I don't have a solid alternative solution yet. Reverting will remove the flaky screenshot tests, but more importantly, the current fix is functionally incorrect.

@Jayesh-11
Copy link
Contributor Author

Let me investigate a better solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: tabs Changes related to the tabs. type: bug It doesn't behave as expected.
Projects
None yet
4 participants