microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.33k stars 2.72k forks source link

Issue with Scrollable component when scroll bar is at bottom of page #16654

Closed gadusumi closed 11 months ago

gadusumi commented 3 years ago

Environment Information

Package version(s): 7.156 (also reproducible in 7.149 and in 8.111.2) Browser and OS versions: Chrome

reproduction of the bug in a codepen: : https://codepen.io/goutham-a/pen/JjRVXOJ

Issue : Scrollable component not working as expected at 80% screen width Description: When we scroll to the end of the screen using mouse in chrome(all recent versions),a blank screen is displayed and data in details list is lost. see error with "maximum update depth exceeded" with scrollable component(on debugging). probably an infinite loop is happening at set state. And I see this issue only at 80% screen size(or zoom) in chrome.It works perfectly fine in all other zoom percentage. I am easily able to replicate this with codepen using Fluentui samples(examples)

Actual behavior: Scroll bar should work even after scroll is at the bottom at all screen zoom percent\size

Expected behavior: Once the user scrolls to the bottom of the page and user clicks on mouse scroll, data is lost and empty page is displayed- at 80% screen size\zoom

Priorities and help requested:

Requested priority: Blocking

Products/sites affected: We have implemented controls using scrollable component and power apps component framework and this is failing.

gadusumi commented 3 years ago

Can you please triage this or help me if there are any alternate solutions? thanks!!

gadusumi commented 3 years ago

@layershifter - can you please provide an update if any of the team is looking into this issue. please let me know if you need any information pertaining to this. thank you!!

JustSlone commented 3 years ago

Confirmed repro in chrome and new Edge.

Only repros if the mouse is over a row while scrolling to the end. If the mouse is over the whitespace to the right it does not repro for me.

Error is:

react-dom.development.js:49 Uncaught Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at invariant (https://unpkg.com/react-dom@16.8.6/umd/react-dom.development.js:49:15)
    at scheduleWork (https://unpkg.com/react-dom@16.8.6/umd/react-dom.development.js:20051:5)
    at Object.enqueueSetState (https://unpkg.com/react-dom@16.8.6/umd/react-dom.development.js:11304:5)
    at Sticky.Component.setState (https://unpkg.com/react@16.8.6/umd/react.development.js:423:16)
    at Sticky._this._onScrollEvent (https://unpkg.com/@fluentui/react@7/dist/fluentui-react.js:49869:23)
    at https://unpkg.com/@fluentui/react@7/dist/fluentui-react.js:45160:21
    at Set.forEach (<anonymous>)
    at ScrollablePaneBase._this.notifySubscribers (https://unpkg.com/@fluentui/react@7/dist/fluentui-react.js:45158:36)
    at ScrollablePaneBase.FluentUIReact.../office-ui-fabric-react/lib/components/ScrollablePane/ScrollablePane.base.js.ScrollablePaneBase.componentDidUpdate (https://unpkg.com/@fluentui/react@7/dist/fluentui-react.js:45416:18)
    at https://unpkg.com/@fluentui/react@7/dist/fluentui-react.js:70059:56
invariant @ react-dom.development.js:49
scheduleWork @ react-dom.development.js:20051
enqueueSetState @ react-dom.development.js:11304
Component.setState @ react.development.js:423
Sticky._this._onScrollEvent @ Sticky.tsx:288
(anonymous) @ ScrollablePane.base.tsx:296
ScrollablePaneBase._this.notifySubscribers @ ScrollablePane.base.tsx:294
FluentUIReact.../office-ui-fabric-react/lib/components/ScrollablePane/ScrollablePane.base.js.ScrollablePaneBase.componentDidUpdate @ ScrollablePane.base.tsx:182
(anonymous) @ appendFunction.ts:13
(anonymous) @ appendFunction.ts:13
commitLifeCycles @ react-dom.development.js:17484
commitAllLifeCycles @ react-dom.development.js:18871
callCallback @ react-dom.development.js:143
invokeGuardedCallbackDev @ react-dom.development.js:193
invokeGuardedCallback @ react-dom.development.js:250
commitRoot @ react-dom.development.js:19083
(anonymous) @ react-dom.development.js:20553
unstable_runWithPriority @ react.development.js:735
completeRoot @ react-dom.development.js:20552
performWorkOnRoot @ react-dom.development.js:20481
performWork @ react-dom.development.js:20389
performSyncWork @ react-dom.development.js:20363
requestWork @ react-dom.development.js:20232
scheduleWork @ react-dom.development.js:20046
enqueueSetState @ react-dom.development.js:11304
Component.setState @ react.development.js:423
Sticky._this._onScrollEvent @ Sticky.tsx:288
(anonymous) @ ScrollablePane.base.tsx:296
ScrollablePaneBase._this.notifySubscribers @ ScrollablePane.base.tsx:294
callback @ Async.ts:280
(anonymous) @ Async.ts:110
setTimeout (async)
FluentUIReact.../utilities/lib/Async.js.Async.setTimeout @ Async.ts:102
callback @ Async.ts:282
resultFunction @ Async.ts:291
ScrollablePaneBase._this._onScroll @ ScrollablePane.base.tsx:478
processElementEvent @ EventGroup.ts:208
react_devtools_backend.js:2430 The above error occurred in the <ScrollablePaneBase> component:
    in ScrollablePaneBase (created by StyledScrollablePaneBase)
    in StyledScrollablePaneBase (created by ScrollablePaneDetailsListExample)
    in div (created by ScrollablePaneDetailsListExample)
    in ScrollablePaneDetailsListExample (created by ScrollablePaneDetailsListExampleWrapper)
    in div (created by FabricBase)
    in FabricBase (created by StyledFabricBase)
    in StyledFabricBase (created by ScrollablePaneDetailsListExampleWrapper)
    in ScrollablePaneDetailsListExampleWrapper

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
JustSlone commented 3 years ago

@khmakoto, can you take a look at this?

khmakoto commented 3 years ago

Yes, I'll take a look.

khmakoto commented 3 years ago

@gadusumi @JustSlone unfortunately this doesn't seem to repro for me? I don't know if there's anything else to do other than set zoom at 80% and scroll to the end in the codepen but if that's it then I can't get it to repro on my setup.

mtsamis2 commented 3 years ago

@khmakoto I'm still able to reproduce this issue in the codepen provided. Try scrolling up and down multiple times via dragging the scrollbar up and down.

khmakoto commented 3 years ago

@mtsamis2 as you can see in the GIF below, I'm still unable to repro this:

Scrollable

mtsamis2 commented 3 years ago

@khmakoto Interesting. Here is a screenshot with the page crashing and the console error: image Let me know if there is any additional information I can provide that could assist in debugging.

JustSlone commented 3 years ago

Hmm, thought this wasn't repro'ing for me for a minute, but I can confirm it is still repro'ing.

JustSlone commented 3 years ago

A little investigation here (for others or maybe just my future self 😄 )

This isn't 100% confirmed, working on verifying.

onScrollEvent (or notifySubscribers): https://github.com/microsoft/fluentui/blob/c7630ee4fef42b8197929dd943695ff3876eaeaa/packages/react/src/components/Sticky/Sticky.tsx#L251

the code calls setState: https://github.com/microsoft/fluentui/blob/c7630ee4fef42b8197929dd943695ff3876eaeaa/packages/react/src/components/Sticky/Sticky.tsx#L280-L284

Which causes ScrollablePane to update. This check is not true prevState.stickyBottomHeight !== this.state.stickyBottomHeight https://github.com/microsoft/fluentui/blob/47b42efbbe9e869864973f3b2cb717e6112b8106/packages/react/src/components/ScrollablePane/ScrollablePane.base.tsx#L177-L183

Which results in the original setState being called again and so on, until hit the react infinite call depth check and 💣

In particular stickyBottomHeight is alternating between 0 and 44.

>this.state
{stickyTopHeight: 60, stickyBottomHeight: 0, scrollbarWidth: 18, scrollbarHeight: 0}
>prevState
{stickyTopHeight: 60, stickyBottomHeight: 44, scrollbarWidth: 18, scrollbarHeight: 0}

>this.state
{stickyTopHeight: 60, stickyBottomHeight: 44, scrollbarWidth: 18, scrollbarHeight: 0}
>prevState
{stickyTopHeight: 60, stickyBottomHeight: 0, scrollbarWidth: 18, scrollbarHeight: 0}

also the Sticky seems to be bouncing between the top and bottom of the container

>_this.state
{isStickyTop: true, isStickyBottom: false, distanceFromTop: 0}

>_this.state
{isStickyTop: false, isStickyBottom: false, distanceFromTop: 8710}

More investigation is needed to understand why this happens at 80% and what the proper fix is.

t-w-in commented 3 years ago

We also suffer from this :( Any workarounds?

dps88 commented 3 years ago

@JustSlone In our particular use case, We happen to have same issue but independent of the 80% zoom. We have a Scrollable Pane inside another Scrollable Pane which intermittently throws the same error when scrolling around.

I have never been able to consistently reproduce it, but can confirm that it does happen here without the odd zoom case.

Hopefully, this will be useful. The infinite call check you mentioned seems most likely.

Below is a quick outline of our scrollable pane setups. image

JustSlone commented 3 years ago

If anyone is able to get this reproing with a more minimal repro, that doesn't involve DetailsList that would help with the investigation

lishibo commented 3 years ago

We are also suffering from this. In our case, we have a ScrollablePane with ShimmeredDetailsList. Like others I cannot consistently repro, but it happens pretty frequently.

szmarci commented 3 years ago

We are also experiencing this issue, when zoom level is either 80 or below. We have a ScrollablePane, inside there is a DetailsList component with sticky header & footer.

bittola commented 2 years ago

Our customers are also experiencing the same problem. Is there any workaround ?

JeffJankowski commented 2 years ago

We are also getting this bug sporadically without a consistent reproduction, and it's affecting our production clients. From the conversation on this thread, I can confirm that the crash can be forced by setting Chrome on Win10 to 80% zoom and scrolling.

Please triage as there is no current workaround.

Edit: Looks like @JustSlone investigation is correct (or at least on the right track) as I cannot force a crash if not rendering a footer.

JeffJankowski commented 2 years ago

After some debugging of the source, I think I was able to track down the problem.

As @JustSlone noted, the footer sticky height is alternating between 0 and a positive value (44 for me as well). This is because the footer-sticky's state boolean isStickyBottom is also alternating, so the offsetHeight is not always being accumulated: https://github.com/microsoft/fluentui/blob/47b42efbbe9e869864973f3b2cb717e6112b8106/packages/react/src/components/ScrollablePane/ScrollablePane.base.tsx#L284-L285

The reason isStickyBottom is not stable can be tracked to Sticky's _onScrollEvent: https://github.com/microsoft/fluentui/blob/47b42efbbe9e869864973f3b2cb717e6112b8106/packages/react/src/components/Sticky/Sticky.tsx#L273-L276

The value returned by _getStickyDistanceFromTopForFooter is unstable by 1px (at least at 80% zoom) causing the condition flip-flop. Inspecting the variables during scrolling shows that the instability comes from footerStickyVisibleContainer.offsetHeight. offsetHeight is a rounded value (snapped to nearest pixel), which I guess becomes a problem when the actual height wanders away from an integer value. We can use getBoundingClientRect().height and see that the fractional value is shifting below and above the rounding midpoint.

Why the actual height is shifting, I'm not sure. I would assume it has to do with the browser rendering engine and not something like fluent's styling code. If we make the following change we can prevent the crash:

distance = container.clientHeight - 
  Math.floor(footerStickyVisibleContainer.getBoundingClientRect().height) + 
  this.stickyContentBottom.offsetTop;

That being said this may have performance implications since I believe getBoundingClientRect causes a reflow. This may be better suited to IntersectionObserver, throttling the scroll event, or something more clever I'm not thinking of.

Sukesh-Gundoji commented 2 years ago

Refreshing the thread as our application is also facing page crash issue due to this component usage. Can you please provide a workaround @JeffJankowski , @JustSlone

JustSlone commented 1 year ago

@spmonahan can you take a look at this complex issue when you get a chance. This is mainly in a DetailsList scenario, however, the code is not in DetailsList itself.

JeffJankowski commented 1 year ago

Bumping this as we're still getting reports from users that are getting this crash with our application. What's strange is that I can no longer reproduce it on my end. My environment is similar to a problematic user's (win10, chrome 111.0.x).

Can anyone (@gadusumi @JustSlone) still consistently repro this?

JustSlone commented 1 year ago

I no longer can reproduce this in Edge 112.0.1722.7 (Win11)

Perhaps it was fixed in recent browsers? @JeffJankowski you say it's reproing for a customer on Chrome 111.0.x? @spmonahan can you see if there might have been a fix in v7 that fixed this?

spmonahan commented 1 year ago

I'm working off the assumption that @JustSlone and @JeffJankowski's analysis is correct and the issue is with Sticky/Scrollable pane because after reviewing it seems correct to me.

The last edits to Sticky.tsx and ScrollablePane happened in February 2021, after this bug report but the bug was reproducible after this change and I doubt changing aria-hidden would affect this issue.

There was a change to how List reads and modifies scroll values in December 2022. Since List finds its scrollable parent and modifies the scroll position of this parent this change should affect this scenario as well.

I made the change in question to make List's virtualization calculations more consistent, so if the root of this issue is from fractional scroll values it makes sense the List change would fix this as well.

I tried setting the version to 7.156.0 (the version called out originally) in Codepen but was unable to reproduce the bug so I cannot say if it's truly fixed or if it just happens to "work on my machine".

@JustSlone as you've been able to reproduce the issue in the past do you still see the issue on this Codepen that is using v7.156.0?

JeffJankowski commented 1 year ago

@JustSlone Looking at my Sentry logs, the customer is triggering the bug with Chrome 111.0.x, but I am also on that version and cannot repro.

@spmonahan I also cannot repro with your codepen using v7.156.0. I tried various zoom levels, which surfaced the bug previously.

I figured this was indirectly fixed by a Chromium update that affected height calculations since I hadn't seen it in a while, but considering the client is on the same version of Chrome as me, I'm at a loss. Perhaps they are using a display/browser window with resolution that manifests the issue?

I might try and find a year-old version of Chrome and testing with that. I know progress on this issue is completed halted if we can't repro on-demand. Very frustrating.

spmonahan commented 1 year ago

@JeffJankowski I think I've found more reliable repro steps:

  1. Zoom page to 67%
  2. Display scale set to 125% on Windows. On Mac I picked a text size that effectively set the scale at ~133%.
  3. Scroll using the track pad as this fires many more scroll events than using the wheel on a mouse
    1. Of course this depends on the mouse and the trackpad. I've been able to repro using the trackpad on a Surface Book 3 and a MacBook Pro.

This makes me think that the fix is to debounce the scroll event called out earlier in the thread.

JeffJankowski commented 1 year ago

@spmonahan Fantastic! I was able to repro, not exactly with the same numbers, but it got me on the right path. I used 125% Windows scaling and 50% Chrome zoom level.

Something interesting I noticed was that the crash really depends on the current height of the render area of the codepen. I could increase/decrease the area by a couple pixels by dragging the source/console pane and that would change the results.

This is consistent with my theory that certain resolutions, causing fraction pixel values, is the root cause of the crash.

spmonahan commented 1 year ago

@JeffJankowski Glad we've found reliable repro steps.

Are you also able to repro using the latest Fluent v7?

JeffJankowski commented 1 year ago

@spmonahan

I can consistently reproduce with the latest fluent version using 125% Windows, 50% Chrome by scrolling to the bottom of the list, and just expanding the content pane.

chrome_tGgU1ox9qi

Edit: Can also repro with 100% Windows, 150% Chrome using the same method.

spmonahan commented 1 year ago

Thanks for the info @JeffJankowski !

spmonahan commented 1 year ago

Updated repro case for Fluent v8: https://codepen.io/seanms/pen/QWzEZGR