Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Latest focus trap changes causes the modal response to be lost #565

Closed
bitwolfe opened this issue Mar 23, 2022 · 1 comment
Closed

Latest focus trap changes causes the modal response to be lost #565

bitwolfe opened this issue Mar 23, 2022 · 1 comment

Comments

@bitwolfe
Copy link

The recent change to how the focus trap is set up (PR #549) is causing issues when you expect the close action to return a value.

Specifically this change in the modal component on lines 103-105:
https://github.com/simplabs/ember-promise-modals/pull/549/files#diff-f1bdcfc6adc126290ecb2862332b13bf329fcd5a397b3ab6210de2911dc65d54L93-R105

Not passing in null here as it did before changes the behaviour in such a way that the focus trap's onDeactivate function calls closeModal again (without return value this time) and resolves the modal before the original @close action call has a chance to resolve with the return value that was passed in.

So currently, this happens:

  1. A link in your modal calls @close
  2. Internally the modal then calls closeModal(result) with the result passed to @close
  3. This then runs focusTrap.deactivate() which immediately runs the onDeactivate function
  4. closeModal() is called again, but without the passed in result this time.
  5. The modal resolves with the undefined result before closeModal(result) from step 2 has a chance to resolve with its data.

Reverting lines 103-105 linked above makes it work properly again, though that probably breaks something else that the PR fixed?

Another workaround might be to set onDeactivate to null through the open() call or globally on the modals service, but that feels like it shouldn't be necessary.

kevinansfield added a commit to TryGhost/ember-promise-modals that referenced this issue Apr 1, 2022
… value

refs mainmatter#565

- full description of the problem can be found in the upstream issue mainmatter#565
- we don't use any custom `onDeactivate` hooks so disabling them is good-enough for now
kevinansfield added a commit to TryGhost/Admin that referenced this issue Apr 1, 2022
…n modals

no issue

- bumped version of our `ember-promise-modals` fork containing a workaround for modal promises missing their return values
  - upstream issue: mainmatter/ember-promise-modals#565
- switched from a GitHub ref to a proper published package to avoid issues with yarn not bumping versions of the fork for developers who installed an earlier version
@pichfl
Copy link
Contributor

pichfl commented Apr 12, 2022

Thanks for reporting this. For some reason we were missing a test that verifies this.

Resolved by #596

@pichfl pichfl closed this as completed Apr 12, 2022
pichfl added a commit that referenced this issue Apr 12, 2022
Turns out `focus-trap` was not the culprit here, but the fact that we resolved the modal _after_ calling the `deactivate` method on the focus trap.

The changes to the events in the dependency were purely coincidental, but using the newly introduced events happended to fix the issue as well as the `onPostDeactivate` happens to run a tick later than the `onDeactivate` which is enough for the `resolve` to happen.

Re #565
tigefa4u pushed a commit to tigefa4u/Ghost that referenced this issue Aug 3, 2022
…n modals

no issue

- bumped version of our `ember-promise-modals` fork containing a workaround for modal promises missing their return values
  - upstream issue: mainmatter/ember-promise-modals#565
- switched from a GitHub ref to a proper published package to avoid issues with yarn not bumping versions of the fork for developers who installed an earlier version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants