react icon indicating copy to clipboard operation
react copied to clipboard

Bug: Disabled button state update prevents `scrollIntoView`

Open jjenzz opened this issue 4 years ago • 13 comments

I have a button that triggers scrollIntoView for a node when clicked. It also updates some state which subsequently disables that button in certain cases. If the button becomes disabled, the scrollIntoView execution aborts.

This seems to only happen in Chrome/Brave.

React version: 17.0.1

Steps To Reproduce

See steps to reproduce in sandbox.

https://codesandbox.io/s/goofy-curie-yts68?file=/src/App.js

The current behavior

Described above.

The expected behavior

The state update should not prevent scrollIntoView.

jjenzz avatar Feb 09 '21 18:02 jjenzz

hi @jjenzz can you try following solution. may it will help.

setTimeout(() => {
            setActiveItem(1);
}, 1000);

arvind-nikam-halodoc avatar Feb 17 '21 08:02 arvind-nikam-halodoc

Reproduces with:

  • 16.14.0
  • experimental and createRoot and createBlockingRoot

Does not reproduce when:

  • using onMouseDown instead of onClick

Experimented a bit with the suggestion of @arvind-nikam-halodoc and it stops scrolling once the state update is triggered. For example, with

setTimeout(() => {
  setActiveItem(1);
}, 300);

it starts scrolling and stops right in the middle once the timer runs.

More confusion: Calling scrollIntoView from a button that isn't being disabled works: https://codesandbox.io/s/onclick-scrollintoview-cancellation-workaround-z7si1

eps1lon avatar Feb 17 '21 09:02 eps1lon

This is not a scrollIntoView issue, but rather a scrollIntoView({ behavior: "smooth" }) issue. Changing "smooth" to "instant" fixes behavior. So I assume it has to do something with the browser having to maintaining focus when it is a smooth scroll and react's re-render causes us to lose that focus for a temporary second. Unfortunately I could not find any implementation details of scrollIntoView so this is just a guess.

Longwashere avatar Feb 19 '21 19:02 Longwashere

A hack you can do is to trigger the setState and then call the scroll in a setTimeout to throw the scroll into the eventloop letting the render happen first such as:

          setActiveItem(1);
          setTimeout(() => {
             //using a ref here
            slideNode.current.scrollIntoView({block:"end", behavior: "smooth" });
          }, 0);

Longwashere avatar Feb 20 '21 05:02 Longwashere

So I assume it has to do something with the browser having to maintaining focus when it is a smooth scroll

Interestingly, this doesn't happen in other browsers as far as I could tell. Perhaps I should raise a Chrome issue?

A hack you can do

Indeed. This was the first thing I had tried but not particularly keen on littering setTimeout if it can be avoided. I may just go with it for now though as it does feel strange to use DOM API for this case.

Thanks for the help ☺️

jjenzz avatar Feb 20 '21 09:02 jjenzz

@jjenzz Please try this:

const slideNode = document.getElementById(`item-${1}`);
setActiveItem(1);
requestAnimationFrame(() =>  slideNode.scrollIntoView({ behavior: "smooth" }));

https://codesandbox.io/s/nice-agnesi-q4g0v

And also I think this issue is about re-rendering

behnammodi avatar Feb 27 '21 07:02 behnammodi

@behnammodi Yes, I am aware that delaying things will make it work, these are all things I tried when debugging it but I shouldn't have to do that. It works perfectly fine in other browsers.

Thanks for the help though 🙂

jjenzz avatar Mar 19 '21 19:03 jjenzz

Hi there, React will refocus an element after it applied effect mutation. However, since your button is disabled, React can not perform refocus. Actually, it has tried to refocus but unsuccessfully. The process of refocusing blocks the scroll run smoothly. And sure, this happened with all elements having disabled attribute. React also acknowledges (comments on the code of restoreSelection) that by refocusing, it's undesirable that focusing a node can change the scroll position.

If you might wonder why it's related to focus, clicking a button triggers focus of that element. React performed mutation that has deletion and had to refocus the priorFocusedElem later. But this time, it failed.

In fact, it's not necessary to refocus an element that have disabled attribute so we should check if it's a disabled element or not. I've created a pull request at #21480. Please have a look.

mrsilver256 avatar May 10 '21 08:05 mrsilver256

I had the same issue and found a workaround for this. Basically I just added the scroll into view call inside a setTimeout, effectively waiting for all the react state updates before scrolling.

setTimeout(() => {
  ref.current?.scrollIntoView({ behavior: "smooth" });
}, 0);

acapro avatar Nov 12 '21 11:11 acapro

I had a similar issue without even any disabled button state and "fixed" it with requestAnimationFrame.

  • react 17.0.2

Before:

useEffect(() => {
  if (scrollToTodayRequired && todayRef.current) {
    todayRef.current.scrollIntoView({
      behavior: "smooth",
    });

    setScrollToTodayRequired(false); // <= this line will cancel the scrollIntoView
  }
}, [scrollToTodayRequired, todayRef.current]);

If you remove behavior (instant) then it's working.

After:

useEffect(() => {
  if (scrollToTodayRequired && todayRef.current) {
    requestAnimationFrame(() => {
      if (todayRef.current) {
        todayRef.current.scrollIntoView({
          behavior: "smooth",
        });
      }
    });

    setScrollToTodayRequired(false);
  }
}, [scrollToTodayRequired, todayRef.current]);

Now, this is working as expected. Somehow the smooth scrollIntoView was being canceled by the setState which I have a hard time understanding.

vvo avatar Nov 25 '21 11:11 vvo

Hi,

After going through your sandbox and trying what everyone else has suggested, It seems that this bug seems to be more related to the browser's scrollIntoView implementation.

However, this seems to work without making too many changes to your code:

instead of using scrollIntoView, I called the container node's scrollTo function and passed it the offsetLeft of the element you wanted the button to scroll to.

Sandbox with the alternate solution

hope this helps !

P.S. Only tried this is Chrome

amangupta1990 avatar May 10 '22 10:05 amangupta1990

We also ran into the original issue. Wrapping the smooth scrollTo in setTimeout works. https://github.com/facebook/react/pull/22256 would probably be a fine fix for this issue. On macOS it only manifests in Chrome, not in Safari.

xixixao avatar Aug 09 '23 01:08 xixixao

A simple fix (and one that doesn't involve setTimeout) would be to call .blur() on the button before .scrollIntoView() is triggered. E.g.

onClick={(event) => {
  event.target.blur();
  const slideNode = document.getElementById(`item-${1}`);
  slideNode.scrollIntoView({ behavior: "smooth" });
  setActiveItem(1);
}}

This way the button loses focus right before the scroll occurs, which means Chrome won't detect a change mid-scroll.

Should note that it's probably best for UX/accessibility to only blur when necessary. So probably a good idea to wrap the .blur() inside an if that checks to see if the button is going to be disabled (e.g. if (index === length - 2) or something like that). For example: https://stackoverflow.com/a/78409295/4157821

tuckergordon avatar Apr 30 '24 14:04 tuckergordon

In my case only setting buttonElement.disabled = true right after #.scrollTo({ behavior: "smooth", top: 0 }) fixes it

dimaMachina avatar Jul 19 '24 23:07 dimaMachina