[Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API #36071

SaidMarar · 2023-02-05T22:49:00Z

Fixes the issues below:
Fixes #31936
Fixes #19673
Fixes #33349
Fixes #23711
Fixes #38035

This fix handle visibility of scroll buttons specially in dynamic content behavior and also it reduces the complexity of the code (calculation logic).

before: https://codesandbox.io/s/interesting-before-b3kdvn?file=/src/App.js
after: https://codesandbox.io/s/interesting-after-zf7ryz

I have followed (at least) the PR section of the contributing guide.

https://deploy-preview-36071--material-ui.netlify.app/material-ui/react-tabs/

…Observer

mui-bot · 2023-02-05T22:51:40Z

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ca7b7b2

SaidMarar · 2023-02-14T23:03:45Z

I've implemented a new method to handle the visibility of scroll buttons, using the IntersectionObserver. Essentially, it allows us to detect when a particular element (in this case, a tab) is in view on the screen.

With this new method, we're observing only the first and last tabs. If either the first or last tab is partially visible or completely invisible, we'll show the appropriate scroll button. For example, if the first tab is only partially visible,we'll show the scroll button that allows the user to scroll to the left and reveal more of the first tab. If the last tab is completely invisible, we'll show the scroll button that allows the user to scroll to the right and reveal the last tab.

By using the IntersectionObserver to only observe the first and last tabs, we're optimizing our code and reducing unnecessary calculations especially in dynamic content scenarios. This new method, the code should be easier to read and maintain.

SaidMarar · 2023-02-22T15:49:55Z

Hi @mnajdova if you have a moment, could you kindly take a look and review this PR please.

JoinerDev · 2023-03-23T10:59:31Z

@mnajdova @SaidMarar Are there any updates on this issue?

SaidMarar · 2023-03-24T11:06:44Z

@mnajdova @SaidMarar Are there any updates on this issue?

@JoinerDev the PR needs only to be reviewed, so we can advance.

ZeeshanTamboli

@SaidMarar Thanks for the PR.

In the demos, when scroll you to the extreme right (ITEM 7), the right scroll button is still available, whereas in production, it hides the scroll button.

Besides, I merged with the latest master branch for easier review.

Also, argos detected visual changes. What happens is that on the initial render, the tabs are in a different position, and then they shift. Argos captures the screenshot on the initial mount. You can reproduce it on the page refresh. This should not happen.

SaidMarar · 2023-06-24T18:52:26Z

Hello @ZeeshanTamboli , Thank you a lot for your review 😄

For the first issue you mentioned in the demos, i don't reproduce it with the latest changes, i have the same behavior as the one in prod. Please check the screenrecords below:
- Dev:
- Prod:
For the second issue in Argos, i just fixed it by removing debounce on both observers, because it causes a delay on render for the buttons.

SaidMarar · 2023-06-26T21:30:29Z

PR is ready, @ZeeshanTamboli review is welcome 😃

ZeeshanTamboli · 2023-06-30T13:44:12Z

For the first issue you mentioned in the demos, i don't reproduce it with the latest changes, i have the same behavior as the one in prod.

I can still see it. Can you tell me your operating system, browser, and its version? I'm using Windows 10 and Chrome Version 114.0.5735.199 (64-bit). I am also seeing it on Firefox.

SaidMarar · 2023-06-30T15:16:50Z

@ZeeshanTamboli Could you please post your resolutions, my environment test on Ubuntu 22.04:

Firefox 115.0 (64 bits), resolution 1920px 995px
Chrome Version 114.0.5735.198 (Build officiel) (64 bits), resolution 1920px 995px

ZeeshanTamboli · 2023-07-01T05:31:55Z

My screen resolution is 1920px 1080px.

SaidMarar · 2023-07-01T09:57:31Z

@ZeeshanTamboli it works also on iPhone.

trim.43D8479F-1641-4A24-9465-EDD23FC097A6.MOV

SaidMarar · 2023-07-01T13:22:52Z

@ZeeshanTamboli can you screenrecord,maybe it will help me to reproduce.

ZeeshanTamboli · 2023-07-03T13:28:29Z

On Windows 10, Chrome Browser:

03.07.2023_18.44.36_REC.mp4

On Android Chrome:

Screenrecorder-2023-07-03-18-53-20-92.mp4

SaidMarar · 2023-07-06T18:18:46Z

@ZeeshanTamboli When you reach the last element could you please print the getBoundingClientRect (value of the attribute right) of:

The last element in tabs the button (item seven)
The element with class MuiTabs-scroller

packages/mui-material/src/Tabs/Tabs.js

packages/mui-material/src/Tabs/Tabs.test.js

Co-authored-by: Zeeshan Tamboli <[email protected]> Signed-off-by: SaidMarar <[email protected]>

packages/mui-material/src/Tabs/Tabs.test.js

SaidMarar · 2023-07-21T17:20:44Z

@ZeeshanTamboli if updateScrollButtons will remain accessible for developers who are relying on it, why we dont just change its old implementation to use IntersectionObserverAPI. The code will be simpler.

we create a state that is responsible to re-init the observer:

const [updateScrollObserver, setUpdateScrollObserver] = React.useState(false);

we change the updateScrollButtonState implementation to this:

  const updateScrollButtonState = useEventCallback(() =>
    setUpdateScrollObserver(!updateScrollObserver),
  );

Finally we just append the Observer useEffect dependencies:

[scrollable, scrollButtons, updateScrollObserver, childrenProp?.length];

What do you think ?

ZeeshanTamboli · 2023-07-21T17:37:50Z

I think I understand what you mean. Whenever the updateScrollButtons action is triggered by the developer, you want to run the IntersectionObserver i.e the useEffect? Sounds like a good idea. Feel free to apply the changes. But when will you reset this scroll observer state?

SaidMarar · 2023-07-21T18:08:55Z

@ZeeshanTamboli Yes the IntersectionObserver effect will run, when the developer triggers updateScrollButtons action.
Sorry, I don't understand what you mean by when i will reset the scroll observer state ?

…fect

ZeeshanTamboli · 2023-07-23T07:29:39Z

Sorry, I don't understand what you mean by when i will reset the scroll observer state ?

Does the updateScrollObserver state, triggered by updateScrollButtons action, reset to false after the effect finishes running?

packages/mui-material/src/Tabs/Tabs.js

SaidMarar · 2023-07-23T11:09:10Z

Does the updateScrollObserver state, triggered by updateScrollButtons action, reset to false after the effect finishes running?

No need to reset, because the updateScrollObserver state is used only to be toggled every time we call updateScrollButtons, ensuring the re-run of the effect, which means rebinding the IntersectionObserver to detect the visibility of buttons.

ZeeshanTamboli

@SaidMarar It looks good! Thanks for your great contribution!

SaidMarar · 2023-07-23T12:38:02Z

@SaidMarar It looks good! Thanks for your great contribution!

Thanks to you too for your assistance and help 👍🏻

oliviertassinari · 2023-07-23T13:18:53Z

packages/mui-material/src/Tabs/Tabs.test.js

+      const tabs = getAllByRole('tab');
+      const lastTab = tabs[tabs.length - 1];
+      fireEvent.click(lastTab);
+      await pause(400);


We never use the real-time in these tests, to revert back to mocking time, for higher control and speed.

Suggested change

await pause(400);

I couldn't make the test cover that bug without this, but I'm aware of it. Do you have any alternative solutions by faking the time?

Same i tried requestAnimationFrame but it doesn't work so i did the pause(400)

@oliviertassinari Another way to do it ?

I tried to simulate the unit test for the old implementation version 5.14.1, and i see that scrollButton is blinking when we delete a tab programmatically, it is not acting like a real user scenario. please see the codesandbox without the setTimeout you will not see the bug line 27.

I had a look, my process was to: 1. revert all the test changes 2. focus on one failing test, 3. make it pass. Here is what I come up with:

oliviertassinari@76209fd#diff-1d60104297e2e5decebdb9cef7bdbf776affd72d75fa7f263d2d5e387358499aR673

We should be able to fix all the other tests like this. I removed the time mocking in this instance, as it wasn't relevant to the test (what we need to depend on is not time but rafs)

Could we fix the rest? Thanks!

what we need to depend on is not time but rafs

Is it because waitFor should be used only in JSDom, and requestAnimationFrame is used in the browser environment?

what we need to depend on is not time but rafs

Is it because waitFor should be used only in JSDom, and requestAnimationFrame is used in the browser environment?

Other tests i used the raf and it worked, but for this specific unit test i don't know why we must delay the delete click.

@oliviertassinari @ZeeshanTamboli
Created a PR for this #38208.

I revert to the old implementation of unit tests then I refacto + added the last unit that fails in old implementation of Tabs before IntersectionObserver.

The last unit test i bind event transitionend before delete a tab, i don't know if it is a good solution than the setimeout.

oliviertassinari · 2023-07-23T13:28:00Z

packages/mui-material/src/Tabs/Tabs.test.js

-    it('should response to scroll events', function test() {
-      if (isJSDOM) {
-        this.skip();
-      }
-      const { container, forceUpdate, getByRole } = render(tabs);
-      const tablistContainer = getByRole('tablist').parentElement;
-
-      Object.defineProperty(tablistContainer, 'clientWidth', { value: 200 - 40 * 2 });
-      tablistContainer.scrollLeft = 10;
-      Object.defineProperty(tablistContainer, 'scrollWidth', { value: 216 });
-      Object.defineProperty(tablistContainer, 'getBoundingClientRect', {
-        value: () => ({
-          left: 0,
-          right: 50,
-        }),
-      });
-      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);
-    });


Did we lose test coverage for this?

Scroll event is not being triggered now. Is it still relevant? #36071 (comment)

Created a PR for this #38208.

packages/mui-material/src/Tabs/Tabs.js

[Tabs] Fix and improve visibility of scrollButtons using Intersection…

f9c6983

…Observer

SaidMarar marked this pull request as draft February 6, 2023 01:35

oliviertassinari added the component: tabs This is the name of the generic UI component, not the React module! label Feb 9, 2023

SaidMarar added 4 commits February 12, 2023 20:23

Merge branch 'master' into fix/scroll-buttons-tabs

a2e8573

refacto observer intersection and fix e2e test

62e219e

add e2e test for dynamic tabs

c2bc67d

fix lint prettier

55359fd

SaidMarar marked this pull request as ready for review February 14, 2023 23:05

zannager requested a review from mnajdova February 15, 2023 08:29

Merge branch 'master' into fix/scroll-buttons-tabs

8b3183b

Merge branch 'master' into fix/scroll-buttons-tabs

b21af61

ZeeshanTamboli requested changes May 3, 2023

View reviewed changes

ZeeshanTamboli added the bug 🐛 Something doesn't work label May 3, 2023

fix delay render of scroll buttons

56804c2

SaidMarar requested a review from ZeeshanTamboli June 25, 2023 13:22

use builtin waitFor

bc978f6

ZeeshanTamboli reviewed Jul 19, 2023

View reviewed changes

packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved

ZeeshanTamboli reviewed Jul 19, 2023

View reviewed changes

ZeeshanTamboli mentioned this pull request Jul 19, 2023

[Tabs] Auto scroll buttons aren't updated automatically when resized #38035

Closed

2 tasks

SaidMarar and others added 2 commits July 19, 2023 19:40

Update packages/mui-material/src/Tabs/Tabs.js

5d65ce8

Co-authored-by: Zeeshan Tamboli <[email protected]> Signed-off-by: SaidMarar <[email protected]>

remove unnecessary tests and keep updateScrollButtons impl

762c6db

ZeeshanTamboli reviewed Jul 20, 2023

View reviewed changes

packages/mui-material/src/Tabs/Tabs.test.js Show resolved Hide resolved

SaidMarar and others added 4 commits July 20, 2023 23:34

add unit test for dynamic tabs in vertical mode

9666111

remove unnecessary tests

2f0d80c

fix lint

ada70ec

Merge branch 'master' into fix/scroll-buttons-tabs

18c81f2

updateScrollButtons action use IntersectionObserver implementation ef…

6079054

…fect

ZeeshanTamboli reviewed Jul 23, 2023

View reviewed changes

packages/mui-material/src/Tabs/Tabs.js Show resolved Hide resolved

Merge branch 'master' into fix/scroll-buttons-tabs

6f16162

Merge branch 'master' into fix/scroll-buttons-tabs

ca7b7b2

ZeeshanTamboli added the package: material-ui Specific to @mui/material label Jul 23, 2023

ZeeshanTamboli approved these changes Jul 23, 2023

View reviewed changes

ZeeshanTamboli changed the title ~~[Tabs] Fix and improve visibility of scrollButtons using IntersectionObserver~~ [Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API Jul 23, 2023

ZeeshanTamboli merged commit 569a43e into mui:master Jul 23, 2023

oliviertassinari reviewed Jul 23, 2023

View reviewed changes

ZeeshanTamboli mentioned this pull request Jul 24, 2023

[Tabs] Refactor IntersectionObserver logic #38133

Merged

1 task

SaidMarar mentioned this pull request Jul 28, 2023

[Tabs] revert and refacto tabs unit test #38208

Closed

1 task

oliviertassinari mentioned this pull request Nov 22, 2023

[tabs] After removing tab scrollable mode is triggered (bad behavior of scrollable mode in case of dynamic tabs) #39394

Open

kealjones-wk mentioned this pull request Jan 19, 2024

[material-ui][Tabs] End scroll button overlaps last tab when it is the initial value #40692

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API #36071

[Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API #36071

SaidMarar commented Feb 5, 2023 •

edited by oliviertassinari

Loading

mui-bot commented Feb 5, 2023 •

edited

Loading

SaidMarar commented Feb 14, 2023

SaidMarar commented Feb 22, 2023

JoinerDev commented Mar 23, 2023 •

edited

Loading

SaidMarar commented Mar 24, 2023

ZeeshanTamboli left a comment •

edited

Loading

SaidMarar commented Jun 24, 2023 •

edited

Loading

SaidMarar commented Jun 26, 2023 •

edited

Loading

ZeeshanTamboli commented Jun 30, 2023 •

edited

Loading

SaidMarar commented Jun 30, 2023 •

edited

Loading

ZeeshanTamboli commented Jul 1, 2023

SaidMarar commented Jul 1, 2023

SaidMarar commented Jul 1, 2023

ZeeshanTamboli commented Jul 3, 2023

SaidMarar commented Jul 6, 2023 •

edited

Loading

SaidMarar commented Jul 21, 2023 •

edited

Loading

ZeeshanTamboli commented Jul 21, 2023

SaidMarar commented Jul 21, 2023

ZeeshanTamboli commented Jul 23, 2023

SaidMarar commented Jul 23, 2023

ZeeshanTamboli left a comment

SaidMarar commented Jul 23, 2023

oliviertassinari Jul 23, 2023 •

edited

Loading

ZeeshanTamboli Jul 23, 2023

SaidMarar Jul 23, 2023

SaidMarar Jul 23, 2023 •

edited

Loading

oliviertassinari Jul 24, 2023

ZeeshanTamboli Jul 26, 2023

SaidMarar Jul 26, 2023

SaidMarar Jul 28, 2023 •

edited

Loading

oliviertassinari Jul 23, 2023

ZeeshanTamboli Jul 26, 2023 •

edited

Loading

SaidMarar Jul 28, 2023

[Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API #36071

[Tabs] Fix and improve visibility of tab scroll buttons using the IntersectionObserver API #36071

Conversation

SaidMarar commented Feb 5, 2023 • edited by oliviertassinari Loading

mui-bot commented Feb 5, 2023 • edited Loading

Netlify deploy preview

Bundle size report

SaidMarar commented Feb 14, 2023

SaidMarar commented Feb 22, 2023

JoinerDev commented Mar 23, 2023 • edited Loading

SaidMarar commented Mar 24, 2023

ZeeshanTamboli left a comment • edited Loading

Choose a reason for hiding this comment

SaidMarar commented Jun 24, 2023 • edited Loading

SaidMarar commented Jun 26, 2023 • edited Loading

ZeeshanTamboli commented Jun 30, 2023 • edited Loading

SaidMarar commented Jun 30, 2023 • edited Loading

ZeeshanTamboli commented Jul 1, 2023

SaidMarar commented Jul 1, 2023

SaidMarar commented Jul 1, 2023

ZeeshanTamboli commented Jul 3, 2023

SaidMarar commented Jul 6, 2023 • edited Loading

SaidMarar commented Jul 21, 2023 • edited Loading

ZeeshanTamboli commented Jul 21, 2023

SaidMarar commented Jul 21, 2023

ZeeshanTamboli commented Jul 23, 2023

SaidMarar commented Jul 23, 2023

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

SaidMarar commented Jul 23, 2023

oliviertassinari Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Jul 23, 2023

Choose a reason for hiding this comment

SaidMarar Jul 23, 2023

Choose a reason for hiding this comment

SaidMarar Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jul 24, 2023

Choose a reason for hiding this comment

ZeeshanTamboli Jul 26, 2023

Choose a reason for hiding this comment

SaidMarar Jul 26, 2023

Choose a reason for hiding this comment

SaidMarar Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jul 23, 2023

Choose a reason for hiding this comment

ZeeshanTamboli Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

SaidMarar Jul 28, 2023

Choose a reason for hiding this comment

SaidMarar commented Feb 5, 2023 •

edited by oliviertassinari

Loading

mui-bot commented Feb 5, 2023 •

edited

Loading

JoinerDev commented Mar 23, 2023 •

edited

Loading

ZeeshanTamboli left a comment •

edited

Loading

SaidMarar commented Jun 24, 2023 •

edited

Loading

SaidMarar commented Jun 26, 2023 •

edited

Loading

ZeeshanTamboli commented Jun 30, 2023 •

edited

Loading

SaidMarar commented Jun 30, 2023 •

edited

Loading

SaidMarar commented Jul 6, 2023 •

edited

Loading

SaidMarar commented Jul 21, 2023 •

edited

Loading

oliviertassinari Jul 23, 2023 •

edited

Loading

SaidMarar Jul 23, 2023 •

edited

Loading

SaidMarar Jul 28, 2023 •

edited

Loading

ZeeshanTamboli Jul 26, 2023 •

edited

Loading