react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Provide a way to add portaled (outer) nodes inside FocusScope

Open yakunins opened this issue 2 years ago • 12 comments

🙋 Feature Request

Provide a way to pass into FocusScope nodes, which should consider as a part of it — even they are mounted outside of FocusScope node tree (e.g. portaled).

🤔 Expected Behavior

Let say we have portaled tooltips with some links, and these tooltips are inside FormDialog. And we need to have them in its FocusScope in order to be available for tabbing. Something like that: <FocusScope shards={tooltipRefs}><FormDialog /></FocusScope>

😯 Current Behavior

Any ideas?

💁 Possible Solution

See https://github.com/theKashey/react-focus-lock/blob/master/src/Lock.js#L199

🔦 Context

FocusScope inner component has portaled nodes with buttons, links, inputs.

💻 Examples

<FocusScope shards={tooltipRefs}>
  <Input1 /> // inside focus scope
  <Input2 /> // inside focus scope
  <Portal>
    <Input3 />  // outside, but we must have it inside focus scope
  </Portal>
</FocusScope>

yakunins avatar Sep 02 '22 14:09 yakunins

Thanks for the issue! I don't quite follow. What are shards? And can you build a codesandbox demonstrating what you think should be working with an explanation of why it isn't working?

snowystinger avatar Sep 02 '22 16:09 snowystinger

@snowystinger Sure! Take a look on this sandbox and try to use the link inside Tooltip with keyboard.

What are shards?

Basically, shards (a prop) are some nodes, to be considered as FocusScope internal nodes, even they are outside of FocusScope's tree (e.g. portaled).

Editor: https://codesandbox.io/s/zen-colden-pmtnfr?file=/src/App.js

<Modal>
  <FocusScope contain>
    <div>
      <input placeholder="Input1" />
      <br />
      <input placeholder="Input2" />
      <br />
      <Tooltip // ← Here's the problem, because Tooltip's content is inside another Portal
        content={<>Please follow <a href="#tooltip">this link</a>.</>}
      >
        Help
      </Tooltip>
    </div>
  </FocusScope>
</Modal>

image

yakunins avatar Sep 05 '22 09:09 yakunins

For starters, you can't have interactive elements in a tooltip for accessibility reasons. Can you try the example again with a popover? I'm sorry I can't write much more, I'm on my phone. @LFDanLu ^

snowystinger avatar Sep 05 '22 16:09 snowystinger

@snowystinger

can't have link in a tooltip

It is just a simple example. The real problem is more like this:

  1. Modal (portaled) have inside...
  2. Select, or DatePicker, or TimePicker, and...
  3. they all have their dropdowns portaled (to prevent clipping by Modal's boundaries), and...
  4. and focus is locked whether on Modal, otherwise on dropdown!

May we have them both together inside one FocusScope?

Our real example:

image

Just for fun, Gmail example:

image

yakunins avatar Sep 06 '22 14:09 yakunins

@yakunins This use case should already be supported by our React Spectrum components, albeit the child Popovers also have their own FocusScope: https://codesandbox.io/s/react-spectrum-template-forked-vnmzwm?file=/src/App.js. FocusScope should allow focus to move inside a child scope from a parent scope and should work even when that child scope is portaled.

Here is a slightly modified version of your sandbox, note you can tab to the button that triggers the tooltip, open it with Enter, that focus lands on the tooltip when it opens, and that you can then tab out of the tooltip and land back on the trigger. Note that ideally the tooltip would automatically close when focus leaves it and that tabbing forward from a parent scope into a portaled child FocusScope is not supported.

LFDanLu avatar Sep 06 '22 17:09 LFDanLu

@LFDanLu In the case with the DatePicker mentioned by @yakunins, the input is still part of the contain.

When the DatePicker is open, the input and the calendar is within the focus contain for users to use either method of input. Would this be something FocusScope can do?

One option I can think of is to put keypress to detect the tab from the input to pass the focus to the calendar and visa-versa.

tounsoo avatar Sep 06 '22 20:09 tounsoo

@tounsoo A parent FocusScope currently considers a child scope as part of its contain which is why you can click into the input inside the tooltip here and not have the parent scope (aka the Modal) try to restore focus back to one of its immediate children. However, as you mentioned the only problem currently is that we don't handle tabbing from a parent scope into a portaled child due to our usage of a TreeWalker to determine the next/previous focusable element when hitting Tab. Now that we've refactored FocusScope so it tracks scopes via a tree, we could potentially walk down the tree into a child scope when we detect that we are tabbing from that child's root scope element? I can bring this up with the team to see what they think.

LFDanLu avatar Sep 06 '22 21:09 LFDanLu

@LFDanLu Thank you!

tounsoo avatar Sep 06 '22 21:09 tounsoo

I'm not sure this is really possible for us to do without deep access to the React tree. While we do have a tree of focus scopes, it's not ordered. So if you had something like this:

<FocusScope>
  <input />
  <Portal>
    <FocusScope>
      <input />
    </FocusScope>
  </Portal>
  <input />
</FocusScope>

we wouldn't know that the portal was between the two inputs vs at the beginning or end. That's why I originally proposed FocusScope as an API to be built into React. See: https://github.com/reactjs/rfcs/pull/109 Unfortunately, it hasn't really gone anywhere. In the mean time, we are kinda limited to what the DOM tree provides us.

devongovett avatar Sep 07 '22 17:09 devongovett

I was expecting something similar to shard in focus lock.

tounsoo avatar Sep 07 '22 18:09 tounsoo

That still doesn't specify what the tab order should be. I guess they assume the shards are always at the end? Not sure this is a safe assumption.

devongovett avatar Sep 07 '22 18:09 devongovett

In the focus lock, the shard are array and it passes the tab to the next shard. As a user of the focus lock atm, I didn't think that was confusing to use.

tounsoo avatar Sep 07 '22 19:09 tounsoo