-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[tabs] After removing tab scrollable mode is triggered (bad behavior of scrollable mode in case of dynamic tabs) #39394
Comments
This is a regression, it used to work correctly in v5.14.1 and got broken in v5.14.2. #36071 or #38133 broke this. I'm reopening as #39415 doesn't include a regression test, the real fix is the regression test IMHO. Now, since this is a regression, and we will need to move that logic to Base UI, I think we definitely need a test. As for the fix, it seems that the origin is the intersection observer, a minimal reproduction: https://codesandbox.io/p/sandbox/inspiring-star-hc3vns?file=%2Fsrc%2FApp.js%3A16%2C9. So I think we can solve this with simply ignoring elements that are from a previous render: diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js
index d4d41ef720..904d187e1e 100644
--- a/packages/mui-material/src/Tabs/Tabs.js
+++ b/packages/mui-material/src/Tabs/Tabs.js
@@ -619,13 +619,16 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
};
const handleScrollButtonStart = (entries) => {
- setDisplayStartScroll(!entries[0].isIntersecting);
+ const isInDom = entries[0].target.parentElement !== null;
+ console.log('entries[0].isIntersecting', entries[0].isIntersecting)
+ setDisplayStartScroll(isInDom && !entries[0].isIntersecting);
};
const firstObserver = new IntersectionObserver(handleScrollButtonStart, observerOptions);
firstObserver.observe(firstTab);
const handleScrollButtonEnd = (entries) => {
- setDisplayEndScroll(!entries[0].isIntersecting);
+ const isInDom = entries[0].target.parentElement !== null;
+ setDisplayEndScroll(isInDom && !entries[0].isIntersecting);
};
const lastObserver = new IntersectionObserver(handleScrollButtonEnd, observerOptions);
lastObserver.observe(lastTab); simpler fix. But then, this is still hiding the root problem, we shouldn't observe an element that is unmounted. This fix makes more sense to me: diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js
index d4d41ef720..b44225cf58 100644
--- a/packages/mui-material/src/Tabs/Tabs.js
+++ b/packages/mui-material/src/Tabs/Tabs.js
@@ -17,6 +17,7 @@ import useEventCallback from '../utils/useEventCallback';
import tabsClasses, { getTabsUtilityClass } from './tabsClasses';
import ownerDocument from '../utils/ownerDocument';
import ownerWindow from '../utils/ownerWindow';
+import useEnhancedEffect from '../utils/useEnhancedEffect';
const nextItem = (list, item) => {
if (list === item) {
@@ -597,48 +598,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
};
}, [updateIndicatorState]);
- /**
- * Toggle visibility of start and end scroll buttons
- * Using IntersectionObserver on first and last Tabs.
- */
- React.useEffect(() => {
- const tabListChildren = Array.from(tabListRef.current.children);
- const length = tabListChildren.length;
-
- if (
- typeof IntersectionObserver !== 'undefined' &&
- length > 0 &&
- scrollable &&
- scrollButtons !== false
- ) {
- const firstTab = tabListChildren[0];
- const lastTab = tabListChildren[length - 1];
- const observerOptions = {
- root: tabsRef.current,
- threshold: 0.99,
- };
-
- const handleScrollButtonStart = (entries) => {
- setDisplayStartScroll(!entries[0].isIntersecting);
- };
- const firstObserver = new IntersectionObserver(handleScrollButtonStart, observerOptions);
- firstObserver.observe(firstTab);
-
- const handleScrollButtonEnd = (entries) => {
- setDisplayEndScroll(!entries[0].isIntersecting);
- };
- const lastObserver = new IntersectionObserver(handleScrollButtonEnd, observerOptions);
- lastObserver.observe(lastTab);
-
- return () => {
- firstObserver.disconnect();
- lastObserver.disconnect();
- };
- }
-
- return undefined;
- }, [scrollable, scrollButtons, updateScrollObserver, childrenProp?.length]);
-
React.useEffect(() => {
setMounted(true);
}, []);
@@ -707,6 +666,48 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
});
});
+ /**
+ * Toggle visibility of start and end scroll buttons
+ * Using IntersectionObserver on first and last Tabs.
+ */
+ useEnhancedEffect(() => {
+ const tabListChildren = Array.from(tabListRef.current.children);
+ const length = tabListChildren.length;
+
+ if (
+ typeof IntersectionObserver !== 'undefined' &&
+ length > 0 &&
+ scrollable &&
+ scrollButtons !== false
+ ) {
+ const firstTab = tabListChildren[0];
+ const lastTab = tabListChildren[length - 1];
+ const observerOptions = {
+ root: tabsRef.current,
+ threshold: 0.99,
+ };
+
+ const handleScrollButtonStart = (entries) => {
+ setDisplayStartScroll(!entries[0].isIntersecting);
+ };
+ const firstObserver = new IntersectionObserver(handleScrollButtonStart, observerOptions);
+ firstObserver.observe(firstTab);
+
+ const handleScrollButtonEnd = (entries) => {
+ setDisplayEndScroll(!entries[0].isIntersecting);
+ };
+ const lastObserver = new IntersectionObserver(handleScrollButtonEnd, observerOptions);
+ lastObserver.observe(lastTab);
+
+ return () => {
+ firstObserver.disconnect();
+ lastObserver.disconnect();
+ };
+ }
+
+ return undefined;
+ }, [scrollable, scrollButtons, updateScrollObserver, children.length]);
+
const handleKeyDown = (event) => {
const list = tabListRef.current;
const currentFocus = ownerDocument(list).activeElement; Potentially, the effect could be on |
Steps to reproduce
Link to live example: (required)
https://codesandbox.io/p/sandbox/frosty-snow-tlq78l?file=%2Fpackage.json%3A6%2C32
Steps:
Remove "Item one"
After remove there is scrollable mode with arrow on the right
Current behavior
After removing last tab, scrollable mode is trigger when there is no need
Expected behavior
After remove tab there should be no arrows for scroll as width of tabs is smaller then width of the wrapper component.
Context
It looks like a BUG as arrow is suddenly shown at the end of tabs toolbar.
Your environment
npx @mui/envinfo
Search keywords: tabs, scrollable
Order ID: 46820
The text was updated successfully, but these errors were encountered: