material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Tabs] Fix and improve visibility of scrollButtons using IntersectionObserver

Open SaidMarar opened this issue 2 years ago • 5 comments

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

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-zej36n?file=/src/App.js

SaidMarar avatar Feb 05 '23 22:02 SaidMarar

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 :no_entry_sign: dangerJS against ca7b7b287498071b10268f96c3bd522a6aa8412f

mui-bot avatar Feb 05 '23 22:02 mui-bot

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 avatar Feb 14 '23 23:02 SaidMarar

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

SaidMarar avatar Feb 22 '23 15:02 SaidMarar

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

JoinerDev avatar Mar 23 '23 10:03 JoinerDev

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

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

SaidMarar avatar Mar 24 '23 11:03 SaidMarar

Hello @ZeeshanTamboli , Thank you a lot for your review :smile:

  1. 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:

      dev

    • Prod:

      prod

  2. 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 avatar Jun 24 '23 18:06 SaidMarar

PR is ready, @ZeeshanTamboli review is welcome :smiley:

SaidMarar avatar Jun 26 '23 21:06 SaidMarar

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.

ZeeshanTamboli avatar Jun 30 '23 13:06 ZeeshanTamboli

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

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

SaidMarar avatar Jun 30 '23 15:06 SaidMarar

My screen resolution is 1920px 1080px.

ZeeshanTamboli avatar Jul 01 '23 05:07 ZeeshanTamboli

@ZeeshanTamboli it works also on iPhone.

https://github.com/mui/material-ui/assets/20561190/cecbef2f-0000-4b3e-b9e4-41c9b1efb869

SaidMarar avatar Jul 01 '23 09:07 SaidMarar

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

SaidMarar avatar Jul 01 '23 13:07 SaidMarar

On Windows 10, Chrome Browser:

https://github.com/mui/material-ui/assets/20900032/543e8be9-e7cb-4352-a1b3-639b9baf970a


On Android Chrome:

https://github.com/mui/material-ui/assets/20900032/eed7564f-57cb-4c5d-aa6b-c264631e49a0

ZeeshanTamboli avatar Jul 03 '23 13:07 ZeeshanTamboli

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

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

SaidMarar avatar Jul 06 '23 18:07 SaidMarar

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

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

@SaidMarar

https://github.com/mui/material-ui/assets/20900032/c48b1797-a020-4dd5-8099-864a4ee6dd3d

ZeeshanTamboli avatar Jul 07 '23 13:07 ZeeshanTamboli

Thank you, @ZeeshanTamboli. It was very helpful, and I was able to reproduce it in both Chrome and Firefox (on windows) and it happens only in 1920 resolution. The problem persists even when we reach the maximum value of scrollLeft (the max value: scrollWidth - clientWidth). The last item on the x-axis always remains higher than its parent.

image

And this is a known issue and it was reported on chromium too, as you can see in the example the scrollLeft is returning a decimal value which is a bug.

For firefox i have doubts they are rounding the scrollLeft but the issue of boundingRect stays the same as chromium.

Possible solution is to use 0.99 as threshold, i tested it on local it resolved the problem.

SaidMarar avatar Jul 09 '23 20:07 SaidMarar

@ZeeshanTamboli could you please test again ? :)

SaidMarar avatar Jul 12 '23 18:07 SaidMarar

The latest updates on your projects. Learn more about Argos notifications ↗︎

Waiting for the first build to start…

argos-ci[bot] avatar Jul 16 '23 17:07 argos-ci[bot]

@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 ?

SaidMarar avatar Jul 21 '23 17:07 SaidMarar

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?

ZeeshanTamboli avatar Jul 21 '23 17:07 ZeeshanTamboli

@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 ?

SaidMarar avatar Jul 21 '23 18:07 SaidMarar

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?

ZeeshanTamboli avatar Jul 23 '23 07:07 ZeeshanTamboli

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.

SaidMarar avatar Jul 23 '23 11:07 SaidMarar

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

Thanks to you too for your assistance and help 👍🏻

SaidMarar avatar Jul 23 '23 12:07 SaidMarar