From f9c6983cfc32ad6d6d4de9ed665afa591e49d4b6 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Sun, 5 Feb 2023 23:36:06 +0100 Subject: [PATCH 01/16] [Tabs] Fix and improve visibility of scrollButtons using IntersectionObserver --- docs/translations/api-docs/tabs/tabs.json | 2 +- packages/mui-material/src/Tabs/Tabs.d.ts | 3 +- packages/mui-material/src/Tabs/Tabs.js | 88 ++++++++------- packages/mui-material/src/Tabs/Tabs.test.js | 112 +++++++++++++++----- 4 files changed, 130 insertions(+), 75 deletions(-) diff --git a/docs/translations/api-docs/tabs/tabs.json b/docs/translations/api-docs/tabs/tabs.json index c52c0548d47842..517d8a9099bc83 100644 --- a/docs/translations/api-docs/tabs/tabs.json +++ b/docs/translations/api-docs/tabs/tabs.json @@ -1,7 +1,7 @@ { "componentDescription": "", "propDescriptions": { - "action": "Callback fired when the component mounts. This is useful when you want to trigger an action programmatically. It supports two actions: updateIndicator() and updateScrollButtons()", + "action": "Callback fired when the component mounts. This is useful when you want to trigger an action programmatically. It supports one action: updateIndicator()", "allowScrollButtonsMobile": "If true, the scroll buttons aren't forced hidden on mobile. By default the scroll buttons are hidden on mobile and takes precedence over scrollButtons.", "aria-label": "The label for the Tabs as a string.", "aria-labelledby": "An id or list of ids separated by a space that label the Tabs.", diff --git a/packages/mui-material/src/Tabs/Tabs.d.ts b/packages/mui-material/src/Tabs/Tabs.d.ts index 30de5788480316..050a7778f0a17c 100644 --- a/packages/mui-material/src/Tabs/Tabs.d.ts +++ b/packages/mui-material/src/Tabs/Tabs.d.ts @@ -14,7 +14,7 @@ export interface TabsTypeMap

; export interface TabsActions { updateIndicator(): void; - updateScrollButtons(): void; } export type TabsProps< diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 8e503ee2d66064..c28f983e119ccc 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -297,10 +297,8 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { const [mounted, setMounted] = React.useState(false); const [indicatorStyle, setIndicatorStyle] = React.useState(defaultIndicatorStyle); - const [displayScroll, setDisplayScroll] = React.useState({ - start: false, - end: false, - }); + const [displayStartScroll, setDisplayStartScroll] = React.useState(false); + const [displayEndScroll, setDisplayEndScroll] = React.useState(false); const [scrollerStyle, setScrollerStyle] = React.useState({ overflow: 'hidden', @@ -493,7 +491,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { /> ) : null; - const scrollButtonsActive = displayScroll.start || displayScroll.end; + const scrollButtonsActive = displayStartScroll || displayEndScroll; const showScrollButtons = scrollable && ((scrollButtons === 'auto' && scrollButtonsActive) || scrollButtons === true); @@ -502,7 +500,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { orientation={orientation} direction={isRtl ? 'right' : 'left'} onClick={handleStartScrollClick} - disabled={!displayScroll.start} + disabled={!displayStartScroll} {...TabScrollButtonProps} className={clsx(classes.scrollButtons, TabScrollButtonProps.className)} /> @@ -513,7 +511,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { orientation={orientation} direction={isRtl ? 'left' : 'right'} onClick={handleEndScrollClick} - disabled={!displayScroll.end} + disabled={!displayEndScroll} {...TabScrollButtonProps} className={clsx(classes.scrollButtons, TabScrollButtonProps.className)} /> @@ -540,28 +538,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { } }); - const updateScrollButtonState = useEventCallback(() => { - if (scrollable && scrollButtons !== false) { - const { scrollTop, scrollHeight, clientHeight, scrollWidth, clientWidth } = tabsRef.current; - let showStartScroll; - let showEndScroll; - - if (vertical) { - showStartScroll = scrollTop > 1; - showEndScroll = scrollTop < scrollHeight - clientHeight - 1; - } else { - const scrollLeft = getNormalizedScrollLeft(tabsRef.current, theme.direction); - // use 1 for the potential rounding error with browser zooms. - showStartScroll = isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; - showEndScroll = !isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; - } - - if (showStartScroll !== displayScroll.start || showEndScroll !== displayScroll.end) { - setDisplayScroll({ start: showStartScroll, end: showEndScroll }); - } - } - }); - React.useEffect(() => { const handleResize = debounce(() => { // If the Tabs component is replaced by Suspense with a fallback, the last @@ -572,7 +548,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { // replaced by Suspense with a fallback, once React is updated to version 18 if (tabsRef.current) { updateIndicatorState(); - updateScrollButtonState(); } }); const win = ownerWindow(tabsRef.current); @@ -594,21 +569,45 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { resizeObserver.disconnect(); } }; - }, [updateIndicatorState, updateScrollButtonState]); - - const handleTabsScroll = React.useMemo( - () => - debounce(() => { - updateScrollButtonState(); - }), - [updateScrollButtonState], - ); + }, [updateIndicatorState]); React.useEffect(() => { + let visibleObserver; + const { 0: firstTab, length, [length - 1]: lastTab } = Array.from(tabListRef.current.children); + + if ( + typeof IntersectionObserver !== 'undefined' && + length > 0 && + scrollable && + scrollButtons !== false + ) { + const handleVisibility = (entries) => { + entries.forEach(({ intersectionRatio, target }) => { + if (target === firstTab) { + setDisplayStartScroll(intersectionRatio !== 1); + } + if (target === lastTab) { + setDisplayEndScroll(intersectionRatio !== 1); + } + }); + }; + visibleObserver = new IntersectionObserver(handleVisibility, { + root: tabsRef.current, + threshold: 1, + }); + + visibleObserver.observe(firstTab); + if (lastTab && firstTab !== lastTab) { + visibleObserver.observe(lastTab); + } + } + return () => { - handleTabsScroll.clear(); + if (visibleObserver) { + visibleObserver.disconnect(); + } }; - }, [handleTabsScroll]); + }, [scrollable, scrollButtons, tabsRef, childrenProp]); React.useEffect(() => { setMounted(true); @@ -616,7 +615,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { React.useEffect(() => { updateIndicatorState(); - updateScrollButtonState(); }); React.useEffect(() => { @@ -628,9 +626,8 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { action, () => ({ updateIndicator: updateIndicatorState, - updateScrollButtons: updateScrollButtonState, }), - [updateIndicatorState, updateScrollButtonState], + [updateIndicatorState], ); const indicator = ( @@ -742,7 +739,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { : -scrollerStyle.scrollbarWidth, }} ref={tabsRef} - onScroll={handleTabsScroll} > {/* The tablist isn't interactive but the tabs are */} ', () => { if (isSafari) { this.skip(); } + global.IntersectionObserver = class IntersectionObserver { + constructor(cb, options) { + IntersectionObserver.cb = cb; + IntersectionObserver.options = options; + } + + disconnect() { + return this; + } + + observe() { + return this; + } + }; }); describeConformance(, () => ({ @@ -602,35 +616,53 @@ describe('', () => { describe('scroll button visibility states', () => { it('should set neither left nor right scroll button state', () => { - const { container, forceUpdate, getByRole } = render( + const { container, forceUpdate, getByRole, getAllByRole } = render( , ); const tablistContainer = getByRole('tablist').parentElement; - - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 200 - 40 * 2 }); + const { 0: firstTab, length, [length - 1]: lastTab } = getAllByRole('tab'); + + act(() => { + IntersectionObserver.cb([ + { + target: firstTab, + intersectionRatio: 1, + }, + { + target: lastTab, + intersectionRatio: 1, + }, + ]); + }); forceUpdate(); + expect(IntersectionObserver.options.root).to.equal(tablistContainer); + expect(IntersectionObserver.options.threshold).to.equal(1); expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(false); }); it('should set only left scroll button state', () => { - const { container, forceUpdate, getByRole } = render( + const { container, forceUpdate, getAllByRole } = render( , ); - const tablistContainer = getByRole('tablist').parentElement; - - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 }); - tablistContainer.scrollLeft = 96; + const [firstTab] = getAllByRole('tab'); + + act(() => { + IntersectionObserver.cb([ + { + target: firstTab, + intersectionRatio: 0.8, + }, + ]); + }); forceUpdate(); expect(hasLeftScrollButton(container)).to.equal(true); @@ -638,18 +670,23 @@ describe('', () => { }); it('should set only right scroll button state', () => { - const { container, forceUpdate, getByRole } = render( + const { container, forceUpdate, getAllByRole } = render( , ); - const tablistContainer = getByRole('tablist').parentElement; - - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 }); - tablistContainer.scrollLeft = 0; + const lastTab = getAllByRole('tab').pop(); + + act(() => { + IntersectionObserver.cb([ + { + target: lastTab, + intersectionRatio: 0.98, + }, + ]); + }); forceUpdate(); expect(hasLeftScrollButton(container)).to.equal(false); @@ -657,17 +694,26 @@ describe('', () => { }); it('should set both left and right scroll button state', () => { - const { container, forceUpdate, getByRole } = render( + const { container, forceUpdate, getAllByRole } = render( , ); - const tablistContainer = getByRole('tablist').parentElement; - - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 }); - tablistContainer.scrollLeft = 5; + const { 0: firstTab, length, [length - 1]: lastTab } = getAllByRole('tab'); + + act(() => { + IntersectionObserver.cb([ + { + target: firstTab, + intersectionRatio: 0.5, + }, + { + target: lastTab, + intersectionRatio: 0.4, + }, + ]); + }); forceUpdate(); expect(hasLeftScrollButton(container)).to.equal(true); @@ -688,13 +734,27 @@ describe('', () => { , ); const tablistContainer = getByRole('tablist').parentElement; - const tabs = getAllByRole('tab'); + const [tab1, tab2, tab3] = getAllByRole('tab'); Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tabs[0], 'clientWidth', { value: 100 }); - Object.defineProperty(tabs[1], 'clientWidth', { value: 50 }); - Object.defineProperty(tabs[2], 'clientWidth', { value: 100 }); + Object.defineProperty(tab1, 'clientWidth', { value: 100 }); + Object.defineProperty(tab2, 'clientWidth', { value: 50 }); + Object.defineProperty(tab3, 'clientWidth', { value: 100 }); Object.defineProperty(tablistContainer, 'scrollWidth', { value: 100 + 50 + 100 }); tablistContainer.scrollLeft = 20; + + act(() => { + IntersectionObserver.cb([ + { + target: tab1, + intersectionRatio: 0.5, + }, + { + target: tab3, + intersectionRatio: 0, + }, + ]); + }); + forceUpdate(); clock.tick(1000); expect(hasLeftScrollButton(container)).to.equal(true); From 62e219ecfd12188d7632921e8b25aa37caf6d4a5 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Tue, 14 Feb 2023 02:15:07 +0100 Subject: [PATCH 02/16] refacto observer intersection and fix e2e test --- packages/mui-material/src/Tabs/Tabs.js | 56 +++-- packages/mui-material/src/Tabs/Tabs.test.js | 237 ++++++++------------ test/utils/createRenderer.tsx | 7 + 3 files changed, 137 insertions(+), 163 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index c28f983e119ccc..7cbd3672ddca68 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -571,9 +571,34 @@ 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(() => { - let visibleObserver; + let firstObserver; + let lastObserver; const { 0: firstTab, length, [length - 1]: lastTab } = Array.from(tabListRef.current.children); + const observerOptions = { + root: tabsRef.current, + threshold: 1, + }; + + const handleScrollButtonStart = debounce((entries) => { + let display = false; + entries.forEach(({ intersectionRatio }) => { + display = intersectionRatio !== 1; + }); + setDisplayStartScroll(display); + }); + + const handleScrollButtonEnd = debounce((entries) => { + let display = false; + entries.forEach(({ intersectionRatio }) => { + display = intersectionRatio !== 1; + }); + setDisplayEndScroll(display); + }); if ( typeof IntersectionObserver !== 'undefined' && @@ -581,31 +606,20 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { scrollable && scrollButtons !== false ) { - const handleVisibility = (entries) => { - entries.forEach(({ intersectionRatio, target }) => { - if (target === firstTab) { - setDisplayStartScroll(intersectionRatio !== 1); - } - if (target === lastTab) { - setDisplayEndScroll(intersectionRatio !== 1); - } - }); - }; - visibleObserver = new IntersectionObserver(handleVisibility, { - root: tabsRef.current, - threshold: 1, - }); + firstObserver = new IntersectionObserver(handleScrollButtonStart, observerOptions); + firstObserver.observe(firstTab); - visibleObserver.observe(firstTab); - if (lastTab && firstTab !== lastTab) { - visibleObserver.observe(lastTab); + if (length > 1) { + lastObserver = new IntersectionObserver(handleScrollButtonEnd, observerOptions); + lastObserver.observe(lastTab); } } return () => { - if (visibleObserver) { - visibleObserver.disconnect(); - } + firstObserver?.disconnect(); + lastObserver?.disconnect(); + handleScrollButtonStart.clear(); + handleScrollButtonEnd.clear(); }; }, [scrollable, scrollButtons, tabsRef, childrenProp]); diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index 1753680a28433d..cea80f7661aa80 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -44,6 +44,15 @@ describe('', () => { const { clock, render, renderToString } = createRenderer(); + const delayObserver = 1200; + const waitForObserver = (fn, delay = delayObserver) => + new Promise((resolve) => { + setTimeout(() => { + fn(); + resolve(); + }, delay); + }); + before(function beforeHook() { const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); @@ -54,20 +63,6 @@ describe('', () => { if (isSafari) { this.skip(); } - global.IntersectionObserver = class IntersectionObserver { - constructor(cb, options) { - IntersectionObserver.cb = cb; - IntersectionObserver.options = options; - } - - disconnect() { - return this; - } - - observe() { - return this; - } - }; }); describeConformance(, () => ({ @@ -476,32 +471,29 @@ describe('', () => { expect(container.querySelectorAll(selector)).to.have.lengthOf(1); }); - it('should response to scroll events', function test() { + it('should response to scroll events', async function test() { + clock.restore(); + this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } const { container, forceUpdate, getByRole } = render(tabs); const tablistContainer = getByRole('tablist').parentElement; - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); + forceUpdate(); tablistContainer.scrollLeft = 10; - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 }); - Object.defineProperty(tablistContainer, 'getBoundingClientRect', { - value: () => ({ - left: 0, - right: 50, - }), + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(true); + expect(hasRightScrollButton(container)).to.equal(true); }); - forceUpdate(); - clock.tick(1000); - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); + tablistContainer.scrollLeft = 0; fireEvent.scroll(container.querySelector(`.${classes.scroller}.${classes.scrollableX}`)); - clock.tick(166); - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(true); + }); }); it('should get a scrollbar size listener', () => { @@ -536,8 +528,6 @@ describe('', () => { }); describe('prop: scrollButtons', () => { - clock.withFakeTimers(); - it('should render scroll buttons', () => { const { container } = render( @@ -575,11 +565,11 @@ describe('', () => { expect(container.querySelectorAll(`.${classes.scrollButtonsHideMobile}`)).to.have.lengthOf(0); }); - it('should handle window resize event', function test() { + it('should handle window resize event', async function test() { + this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( @@ -590,134 +580,107 @@ describe('', () => { const tablistContainer = getByRole('tablist').parentElement; - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); + forceUpdate(); tablistContainer.scrollLeft = 10; - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 }); - Object.defineProperty(tablistContainer, 'getBoundingClientRect', { - value: () => ({ - left: 0, - right: 100, - }), + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(true); + expect(hasRightScrollButton(container)).to.equal(true); }); - forceUpdate(); - clock.tick(1000); - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); tablistContainer.scrollLeft = 0; act(() => { window.dispatchEvent(new window.Event('resize', {})); }); - clock.tick(166); - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(true); + }); }); describe('scroll button visibility states', () => { - it('should set neither left nor right scroll button state', () => { - const { container, forceUpdate, getByRole, getAllByRole } = render( + it('should set neither left nor right scroll button state', async function test() { + if (isJSDOM) { + this.skip(); + } + const { container, forceUpdate } = render( , ); - const tablistContainer = getByRole('tablist').parentElement; - const { 0: firstTab, length, [length - 1]: lastTab } = getAllByRole('tab'); - - act(() => { - IntersectionObserver.cb([ - { - target: firstTab, - intersectionRatio: 1, - }, - { - target: lastTab, - intersectionRatio: 1, - }, - ]); - }); forceUpdate(); - expect(IntersectionObserver.options.root).to.equal(tablistContainer); - expect(IntersectionObserver.options.threshold).to.equal(1); - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(false); + + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + }); }); - it('should set only left scroll button state', () => { - const { container, forceUpdate, getAllByRole } = render( + it('should set only left scroll button state', async function test() { + if (isJSDOM) { + this.skip(); + } + const { container, forceUpdate, getByRole } = render( , ); - const [firstTab] = getAllByRole('tab'); - - act(() => { - IntersectionObserver.cb([ - { - target: firstTab, - intersectionRatio: 0.8, - }, - ]); - }); + const tablistContainer = getByRole('tablist').parentElement; forceUpdate(); - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(false); + tablistContainer.scrollLeft = 240; + + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(true); + expect(hasRightScrollButton(container)).to.equal(false); + }); }); - it('should set only right scroll button state', () => { - const { container, forceUpdate, getAllByRole } = render( + it('should set only right scroll button state', async function test() { + if (isJSDOM) { + this.skip(); + } + const { container, forceUpdate, getByRole } = render( , ); - const lastTab = getAllByRole('tab').pop(); - - act(() => { - IntersectionObserver.cb([ - { - target: lastTab, - intersectionRatio: 0.98, - }, - ]); - }); + const tablistContainer = getByRole('tablist').parentElement; forceUpdate(); - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); + tablistContainer.scrollLeft = 0; + + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(true); + }); }); - it('should set both left and right scroll button state', () => { - const { container, forceUpdate, getAllByRole } = render( + it('should set both left and right scroll button state', async function test() { + if (isJSDOM) { + this.skip(); + } + const { container, forceUpdate, getByRole } = render( , ); - const { 0: firstTab, length, [length - 1]: lastTab } = getAllByRole('tab'); - - act(() => { - IntersectionObserver.cb([ - { - target: firstTab, - intersectionRatio: 0.5, - }, - { - target: lastTab, - intersectionRatio: 0.4, - }, - ]); - }); + const tablistContainer = getByRole('tablist').parentElement; forceUpdate(); - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); + tablistContainer.scrollLeft = 5; + + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(true); + expect(hasRightScrollButton(container)).to.equal(true); + }); }); }); }); @@ -725,8 +688,13 @@ describe('', () => { describe('scroll button behavior', () => { clock.withFakeTimers(); - it('should scroll visible items', () => { - const { container, forceUpdate, getByRole, getAllByRole } = render( + it('should scroll visible items', async function test() { + clock.restore(); + this.timeout(delayObserver * 2.5); + if (isJSDOM) { + this.skip(); + } + const { container, forceUpdate, getByRole } = render( @@ -734,40 +702,25 @@ describe('', () => { , ); const tablistContainer = getByRole('tablist').parentElement; - const [tab1, tab2, tab3] = getAllByRole('tab'); - Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 }); - Object.defineProperty(tab1, 'clientWidth', { value: 100 }); - Object.defineProperty(tab2, 'clientWidth', { value: 50 }); - Object.defineProperty(tab3, 'clientWidth', { value: 100 }); - Object.defineProperty(tablistContainer, 'scrollWidth', { value: 100 + 50 + 100 }); + + forceUpdate(); tablistContainer.scrollLeft = 20; - act(() => { - IntersectionObserver.cb([ - { - target: tab1, - intersectionRatio: 0.5, - }, - { - target: tab3, - intersectionRatio: 0, - }, - ]); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(true); + expect(hasRightScrollButton(container)).to.equal(true); }); - forceUpdate(); - clock.tick(1000); - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); - fireEvent.click(findScrollButton(container, 'left')); - clock.tick(1000); - expect(tablistContainer.scrollLeft).not.to.be.above(0); + await waitForObserver(() => { + expect(tablistContainer.scrollLeft).not.to.be.above(0); + }, delayObserver * 0.5); tablistContainer.scrollLeft = 0; fireEvent.click(findScrollButton(container, 'right')); - clock.tick(1000); - expect(tablistContainer.scrollLeft).equal(100); + await waitForObserver(() => { + expect(tablistContainer.scrollLeft).equal(100); + }, delayObserver * 0.5); }); it('should horizontally scroll by width of partially visible item', () => { diff --git a/test/utils/createRenderer.tsx b/test/utils/createRenderer.tsx index b7de81adbe5044..37eceffa5708be 100644 --- a/test/utils/createRenderer.tsx +++ b/test/utils/createRenderer.tsx @@ -357,6 +357,10 @@ interface Clock { * Runs the current test suite (i.e. `describe` block) with fake timers. */ withFakeTimers(): void; + /** + * Restore the real timer + */ + restore(): void; } type ClockConfig = undefined | number | Date; @@ -426,6 +430,9 @@ function createClock(defaultMode: 'fake' | 'real', config: ClockConfig): Clock { mode = defaultMode; }); }, + restore() { + clock?.restore(); + }, }; } From c2bc67d10a7869557443e755ab04daeafe0d5358 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Tue, 14 Feb 2023 23:08:22 +0100 Subject: [PATCH 03/16] add e2e test for dynamic tabs --- packages/mui-material/src/Tabs/Tabs.test.js | 73 +++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index cea80f7661aa80..0f276477ee5817 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -1363,4 +1363,77 @@ describe('', () => { ]); }); }); + + describe('dynamic tabs', () => { + it('should not show scroll buttons if a tab added or removed in auto width', async function test() { + this.timeout(delayObserver * 2.5); + if (isJSDOM) { + this.skip(); + } + + const DynamicTabs = () => { + const [tabs, setTabs] = React.useState(['item1', 'item2', 'item3']); + return ( + <> + + + + {tabs.map( (label, index) => )} + + + ) + }; + const { container, getByTestId } = render(); + const addButton = getByTestId('add'); + const deleteButton = getByTestId('delete'); + + fireEvent.click(addButton); + fireEvent.click(addButton); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + }); + + fireEvent.click(deleteButton); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + }); + }); + + it.only('should show scroll buttons if a tab added or removed in fixed width', async function test() { + this.timeout(delayObserver * 2.5); + if (isJSDOM) { + this.skip(); + } + + const DynamicTabs = () => { + const [tabs, setTabs] = React.useState(['item1', 'item2']); + return ( + <> + + + + {tabs.map( (label, index) => )} + + + ) + }; + const { container, getByTestId } = render(); + const addButton = getByTestId('add'); + const deleteButton = getByTestId('delete'); + + fireEvent.click(addButton); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(true); + }); + + fireEvent.click(deleteButton); + await waitForObserver(() => { + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + }); + }); + }); }); From 55359fdb9e0c6c75f56f8e43dd850c57cad66445 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Tue, 14 Feb 2023 23:37:01 +0100 Subject: [PATCH 04/16] fix lint prettier --- packages/mui-material/src/Tabs/Tabs.test.js | 48 +++++++++++++-------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index 0f276477ee5817..0e97fe4730f442 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -1371,18 +1371,24 @@ describe('', () => { this.skip(); } - const DynamicTabs = () => { + function DynamicTabs() { const [tabs, setTabs] = React.useState(['item1', 'item2', 'item3']); return ( - <> - - + + + - {tabs.map( (label, index) => )} + {tabs.map((label, index) => ( + + ))} - - ) - }; + + ); + } const { container, getByTestId } = render(); const addButton = getByTestId('add'); const deleteButton = getByTestId('delete'); @@ -1401,24 +1407,30 @@ describe('', () => { }); }); - it.only('should show scroll buttons if a tab added or removed in fixed width', async function test() { + it('should show scroll buttons if a tab added or removed in fixed width', async function test() { this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } - const DynamicTabs = () => { + function DynamicTabs() { const [tabs, setTabs] = React.useState(['item1', 'item2']); return ( - <> - - - - {tabs.map( (label, index) => )} + + + + + {tabs.map((label, index) => ( + + ))} - - ) - }; + + ); + } const { container, getByTestId } = render(); const addButton = getByTestId('add'); const deleteButton = getByTestId('delete'); From 56804c266baf9fdaf13620d0762320562fe97831 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Sat, 24 Jun 2023 19:41:23 +0200 Subject: [PATCH 05/16] fix delay render of scroll buttons --- packages/mui-material/src/Tabs/Tabs.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 35334a39f0c74c..6b356dfb38ea43 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -505,11 +505,9 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { /> ) : null; - const scrollButtonsActive = displayStartScroll || displayEndScroll; - const showScrollButtons = - scrollable && ((scrollButtons === 'auto' && scrollButtonsActive) || scrollButtons === true); + const isScrollableButtons = scrollable && (scrollButtons === 'auto' || scrollButtons === true); - conditionalElements.scrollButtonStart = showScrollButtons ? ( + conditionalElements.scrollButtonStart = isScrollableButtons ? ( ) : null; - conditionalElements.scrollButtonEnd = showScrollButtons ? ( + conditionalElements.scrollButtonEnd = isScrollableButtons ? ( { + const handleScrollButtonStart = (entries) => { let display = false; entries.forEach(({ intersectionRatio }) => { display = intersectionRatio !== 1; }); setDisplayStartScroll(display); - }); + }; - const handleScrollButtonEnd = debounce((entries) => { + const handleScrollButtonEnd = (entries) => { let display = false; entries.forEach(({ intersectionRatio }) => { display = intersectionRatio !== 1; }); setDisplayEndScroll(display); - }); + }; if ( typeof IntersectionObserver !== 'undefined' && @@ -638,8 +636,6 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { return () => { firstObserver?.disconnect(); lastObserver?.disconnect(); - handleScrollButtonStart.clear(); - handleScrollButtonEnd.clear(); }; }, [scrollable, scrollButtons, tabsRef, childrenProp]); From bc978f6b35abfd36119f8ef33f25e90ac558fcb2 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Mon, 26 Jun 2023 23:04:41 +0200 Subject: [PATCH 06/16] use builtin waitFor --- packages/mui-material/src/Tabs/Tabs.test.js | 73 +++++++-------------- 1 file changed, 25 insertions(+), 48 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index 621846df39b509..b5bd7c6c5566e0 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -8,6 +8,7 @@ import { fireEvent, screen, strictModeDoubleLoggingSuppressed, + waitFor, } from 'test/utils'; import Tab from '@mui/material/Tab'; import Tabs, { tabsClasses as classes } from '@mui/material/Tabs'; @@ -47,15 +48,6 @@ describe('', () => { const { clock, render, renderToString } = createRenderer(); - const delayObserver = 1200; - const waitForObserver = (fn, delay = delayObserver) => - new Promise((resolve) => { - setTimeout(() => { - fn(); - resolve(); - }, delay); - }); - before(function beforeHook() { const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); @@ -492,7 +484,6 @@ describe('', () => { }); describe('prop: variant="scrollable"', () => { - clock.withFakeTimers(); const tabs = ( @@ -509,17 +500,14 @@ describe('', () => { }); it('should response to scroll events', async function test() { - clock.restore(); - this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render(tabs); + const { container, getByRole } = render(tabs); const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 10; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(true); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -527,7 +515,7 @@ describe('', () => { tablistContainer.scrollLeft = 0; fireEvent.scroll(container.querySelector(`.${classes.scroller}.${classes.scrollableX}`)); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -603,11 +591,10 @@ describe('', () => { }); it('should handle window resize event', async function test() { - this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( + const { container, getByRole } = render( @@ -617,9 +604,8 @@ describe('', () => { const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 10; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(true); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -629,7 +615,7 @@ describe('', () => { window.dispatchEvent(new window.Event('resize', {})); }); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -640,16 +626,14 @@ describe('', () => { if (isJSDOM) { this.skip(); } - const { container, forceUpdate } = render( + const { container } = render( , ); - forceUpdate(); - - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(false); }); @@ -659,7 +643,7 @@ describe('', () => { if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( + const { container, getByRole } = render( @@ -668,10 +652,9 @@ describe('', () => { ); const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 240; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(true); expect(hasRightScrollButton(container)).to.equal(false); }); @@ -681,7 +664,7 @@ describe('', () => { if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( + const { container, getByRole } = render( @@ -690,10 +673,9 @@ describe('', () => { ); const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 0; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -703,7 +685,7 @@ describe('', () => { if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( + const { container, getByRole } = render( @@ -711,10 +693,9 @@ describe('', () => { ); const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 5; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(true); expect(hasRightScrollButton(container)).to.equal(true); }); @@ -727,11 +708,10 @@ describe('', () => { it('should scroll visible items', async function test() { clock.restore(); - this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } - const { container, forceUpdate, getByRole } = render( + const { container, getByRole } = render( @@ -740,24 +720,23 @@ describe('', () => { ); const tablistContainer = getByRole('tablist').parentElement; - forceUpdate(); tablistContainer.scrollLeft = 20; - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(true); expect(hasRightScrollButton(container)).to.equal(true); }); fireEvent.click(findScrollButton(container, 'left')); - await waitForObserver(() => { + await waitFor(() => { expect(tablistContainer.scrollLeft).not.to.be.above(0); - }, delayObserver * 0.5); + }); tablistContainer.scrollLeft = 0; fireEvent.click(findScrollButton(container, 'right')); - await waitForObserver(() => { + await waitFor(() => { expect(tablistContainer.scrollLeft).equal(100); - }, delayObserver * 0.5); + }); }); it('should horizontally scroll by width of partially visible item', () => { @@ -1403,7 +1382,6 @@ describe('', () => { describe('dynamic tabs', () => { it('should not show scroll buttons if a tab added or removed in auto width', async function test() { - this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } @@ -1432,20 +1410,19 @@ describe('', () => { fireEvent.click(addButton); fireEvent.click(addButton); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(false); }); fireEvent.click(deleteButton); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(false); }); }); it('should show scroll buttons if a tab added or removed in fixed width', async function test() { - this.timeout(delayObserver * 2.5); if (isJSDOM) { this.skip(); } @@ -1473,13 +1450,13 @@ describe('', () => { const deleteButton = getByTestId('delete'); fireEvent.click(addButton); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(true); }); fireEvent.click(deleteButton); - await waitForObserver(() => { + await waitFor(() => { expect(hasLeftScrollButton(container)).to.equal(false); expect(hasRightScrollButton(container)).to.equal(false); }); From 207c9602fd0aa7b4d61b02bc96f94f62088d7605 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Mon, 10 Jul 2023 14:29:40 +0200 Subject: [PATCH 07/16] fix decimal scroll --- packages/mui-material/src/Tabs/Tabs.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 6b356dfb38ea43..bdf839f78eea16 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -597,15 +597,16 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { let firstObserver; let lastObserver; const { 0: firstTab, length, [length - 1]: lastTab } = Array.from(tabListRef.current.children); + const threshold = 0.99; const observerOptions = { root: tabsRef.current, - threshold: 1, + threshold, }; const handleScrollButtonStart = (entries) => { let display = false; entries.forEach(({ intersectionRatio }) => { - display = intersectionRatio !== 1; + display = intersectionRatio < threshold; }); setDisplayStartScroll(display); }; @@ -613,7 +614,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { const handleScrollButtonEnd = (entries) => { let display = false; entries.forEach(({ intersectionRatio }) => { - display = intersectionRatio !== 1; + display = intersectionRatio < threshold; }); setDisplayEndScroll(display); }; From 742a838854dd0609b5c16fe3afd35effd4fb9cd3 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Sun, 16 Jul 2023 19:24:23 +0200 Subject: [PATCH 08/16] fix comments --- docs/translations/api-docs/tabs/tabs.json | 2 +- packages/mui-material/src/Tabs/Tabs.d.ts | 4 +++- packages/mui-material/src/Tabs/Tabs.js | 21 ++++++++++++--------- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/docs/translations/api-docs/tabs/tabs.json b/docs/translations/api-docs/tabs/tabs.json index 6129e6cbb7f622..9ae0efcf1a4207 100644 --- a/docs/translations/api-docs/tabs/tabs.json +++ b/docs/translations/api-docs/tabs/tabs.json @@ -1,7 +1,7 @@ { "componentDescription": "", "propDescriptions": { - "action": "Callback fired when the component mounts. This is useful when you want to trigger an action programmatically. It supports one action: updateIndicator()", + "action": "Callback fired when the component mounts. This is useful when you want to trigger an action programmatically. It supports two actions: updateIndicator() and updateScrollButtons()", "allowScrollButtonsMobile": "If true, the scroll buttons aren't forced hidden on mobile. By default the scroll buttons are hidden on mobile and takes precedence over scrollButtons.", "aria-label": "The label for the Tabs as a string.", "aria-labelledby": "An id or list of ids separated by a space that label the Tabs.", diff --git a/packages/mui-material/src/Tabs/Tabs.d.ts b/packages/mui-material/src/Tabs/Tabs.d.ts index d3aea17f304ca0..312d05a9b06144 100644 --- a/packages/mui-material/src/Tabs/Tabs.d.ts +++ b/packages/mui-material/src/Tabs/Tabs.d.ts @@ -29,7 +29,7 @@ export interface TabsTypeMap

; export interface TabsActions { updateIndicator(): void; + /** @deprecated this action does nothing */ + updateScrollButtons(): void; } export type TabsProps< diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index bdf839f78eea16..37b246ce4f1e91 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -505,9 +505,11 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { /> ) : null; - const isScrollableButtons = scrollable && (scrollButtons === 'auto' || scrollButtons === true); + const scrollButtonsActive = displayStartScroll || displayEndScroll; + const showScrollableButtons = + scrollable && ((scrollButtons === 'auto' && scrollButtonsActive) || scrollButtons === true); - conditionalElements.scrollButtonStart = isScrollableButtons ? ( + conditionalElements.scrollButtonStart = showScrollableButtons ? ( ) : null; - conditionalElements.scrollButtonEnd = isScrollableButtons ? ( + conditionalElements.scrollButtonEnd = showScrollableButtons ? ( { let display = false; - entries.forEach(({ intersectionRatio }) => { - display = intersectionRatio < threshold; + entries.forEach(({ isIntersecting }) => { + display = !isIntersecting; }); setDisplayStartScroll(display); }; const handleScrollButtonEnd = (entries) => { let display = false; - entries.forEach(({ intersectionRatio }) => { - display = intersectionRatio < threshold; + entries.forEach(({ isIntersecting }) => { + display = !isIntersecting; }); setDisplayEndScroll(display); }; @@ -638,7 +640,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { firstObserver?.disconnect(); lastObserver?.disconnect(); }; - }, [scrollable, scrollButtons, tabsRef, childrenProp]); + }, [scrollable, scrollButtons, childrenProp]); React.useEffect(() => { setMounted(true); @@ -657,6 +659,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { action, () => ({ updateIndicator: updateIndicatorState, + updateScrollButtons: () => {}, }), [updateIndicatorState], ); @@ -799,7 +802,7 @@ Tabs.propTypes /* remove-proptypes */ = { /** * Callback fired when the component mounts. * This is useful when you want to trigger an action programmatically. - * It supports one action: `updateIndicator()` + * It supports two actions: `updateIndicator()` and `updateScrollButtons()` * * @param {object} actions This object contains all possible actions * that can be triggered programmatically. From 67971a5abef1dd07e9107e2c6c9e99605d963402 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Sun, 16 Jul 2023 19:44:43 +0200 Subject: [PATCH 09/16] fix naming --- packages/mui-material/src/Tabs/Tabs.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 37b246ce4f1e91..7246fe53b3d06f 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -506,10 +506,10 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { ) : null; const scrollButtonsActive = displayStartScroll || displayEndScroll; - const showScrollableButtons = + const showScrollButtons = scrollable && ((scrollButtons === 'auto' && scrollButtonsActive) || scrollButtons === true); - conditionalElements.scrollButtonStart = showScrollableButtons ? ( + conditionalElements.scrollButtonStart = showScrollButtons ? ( ) : null; - conditionalElements.scrollButtonEnd = showScrollableButtons ? ( + conditionalElements.scrollButtonEnd = showScrollButtons ? ( Date: Tue, 18 Jul 2023 22:48:42 +0200 Subject: [PATCH 10/16] use length as depcy, remove deprecated --- packages/mui-material/src/Tabs/Tabs.d.ts | 1 - packages/mui-material/src/Tabs/Tabs.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.d.ts b/packages/mui-material/src/Tabs/Tabs.d.ts index 312d05a9b06144..4fc93a32af82b1 100644 --- a/packages/mui-material/src/Tabs/Tabs.d.ts +++ b/packages/mui-material/src/Tabs/Tabs.d.ts @@ -189,7 +189,6 @@ declare const Tabs: OverridableComponent; export interface TabsActions { updateIndicator(): void; - /** @deprecated this action does nothing */ updateScrollButtons(): void; } diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 7246fe53b3d06f..fe74e9fc97d250 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -640,7 +640,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { firstObserver?.disconnect(); lastObserver?.disconnect(); }; - }, [scrollable, scrollButtons, childrenProp]); + }, [scrollable, scrollButtons, childrenProp?.length]); React.useEffect(() => { setMounted(true); From 5d65ce8f5389f9577d1f98a69f28198367f538eb Mon Sep 17 00:00:00 2001 From: SaidMarar Date: Wed, 19 Jul 2023 19:40:36 +0200 Subject: [PATCH 11/16] Update packages/mui-material/src/Tabs/Tabs.js Co-authored-by: Zeeshan Tamboli Signed-off-by: SaidMarar --- packages/mui-material/src/Tabs/Tabs.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 1b757632a37231..e09e5482bdb971 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -599,7 +599,10 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { React.useEffect(() => { let firstObserver; let lastObserver; - const { 0: firstTab, length, [length - 1]: lastTab } = Array.from(tabListRef.current.children); + const tabListChildren = Array.from(tabListRef.current.children); + const length = tabListChildren.length; + const firstTab = tabListChildren[0]; + const lastTab = tabListChildren[length - 1]; const threshold = 0.99; const observerOptions = { root: tabsRef.current, From 762c6db6322694a6dd89e26ca43258c9f0ead265 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Wed, 19 Jul 2023 20:09:11 +0200 Subject: [PATCH 12/16] remove unnecessary tests and keep updateScrollButtons impl --- packages/mui-material/src/Tabs/Tabs.js | 27 ++++++++- packages/mui-material/src/Tabs/Tabs.test.js | 61 +-------------------- 2 files changed, 28 insertions(+), 60 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index e09e5482bdb971..8673890d908fb9 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -559,6 +559,29 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { } }); + const updateScrollButtonState = useEventCallback(() => { + if (scrollable && scrollButtons !== false) { + const { scrollTop, scrollHeight, clientHeight, scrollWidth, clientWidth } = tabsRef.current; + let showStartScroll; + let showEndScroll; + + if (vertical) { + showStartScroll = scrollTop > 1; + showEndScroll = scrollTop < scrollHeight - clientHeight - 1; + } else { + const scrollLeft = getNormalizedScrollLeft(tabsRef.current, theme.direction); + // use 1 for the potential rounding error with browser zooms. + showStartScroll = isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; + showEndScroll = !isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; + } + + if (showStartScroll !== displayStartScroll || showEndScroll !== displayEndScroll) { + setDisplayStartScroll(showStartScroll); + setDisplayEndScroll(showEndScroll); + } + } + }); + React.useEffect(() => { const handleResize = debounce(() => { // If the Tabs component is replaced by Suspense with a fallback, the last @@ -663,9 +686,9 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { action, () => ({ updateIndicator: updateIndicatorState, - updateScrollButtons: () => {}, + updateScrollButtons: updateScrollButtonState, }), - [updateIndicatorState], + [updateIndicatorState, updateScrollButtonState], ); const indicator = ( diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index bd94ba4fbba57b..02af64ad2770e3 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -509,28 +509,6 @@ describe('', () => { expect(container.querySelectorAll(selector)).to.have.lengthOf(1); }); - it('should response to scroll events', async function test() { - if (isJSDOM) { - this.skip(); - } - const { container, getByRole } = render(tabs); - const tablistContainer = getByRole('tablist').parentElement; - - tablistContainer.scrollLeft = 10; - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); - }); - - tablistContainer.scrollLeft = 0; - fireEvent.scroll(container.querySelector(`.${classes.scroller}.${classes.scrollableX}`)); - - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); - }); - }); - it('should get a scrollbar size listener', () => { const { setProps, getByRole } = render( @@ -600,39 +578,8 @@ describe('', () => { expect(container.querySelectorAll(`.${classes.scrollButtonsHideMobile}`)).to.have.lengthOf(0); }); - it('should handle window resize event', async function test() { - if (isJSDOM) { - this.skip(); - } - const { container, getByRole } = render( - - - - - , - ); - - const tablistContainer = getByRole('tablist').parentElement; - - tablistContainer.scrollLeft = 10; - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(true); - expect(hasRightScrollButton(container)).to.equal(true); - }); - tablistContainer.scrollLeft = 0; - - act(() => { - window.dispatchEvent(new window.Event('resize', {})); - }); - - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); - }); - }); - describe('scroll button visibility states', () => { - it('should set neither left nor right scroll button state', async function test() { + it('should set neither left nor right scroll button state', function test() { if (isJSDOM) { this.skip(); } @@ -643,10 +590,8 @@ describe('', () => { , ); - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(false); - }); + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); }); it('should set only left scroll button state', async function test() { From 9666111d86c3e0457a24f351aded8dd5dc3b99dd Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Thu, 20 Jul 2023 23:34:49 +0200 Subject: [PATCH 13/16] add unit test for dynamic tabs in vertical mode --- packages/mui-material/src/Tabs/Tabs.test.js | 65 +++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index 02af64ad2770e3..ddffb49ecb29ea 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -1336,6 +1336,8 @@ describe('', () => { }); describe('dynamic tabs', () => { + const pause = (timeout) => new Promise((resolve) => setTimeout(resolve, timeout)); + it('should not show scroll buttons if a tab added or removed in auto width', async function test() { if (isJSDOM) { this.skip(); @@ -1416,5 +1418,68 @@ describe('', () => { expect(hasRightScrollButton(container)).to.equal(false); }); }); + + // https://github.com/mui/material-ui/issues/31936 + it('should not show scroll buttons if a tab added or removed in vertical mode', async function test() { + if (isJSDOM) { + this.skip(); + } + function DynamicTabs() { + const [value, setValue] = React.useState(0); + const handleChange = (event, newValue) => { + setValue(newValue); + }; + const [tabs, setTabs] = React.useState(['item1', 'item2']); + return ( + + + + + {tabs.map((label, index) => ( + + ))} + + + ); + } + const { container, getByTestId, getAllByRole } = render(); + const addButton = getByTestId('add'); + const deleteButton = getByTestId('delete'); + + fireEvent.click(addButton); + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + + const tabs = getAllByRole('tab'); + const lastTab = tabs[tabs.length - 1]; + fireEvent.click(lastTab); + await pause(400); + + fireEvent.click(deleteButton); + expect(hasLeftScrollButton(container)).to.equal(false); + expect(hasRightScrollButton(container)).to.equal(false); + }); }); }); From 2f0d80c3fcf85d5d4af927d9822020be0651faf4 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Fri, 21 Jul 2023 02:27:14 +0200 Subject: [PATCH 14/16] remove unnecessary tests --- packages/mui-material/src/Tabs/Tabs.test.js | 81 --------------------- 1 file changed, 81 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index ddffb49ecb29ea..9318c3ad0575e9 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -1338,87 +1338,6 @@ describe('', () => { describe('dynamic tabs', () => { const pause = (timeout) => new Promise((resolve) => setTimeout(resolve, timeout)); - it('should not show scroll buttons if a tab added or removed in auto width', async function test() { - if (isJSDOM) { - this.skip(); - } - - function DynamicTabs() { - const [tabs, setTabs] = React.useState(['item1', 'item2', 'item3']); - return ( - - - - - {tabs.map((label, index) => ( - - ))} - - - ); - } - const { container, getByTestId } = render(); - const addButton = getByTestId('add'); - const deleteButton = getByTestId('delete'); - - fireEvent.click(addButton); - fireEvent.click(addButton); - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(false); - }); - - fireEvent.click(deleteButton); - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(false); - }); - }); - - it('should show scroll buttons if a tab added or removed in fixed width', async function test() { - if (isJSDOM) { - this.skip(); - } - - function DynamicTabs() { - const [tabs, setTabs] = React.useState(['item1', 'item2']); - return ( - - - - - {tabs.map((label, index) => ( - - ))} - - - ); - } - const { container, getByTestId } = render(); - const addButton = getByTestId('add'); - const deleteButton = getByTestId('delete'); - - fireEvent.click(addButton); - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(true); - }); - - fireEvent.click(deleteButton); - await waitFor(() => { - expect(hasLeftScrollButton(container)).to.equal(false); - expect(hasRightScrollButton(container)).to.equal(false); - }); - }); - // https://github.com/mui/material-ui/issues/31936 it('should not show scroll buttons if a tab added or removed in vertical mode', async function test() { if (isJSDOM) { From ada70ecb2a3429570850aa479d560d8f897bc311 Mon Sep 17 00:00:00 2001 From: Said MARAR Date: Fri, 21 Jul 2023 02:54:04 +0200 Subject: [PATCH 15/16] fix lint --- packages/mui-material/src/Tabs/Tabs.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/mui-material/src/Tabs/Tabs.test.js b/packages/mui-material/src/Tabs/Tabs.test.js index 9318c3ad0575e9..03f0be8f8bdb14 100644 --- a/packages/mui-material/src/Tabs/Tabs.test.js +++ b/packages/mui-material/src/Tabs/Tabs.test.js @@ -1336,7 +1336,12 @@ describe('', () => { }); describe('dynamic tabs', () => { - const pause = (timeout) => new Promise((resolve) => setTimeout(resolve, timeout)); + const pause = (timeout) => + new Promise((resolve) => { + setTimeout(() => { + resolve(); + }, timeout); + }); // https://github.com/mui/material-ui/issues/31936 it('should not show scroll buttons if a tab added or removed in vertical mode', async function test() { From 60790541895c5d820e287aea8b38c1759fbe6915 Mon Sep 17 00:00:00 2001 From: Said Marar Date: Fri, 21 Jul 2023 20:34:56 +0200 Subject: [PATCH 16/16] updateScrollButtons action use IntersectionObserver implementation effect --- packages/mui-material/src/Tabs/Tabs.js | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js index 8673890d908fb9..c68f8518422260 100644 --- a/packages/mui-material/src/Tabs/Tabs.js +++ b/packages/mui-material/src/Tabs/Tabs.js @@ -314,6 +314,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { const [indicatorStyle, setIndicatorStyle] = React.useState(defaultIndicatorStyle); const [displayStartScroll, setDisplayStartScroll] = React.useState(false); const [displayEndScroll, setDisplayEndScroll] = React.useState(false); + const [updateScrollObserver, setUpdateScrollObserver] = React.useState(false); const [scrollerStyle, setScrollerStyle] = React.useState({ overflow: 'hidden', @@ -561,24 +562,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { const updateScrollButtonState = useEventCallback(() => { if (scrollable && scrollButtons !== false) { - const { scrollTop, scrollHeight, clientHeight, scrollWidth, clientWidth } = tabsRef.current; - let showStartScroll; - let showEndScroll; - - if (vertical) { - showStartScroll = scrollTop > 1; - showEndScroll = scrollTop < scrollHeight - clientHeight - 1; - } else { - const scrollLeft = getNormalizedScrollLeft(tabsRef.current, theme.direction); - // use 1 for the potential rounding error with browser zooms. - showStartScroll = isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; - showEndScroll = !isRtl ? scrollLeft < scrollWidth - clientWidth - 1 : scrollLeft > 1; - } - - if (showStartScroll !== displayStartScroll || showEndScroll !== displayEndScroll) { - setDisplayStartScroll(showStartScroll); - setDisplayEndScroll(showEndScroll); - } + setUpdateScrollObserver(!updateScrollObserver); } }); @@ -667,7 +651,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { firstObserver?.disconnect(); lastObserver?.disconnect(); }; - }, [scrollable, scrollButtons, childrenProp?.length]); + }, [scrollable, scrollButtons, updateScrollObserver, childrenProp?.length]); React.useEffect(() => { setMounted(true);