Disabling a button while clicking it leads to invalid Focus-Outline shown later on

[tomsontom]

🐛 Bug Report

If you disable a button when clicking the very same button leads to an invalid focus-outline if you afterwards move focus using the the keyboard leading to an UI like this:

🤔 Expected Behavior

No focus outline on the disabled button

😯 Current Behavior

2 Focus-Outlines appear on 2 different controls

💁 Possible Solution

🔦 Context

I tried to implement a wizard dialog and ran into this

💻 Code Sample

Add this snippet to the ButtonGroup.stories.tsx

...
  .add('focusbug', () => <FocusBorderBug />);

function FocusBorderBug() {
  const [total] = useState(2);
  const [pageIndex,setPageIndex] = useState(0);
  return <ButtonGroup>
      <Button variant="secondary" isDisabled={pageIndex-1 < 0} onPress={() => setPageIndex(pageIndex-1)}>Previous</Button>
      <Button variant="secondary" isDisabled={pageIndex+1 >= total} onPress={() => setPageIndex(pageIndex+1)}>Next</Button>
      <Button variant="cta">Finish</Button>
      <Button variant="secondary">Cancel</Button>
  </ButtonGroup>
}

To reproduce:

• Click on "Next" until it gets disabled
• Hit Tab
• You should see the above state

🌍 Your Environment

Software	Version(s)
react-spectrum	current master
Browser	latest FF & Chrome
Operating System	OS-X

🧢 Your Company/Team

🕷 Tracking Issue (optional)

[snowystinger]

I think this is related, #1157
the key take aways are

1. A change to useFocus

There might be an improvement we could make around this line https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/interactions/src/useFocus.ts#L36 where we still listen for onBlur.

2. Move focus off the disabled control when it becomes disabled
   combining these two, the focusring class would get cleaned up automatically

[snowystinger]

After investigating, it would appear that we are running into this Issue facebook/react#9142
We cannot appropriately solve the issue of blur not getting called in React Spectrum, it has to be solved in React
TL;DR of the issue is that React ignores events during an update in order to not be overwhelmed by events it causes from manipulating the DOM

As a workaround, you will need to first move focus to the new element. THEN disable the button via state. Focus and blur will then happen before the update instead of during it.

[tomsontom]

well in my case that might be a possible work-around but in other scenarios this will not work because one probably does not know where the focus should go next and it might turn out hard to not violate accessibility rules because you need to shift the focus to the appropriate previous focus element (not sure about the rules there).

In general this means that one has to guard all elements in a codebase who could potentially get disabled while holding the input focus.

[snowystinger]

I mean, that's no different than you would have to do no matter what. In the case of something such as a pagination component, buttons - [prev, 1, 2, 3, ..., next], if you're on prev and you get down to page one, prev is typically disabled. The component knows this and will take care of moving focus to an appropriate element within itself if it can.
A standalone button in your application though cannot possibly know where focus should go, but the application or component containing that button should know that if that button becomes disabled with focus that the focus should be moved and where it should be moved to. We've provided some helpers if you use a FocusScope that can allow you to move focus to the previous focusable or next focusable element https://react-spectrum.adobe.com/react-aria/FocusScope.html#focusmanager-interface

[tomsontom]

Thanks did not know about this focus-manager thing!