Add 'enabled' prop to FocusScope component

[smashercosmo]

🙋 Feature Request

Add 'enabled' prop to FocusScope component for convenience. So that if you want, for example, to use the same navigation component both in top header and in sidebar (burger menu pattern) you could do this:

<div>
  <FocusScope contain restoreFocus autoFocus enabled={isSideBarExpanded}>
    <HeaderNavigation>
  </FocusScope>
</div>

instead of this:

<div>
  {
    isSideBarExpanded
     ? <FocusScope contain restoreFocus autoFocus><HeaderNavigation></FocusScope>
     : <HeaderNavigation>
   }
</div>

P.S. Sorry, I removed the rest of the issue template, because feature description is pretty sufficient in my opinion.

[smashercosmo]

Oh, I've just realised that you can use 'contain' prop for this purpose

[smashercosmo]

no, it doesn't work unfortunately :(

[smashercosmo]

hm... it actually doesn't work even with conditional rendering, but that's another issue

[snowystinger]

Hey! Thanks for the feedback. I think the way of thinking about it is that the burger menu is a modal, and so that (the modal) should have the FocusScope, the HeaderNavigation shouldn't. I don't think you'd run into the situation you described above because it'd really look like this (I'm using Spectrum components, but you could build your own following the docs) https://react-spectrum.adobe.com/react-aria/useDialog.html#example

{
  isSmallScreen
    ? <DialogTrigger><ActionButton aria-label="Site Navigation"><HamburgerIcon /></ActionButton><Dialog><HeaderNavigation /></Dialog></DialogTrigger>
    : <HeaderNavigation />

You might want to be careful about letting this close based on screensize though, you'd need to figure out where focus should go if it suddenly unmounts both the modal and the button that opened the modal.

Let me know if I've misunderstood. A CodeSandbox can usually help clear that up.

[tristandubbeld]

NOT PART OF THIS ISSUE but this might be the reason @smashercosmo created this issue.

Hi, I also ran into this "problem". I was under the impression that the FocusScope component could be used to contain focus to certain elements based on the contain property and that seems to be the case in this CodeSandbox example. All good.

However, in this CodeSandbox example you'll see that it's acting a little weird. It should not contain focus, but it does not focus past the second focusable element in the FocusScope. The difference is that the second example has an element with display: none following the FocusScope:

<FocusScope contain={false}>
  <button>Focused first</button>
  <button>Focused second, and doesn't leave me</button>
</FocusScope>

<div style={{ display: 'none' }}>
  <button>My parent has display none</button>
</div>

<button>I am not focusable</button>

I'm not sure about the science behind why focus "breaks" when the FocusScope is followed by a focusable element wrapped in a parent with display: none, but maybe this helps in your case @smashercosmo?

@snowystinger do you know if this is a bug, or intended behaviour? I would think that the button after the display: none one should be focused. I've been looking into the source for something obvious but haven't found anything yet. ~Should I move this to a new issue or will this suffice?~

Edit: Just checked and an issue (that looks similar) already exists: https://github.com/adobe/react-spectrum/issues/1478

[snowystinger]

@tristandubbeld just to confirm, your issue is captured by https://github.com/adobe/react-spectrum/issues/1478 ? Would you mind adding something to the top of your comment to clarify that it's not part of this issue? Just trying to keep issues from being conflated. Thanks!

[tristandubbeld]

@snowystinger I'm working that out right now. The issue seems to be talking mostly about elements inside the FocusScope, but my issue has to do with elements outside FocusScope. The PR they're working on (https://github.com/adobe/react-spectrum/pull/1493) might fix the issue, but I'm not completely sure. I could write a test for it in a separate PR (we'll have to wait until 1492 is merged), or suggest they add a test in theirs.

Edit: I've checked out the PR (cloned, installed) and wrote a test to check if the issue is fixed, but it's not (yet). I've written a reply in the PR on how I think it could be fixed. I don't think I need to create a new issue.

[tristandubbeld]

@snowystinger #1478 fixes my issue! It was the current version userEvent.tab that had a bug, throwing me off. Thanks for the replies.

[tounsoo]

A bit different but something @smashercosmo mentioned above about conditional contain prop. I'm trying to have 2 FocusScope as a sibling but controlling the containment using contain prop.

Basically, I'm trying to shift the contain from first FocusScope to the second on a button click by having contain be conditional using state and passing the focus to the input inside the second FocusScope. I'm not sure why but this does not seem to work.

Here's a very simple test on code sandbox: https://codesandbox.io/s/testing-aria-focus-k8queh

[snowystinger]

@tounsoo this might be addressed in https://github.com/adobe/react-spectrum/pull/3323

[tounsoo]

@snowystinger thank you! I'll check it out!

[snowystinger]

I think everything in here has been addressed, so I'm going to close this. Feel free to open a new issue if anything is still left.