Add code & demo to pause/unpause parents of nested traps #30

chandlerprall · 2018-08-14T17:33:16Z

Closes #27

This change adds a _FocusTrapReact object to the React context when a focus trap is used. Subsequent child focus traps look for this object and, if it exists, uses it to pause the parent trap on child mount and unpause the parent when the child unmounts.

I'm not completely sure about how the props should interact with the pausing/unpausing (lines 32-37), I decided to always call pause and only call unpause if pause=false

I tested this change in the setup described in #27 and it works as expected.

davidtheclark · 2018-08-15T04:43:16Z

Thanks @chandlerprall! I'm pretty busy right now so might not get to this for a few days, but I'll keep in my todo list and try to have a close look soon.

davidtheclark

Great work figuring this out, @chandlerprall. I have a few questions for you.

davidtheclark · 2018-09-02T18:33:01Z

package.json

    "react": "^16.0.0",
    "react-dom": "^16.0.0"
  },
  "dependencies": {
    "focus-trap": "^3.0.0"
  },
  "peerDependencies": {
+    "prop-types": "^15.0.0",


It this absolutely necessary, or could we avoid the breaking change by providing a dummy function for the context types?

Maybe we should end up adding a PropTypes peer dependency, but I'd love to treat that as a separate issue if possible.

davidtheclark · 2018-09-02T18:34:07Z

src/focus-trap-react.js

+      _FocusTrapReact: {
+        pause: () => this.focusTrap.pause(),
+        unpause: () => {
+          if (this.props.paused === false) {


I think we could leave out this condition, since focus-trap already handles it: https://github.com/davidtheclark/focus-trap/blob/master/index.js#L93

davidtheclark · 2018-09-02T18:34:50Z

src/focus-trap-react.js

@@ -45,6 +66,10 @@ class FocusTrap extends React.Component {
    if (this.props.paused) {
      this.focusTrap.pause();
    }
+
+    // if there is a _FocusTrapReact context from a parent focus trap, pause it
+    const {_FocusTrapReact: {pause} = {}} = this.context;


Let's name this context prop something like FocusTrapParent, to clearly indicate that we're talking about a parent.

davidtheclark · 2018-09-02T18:36:26Z

src/focus-trap-react.js

@@ -45,6 +66,10 @@ class FocusTrap extends React.Component {
    if (this.props.paused) {
      this.focusTrap.pause();
    }
+
+    // if there is a _FocusTrapReact context from a parent focus trap, pause it
+    const {_FocusTrapReact: {pause} = {}} = this.context;


I'd prefer some more straightforward non-destructured code here — something like if (this.context.FocusTrapParent && this.context.FocusTrapParent.pause) { .. } — especially because this instance has its own pause function.

davidtheclark · 2018-09-02T18:40:01Z

src/focus-trap-react.js

+
+    // if there is a _FocusTrapReact context from a parent focus trap, pause it
+    const {_FocusTrapReact: {pause} = {}} = this.context;
+    if (pause) pause();


If I recall right, focus-trap itself should already pause any active focus traps when a new once is activated: https://github.com/davidtheclark/focus-trap/blob/6ecf7575401b2050cf2f3935544b82d75155c560/index.js#L101-L105. Does that work in this context? If so, maybe we could remove this part of the change?

Then the only trick would be unpausing the previously active trap — similar to this focus-trap demo: https://github.com/davidtheclark/focus-trap/blob/6ecf7575401b2050cf2f3935544b82d75155c560/demo/js/nested.js

That actually makes me wonder if this is how all focus-traps should behave, whether in React or not. If the activation of trap B ends up pausing trap A, should the deactivation of trap B always end up unpausing trap B?

chandlerprall · 2018-09-05T17:56:37Z

That actually makes me wonder if this is how all focus-traps should behave, whether in React or not. If the activation of trap B ends up pausing trap A, should the deactivation of trap B always end up unpausing trap B?

I think that does make a lot of sense. It would need to keep a queue of pauses, for example:

trap 1 activates
trap 2 activates ([1] is paused)
trap 3 activates ([1, 2] are paused)

couple things could happen

trap 3 deactivates, unpauses most recent in queue (2)
trap 2 deactivates - should this remove from the queue altogether, or should the trap remain in the queue until it is popped off and then check to see if it is active (otherwise a trap that is de- and re-activating jumps around in the queue)?

trap 1 is destroyed & removed from the queue altogether

which I think opens an additional question: if a trap is paused by another trap, does de-activating and re-activating the first trap make it the active trap?

davidtheclark · 2018-09-07T15:17:59Z

These are all good points, @chandlerprall, and make me second guess whether we should tray to automate this at all — given the complexity, and the danger of edge cases. Within React I guess we have the advantage that when you nest trap 2 in trap 1, you cannot unmount/deactivate trap 1 without also deactivating trap 2. I wonder, then, if we should just move forward with auto-unpausing in the React context and see how it goes?

chandlerprall · 2018-09-10T15:49:59Z

Ha! Based on

If I recall right, focus-trap itself should already pause any active focus traps when a new once is activated: https://github.com/davidtheclark/focus-trap/blob/6ecf7575401b2050cf2f3935544b82d75155c560/index.js#L101-L105

My original use case should have worked. Turns out active was never set to true on the second (child) instance, so the parent listener wasn't paused to allow focus (and therefore clicks) to shift to the child. Fixing that oversight on my side resolves the original issue, but deactivating the child does not re-activate the original, so I think a basic queue in focus-trap still makes sense.

maintain a queue of traps
when a trap is activated
- pause the currently active trap (which is at the end of the queue)
- if the activating trap is in the queue, remove it
- add the activating trap to the end of the queue
when a trap is paused / destroyed and is at the end of the queue (currently active)
- remove from queue
- start at end of queue and walk back until a trap is found that should be active, activate that trap (moving it to the end of the queue if needed)

IMO, a consumer managing the active state of multiple, competing traps has a bad UX flow and should more or less result in undefined behaviour. The only valid use case I can think of is a modal taking focus from something else, but even then the original trap shouldn't pause & re-activate, that would be a terrible flow for a user. So I feel the above queue management approach is fine for all situations.

davidtheclark · 2018-09-17T13:52:37Z

@chandlerprall: (I've been on vacation the past week ...) The last-in-first-out queue idea sounds like a good one. I agree with you that nesting of focus traps is not desirable except in some pretty rare situations. So I wonder if we could add it to focus-trap as a new opt-in option, queueNested of something, with a default value false? Open to a PR!

chandlerprall · 2018-09-18T15:53:08Z

I hope you had a good vacation!

I feel this shouldn't be behind a flag; I believe the current implementation around listeningFocusTrap in focus-trap is buggy as it doesn't unpause any previous traps.

davidtheclark · 2018-09-23T22:36:56Z

Ok, @chandlerprall, I think I'd be ok with another major release of focus-trap that introduces this change. However, I do want a flag to turn it off (it could be on by default), in case someone's special situation means they don't want that behavior.

chandlerprall · 2018-09-27T14:51:15Z

Sounds good, I'll take a shot at that.

davidtheclark · 2019-01-26T15:13:14Z

With 4b6e412, released in 6.0.0, the trap-queueing built into focus-trap is now available in this library.

Add code & demo to pause/unpause parents of nested traps

d927233

davidtheclark reviewed Sep 2, 2018

View reviewed changes

chandlerprall mentioned this pull request Nov 13, 2018

Maintain a history of traps to restore focus to a previously active trap focus-trap/focus-trap#68

Merged

chandlerprall mentioned this pull request Nov 30, 2018

Trap doesn't work if the children are rendered into a React portal #27

Closed

davidtheclark closed this Jan 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code & demo to pause/unpause parents of nested traps #30

Add code & demo to pause/unpause parents of nested traps #30

chandlerprall commented Aug 14, 2018

davidtheclark commented Aug 15, 2018

davidtheclark left a comment

davidtheclark Sep 2, 2018

davidtheclark Sep 2, 2018

davidtheclark Sep 2, 2018

davidtheclark Sep 2, 2018

davidtheclark Sep 2, 2018

davidtheclark Sep 2, 2018

chandlerprall commented Sep 5, 2018

davidtheclark commented Sep 7, 2018

chandlerprall commented Sep 10, 2018

davidtheclark commented Sep 17, 2018

chandlerprall commented Sep 18, 2018

davidtheclark commented Sep 23, 2018

chandlerprall commented Sep 27, 2018

davidtheclark commented Jan 26, 2019

Add code & demo to pause/unpause parents of nested traps #30

Add code & demo to pause/unpause parents of nested traps #30

Conversation

chandlerprall commented Aug 14, 2018

davidtheclark commented Aug 15, 2018

davidtheclark left a comment

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

davidtheclark Sep 2, 2018

Choose a reason for hiding this comment

chandlerprall commented Sep 5, 2018

davidtheclark commented Sep 7, 2018

chandlerprall commented Sep 10, 2018

davidtheclark commented Sep 17, 2018

chandlerprall commented Sep 18, 2018

davidtheclark commented Sep 23, 2018

chandlerprall commented Sep 27, 2018

davidtheclark commented Jan 26, 2019