mainmatter / ember-promise-modals Goto Github PK

This issue lists Renovate updates and detected dependencies. Read the Dependency Dashboard docs to learn more.

Rate-Limited

These updates are currently rate-limited. Click on a checkbox below to force their creation now.

• Update dependency eslint to v8.56.0
• Update dependency eslint-plugin-ember to v11.12.0
• Update dependency pnpm to v8.14.0
• Update dependency ember-source to v5
• Update dependency ember-try to v3
• Update dependency eslint-config-prettier to v9
• Update dependency eslint-plugin-prettier to v5
• Update dependency eslint-plugin-qunit to v8
• Update dependency postcss-preset-env to v9
• Update dependency prettier to v3
• Update dependency qunit-dom to v3
• Update dependency release-it to v17
• Update dependency sinon to v17
• 🔐 Create all rate-limited PRs at once 🔐

Open

These updates have all been created already. Click a checkbox below to force a retry/rebase of any.

• Update dependency ember-template-lint to v5.13.0
• Update actions/checkout action to v4
• Update actions/setup-node action to v4
• Update dependency @ember/test-helpers to v3
• Update dependency @release-it-plugins/lerna-changelog to v6
• Update dependency ember-cli to v5
• Update dependency ember-cli-babel to v8
• Update dependency ember-resolver to v11
• Update dependency focus-trap to v7
• Lock file maintenance
• Click on this checkbox to rebase all open PRs at once

Detected dependencies

• Check this box to trigger a request for Renovate to run again on this repository

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.

This way we could allow users to defer (parts of) rendering the content to keep the animations nice and smooth.

Hi!

@herzzanu and me have investigated a flaky test in a huge app.

After some frustrating investigation we have discovered, that the cause of test failures was that a modal's @close callback was called twice.

The frustration was caused by trhee parts:

• everything is async, so the stack trace points at a promise that had been caught by Ember Backburner or something, and the stack trace does not point at the code of the promise callback;
• we have a chain of ember-concurrency tasks threading through multiple components; a modal is opening a modal, etc;
• tests never failed locally when the browser tab was focused; tests would sometimes fail when the tab was not focused. 🤯

The specific error turned out to be "calling set on destroyed object", with stack trace pointing at this line:
https://github.com/mainmatter/ember-promise-modals/blob/v4.0.0/addon/components/modal.js#L148

The solution on our side was to make sure the modal was not being closed twice.

But I wonder, if this should even be a developer's concern? Why not let developers close the modal more than once?

At the very least, we could wrap the set into if (!this.isDestroying && !this.isDestroyed). Or we could make the close callback return early if it is called more than once.

Given we did so for most of our other projects, we might do it here as well.

When closing via click on the container, the modal gets "hidden" but stays in the DOM. This essentially disables all interaction with the application which then has to be reloaded to work again.

It does not always happen, but most of the time.

Screencast Chrome 102 Mac with the demo app:
https://user-images.githubusercontent.com/5049311/170370725-1b7b6ef4-29d7-467d-8e99-132d645e7fcb.mp4

I also did some debugging as in one project I often see timeouts in qunit tests with modals. It turned out that when a timeout happens, the event animationend is not being triggered which leads to not calling modal._remove() which leads to not resolving the promise. I am unsure if this is related or another problem.

• Update ember-auto-import to v2 (breaking change)
• Update to v2 format (requires ember-auto-import v2)

Currently we immediately apply the focus-trap even if an animation is still about to happen. This may cause problems if something like an ember-tether is rendered inside the modal as the modal might still be moving.

This could be solved by utilizing the checkCanFocusTrap hook: https://github.com/focus-trap/focus-trap#createoptions . We can pass it a promise that resolves once the animation is complete.

We might want similar behaviour when closing the modal

Error: testWaiter.endAsync called with no preceding testWaiter.beginAsync call.
Test waiter calls should always be paired. This can occur when a test waiter's paired calls are invoked in a non-deterministic order.

Hapens in a larger codebase with setupPromiseModals(hooks); enabled. Needs further investigation.

We're currently running a fork of ember-promise-modals but it looks like most of our changes are now possible using the official addon. Our one remaining custom change is the addition of a beforeClose modal option.

Our typical use-case for beforeClose is when we have modal UI that allows user input, when that modal is closed if there are unsaved changes we want to abort the close and open another modal to get confirmation.

Simplified example:

setupController(controller, model) {
    this.newsletterModal = this.modals.open(
        EditNewsletterModal,
        {newsletter: model},
        {beforeClose: this.confirmUnsavedChanges}
    );
}

async confirmUnsavedChanges() {
    if (!this.model.hasDirtyAttributes) { return true; }

    // result: true = close and discard, false = abort close, allow further edits and save
    this.confirmModal = this.modals.open(ConfirmUnsavedChangesModal);

    return this.confirmModal;
}

Does something like this sound like an appropriate addition? Is there something like this already available that I've missed?

Our implementation in the fork was very basic and can be seen in this commit.

Some addons like Ember Power Select render outside the component they're invoked in, making it impossible to use with EPM. (Although EPS can be forced to render in place, but could be difficult to deal with for various reasons). I saw no option in Focus Trap to exclude certain selectors.

I know it would reduce accesibility, but spending time with code that handle modals if we don't have EPM also takes away precious time we could otherwise spend with making our app more accessible.

It doesn't even have to be public API, just an option that's one would only use if really needed it.

Should I open a PR?

In theory it shuts down the 3rd of August 2021

https://github.blog/2021-04-29-goodbye-dependabot-preview-hello-dependabot/

Calling close() on the modal from the outside doesn't seem to close the modal if prefer-reduced-motion is enabled on the system and if that applies a 0s duration through CSS media queries. The modal elements are left in the DOM. If I set a non-0s duration, for example 0.01s, then it works.

If however close() is called from inside the modal, then it works fine and the modal elements are removed from the DOM no matter what duration is used. With the 3.0.1 release it broke if you clicked the backdrop to close, but this seems to be fixed in the main branch.

So TLDR: A workaround seems to be to not set a 0s duration, such as 0.01s. Then everything works fine even with 3.0.1.

Originally posted by @bitwolfe in #665 (comment)

Unfortunately it seems this breaks installing this package from Git using Yarn Berry (at least v3.1.0 that I'm using) with nodeLinker: node-modules. Yarn says "Unsupported workflow", as it's seems like it doesn't understand pnpm lockfiles yet..

Originally posted by @bitwolfe in #477 (comment)

We're already ember 4 compatible and implicitly testing against it through ember-release, ember-beta etc. but would be good to add 4.0.0 to the CI config explicitly.

We don't seem to have documentation explaining what the Modal instance can do (e.g. let modal = this.modals.open(...); modal.close()).

Converting ember-promise-modals/modal to a classic Ember Object would expand support to even earlier Ember versions. Huge codebases that struggle with updating could benefit greatly from EPM, and from what I observed, that part of the addon is not performance-critical.

We're patching EPM in our project at the moment. Do you accept PRs? :)

Environment

This happens on a PC in both Firefox and Chrome.

What I expect to happen

ESC and the close button should both close the modal and return focus to the element that opened the modal.

What happens

• When I press ESC and close the modal, the focus (correctly) returns to the button that opened the modal.
• When I interact with the "close" button to close the modal, focus is not returned to the button that opened the modal.