Fix modals not being able to open after being closed once #1393

SanderKnauff · 2021-01-11T18:50:02Z

Currently, modals can only be opened once. After this, it gets stuck in a state on which the BsModal thinks it is open already, causing the show() method to do nothing at all.

While the issue I encountered did not exactly fit issue #1386 (I did not receive any errors on my side), it might resolve that issue as well. I however do not know how to test that issue.

jelhan · 2021-01-11T22:56:00Z

addon/components/bs-modal.js

@@ -473,7 +473,7 @@ export default class Modal extends Component {
    if (!this._isOpen) {
      return;
    }
-    this.isOpen = false;
+    this._isOpen = false;


If I didn't missed something, this was the only place, in which isOpen was actually set. So we wouldn't need @localCopy on it anymore. It would be just an alias to this.args.isOpen.

If the big could be resolved by not setting isOpen, would actually be inline with my findings in #1386 (comment).

I've refactored out the @localcopy. I've tried to set the value to this.args.open, however it seems like that breaks more than it fixes. Accepting open as an argument with a default value of true does not seem to break any tests. Any calls to this.isOpen without the underscore has been changed to open.

Glad to see that we could actually drop @localCopy decorator. Less complexity is always good. 😄

Your changes look very good. I'm just a little bit confused why you haven't stuck with isOpen name. The is prefix is a helpful indicator that a property is a boolean in my opinion. In this special case I also fear that a property open might be confused with the show method.

Would it be okay for you to change the name back to isOpen? Not the implementation. That one is totally fine. I'm only talking about the naming.

jelhan · 2021-01-11T23:06:22Z

Thanks a lot for working on this. I think you are on the right track here

While the issue I encountered did not exactly fit issue #1386, it might resolve that issue as well. I however do not know how to test that issue.

Such a test would look very similar to the test you have. But instead closing the model by setting @open to false, you would close it with a click on backdrop (or close button). Additionally you would provide an action handler to @onHidden, which sets @open to false. So invokation would look like this:

this.set('open', false);
this.set('close', () => this.open = false);

await render(hbs`
  <BsModal
    @open={{this.open}}
    @onHidden={{action this.close}}
  />
`);

SanderKnauff · 2021-01-12T00:29:42Z

Thanks a lot for working on this. I think you are on the right track here

While the issue I encountered did not exactly fit issue #1386, it might resolve that issue as well. I however do not know how to test that issue.

Such a test would look very similar to the test you have. But instead closing the model by setting @open to false, you would close it with a click on backdrop (or close button). Additionally you would provide an action handler to @onHidden, which sets @open to false. So invokation would look like this:
this.set('open', false);
this.set('close', () => this.open = false);

await render(hbs`
  <BsModal
    @open={{this.open}}
    @onHidden={{action this.close}}
  />
`);

SanderKnauff · 2021-01-12T00:33:28Z

(Apologies for closing, I misclicked)

@jelhan I've updated the test as well as per your suggestions. The test is a bit closer to the case that was presented but is still in the same spirit as it was.

As I've added more commits I'm wondering about the workflow. Should I squash these after everything has been cleared or will that be handled when merging?

jelhan

I've updated the test as well as per your suggestions. The test is a bit closer to the case that was presented but is still in the same spirit as it was.

The new test looks very good. But I meant it as an addition not as a replacement of your original test. Both tests - the original one in your first commit and the current one - are testing two different cases. And it seems as we have missed test coverage for both.

I'm sorry for that confusion. Would it be okay for you to add the test again?

As I've added more commits I'm wondering about the workflow. Should I squash these after everything has been cleared or will that be handled when merging?

You don't need to care about squashing. We can squash when merging.

jelhan · 2021-01-12T12:28:51Z

addon/components/bs-modal.js

+  /**
+   * @property open
+   * @private
+   */
+  @arg
+  open = true;


Any reason you renamed the variable from isOpen to open? The is prefix indicates that it's a boolean. I fear open can be confused with a method to open the modal. I would prefer to keep isOpen to be honest. Would it be okay for you to change back?

I haven't reviewed this PR yet, but I am a bit confused at this point: @open is the public argument, which we cannot rename. The JS docs above are wrong somehow, as there is @property open twice!

I've tried to rename isOpen to open to remove the dependency on the @localcopy and to clarify that it is an argument to the BsModal component. I must admit that I do not understand why there is both a isOpen and a _isOpen. The JSDoc seems indeed to be a bit odd, as isOpen had two JSDocs above it as if something between the docs was missing.

While I do agree that isOpen would be a better name for a boolean, it does not reflect the actual args of the component well, causing confusion. I've tried a few ways of getting it to be a proper alias of open, but none of them were succesful.

Oh. I missed that this has @arg decorator pointing to the argument. Sorry. Should have taken more time to review. Can not could how often I already got confused by @open argument and the two internal properties isOpen and _isOpen.

To be honest I thought this PR decouples @open from the internal state and manages the state updates through {{did-update}} modifier in the template only. I think I need to reserve some time to fully understand state flow in this component.

I'm not fully clear about the lifecycle of this component. I mostly noticed that the _isOpen state did not get put back to false, causing the modal to never reopen. It does seem to fix the odd behaviour people have been experiencing without breaking the existing test conditions.

jelhan · 2021-01-12T12:38:49Z

addon/components/bs-modal.js

@@ -473,7 +473,7 @@ export default class Modal extends Component {
    if (!this._isOpen) {
      return;
    }
-    this.isOpen = false;
+    this._isOpen = false;


Glad to see that we could actually drop @localCopy decorator. Less complexity is always good. 😄

Your changes look very good. I'm just a little bit confused why you haven't stuck with isOpen name. The is prefix is a helpful indicator that a property is a boolean in my opinion. In this special case I also fear that a property open might be confused with the show method.

Would it be okay for you to change the name back to isOpen? Not the implementation. That one is totally fine. I'm only talking about the naming.

SanderKnauff · 2021-01-12T14:03:28Z

I've added back the original test case as well now

simonihmig

Apart from the minor JSDoc issue (see inline comment) this seems good to me!

Tbh I was a bit puzzled that we were able to simply remove that @localCopy without any breaking changes, but as all tests are passing (including the newly added ones 👍), this seems to be the case.

Apparently I was also getting confused with those different states of the component. 🤦‍♂️
Initially that mutable isOpen copy of @open was used to be able to directly close the modal by user interaction (ESC, backdrop click) without mutating (and thus violating DDAU) the original @open (which was possible in the Ember.Component days, but luckily became impossible with Glimmer components). Whether you like that API or not (see discussion in #830), we can only change that as a breaking change API overhaul in a major version, not now. Anyways, we seem to already have mutable component state like showModal and inDom that is enough for this purpose, with requiring @localCopy...

Thanks a lot for this work here @SanderKnauff! 🙏

simonihmig · 2021-01-17T15:09:42Z

addon/components/bs-modal.js

+  /**
+   * @property open
+   * @private
+   */


Suggested change

/**

* @property open

* @private

*/

This JSDoc block should be removed, as it is wrongly stating that @open is private, and is in direct conflict with the doc block (referring to the same @property open) just a few lines above.

Good point! I've removed it now.

…/ember-bootstrap into fix-repeated-modal-openings

simonihmig · 2021-01-19T16:29:06Z

This looks good from my point of view. @jelhan I believe your feedback has been addressed, or is not relevant anymore (isOpen vs open), right? If so, I would be happy to merge this!

jelhan

Thanks a lot @SanderKnauff for working on this. Removing @localCopy decorator simplifies the code a lot. The two tests will help us to catch regressions earlier next time.

I still feel that we can (and should) improve the state flow in this component. But that can be done in a follow-up pull request.

simonihmig · 2021-01-19T16:37:04Z

Released: https://github.com/kaliber5/ember-bootstrap/releases/tag/v4.6.1 🎉

tbartelmess · 2021-01-21T16:23:57Z

Thank you!

Fix modals not being able to open after being closed once

b499c11

jelhan reviewed Jan 11, 2021

View reviewed changes

SanderKnauff added 2 commits January 12, 2021 01:24

Refactor out localCopy in favor of argument with default value

2025f21

Update test to verify issue #1386 as well

a8b831e

SanderKnauff closed this Jan 12, 2021

SanderKnauff reopened this Jan 12, 2021

Use this.set() syntax to ensure compatibility with Ember 3.16 LTS

aa69e30

jelhan requested changes Jan 12, 2021

View reviewed changes

Add test case for closing by setting @OPEN argument

6cef0cb

simonihmig reviewed Jan 17, 2021

View reviewed changes

SanderKnauff added 2 commits January 17, 2021 16:59

Remove redundant JSDock block

d1715f5

Merge branch 'fix-repeated-modal-openings' of github.com:SanderKnauff…

433f228

…/ember-bootstrap into fix-repeated-modal-openings

simonihmig approved these changes Jan 19, 2021

View reviewed changes

simonihmig added the bug label Jan 19, 2021

jelhan approved these changes Jan 19, 2021

View reviewed changes

jelhan merged commit 1e0fb6e into ember-bootstrap:master Jan 19, 2021

simonihmig mentioned this pull request Jan 19, 2021

Modal open property only updates once after update to 4.6.0 #1386

Closed

jelhan mentioned this pull request Jan 19, 2021

WIP: decouple internal and external open state of <BsModal> #1402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix modals not being able to open after being closed once #1393

Fix modals not being able to open after being closed once #1393

SanderKnauff commented Jan 11, 2021 •

edited

jelhan Jan 11, 2021

SanderKnauff Jan 12, 2021

jelhan Jan 12, 2021

jelhan commented Jan 11, 2021

SanderKnauff commented Jan 12, 2021

SanderKnauff commented Jan 12, 2021 •

edited

jelhan left a comment

jelhan Jan 12, 2021

simonihmig Jan 12, 2021

SanderKnauff Jan 12, 2021

jelhan Jan 12, 2021

SanderKnauff Jan 13, 2021

jelhan Jan 12, 2021

SanderKnauff commented Jan 12, 2021

simonihmig left a comment

simonihmig Jan 17, 2021

SanderKnauff Jan 17, 2021

simonihmig commented Jan 19, 2021

jelhan left a comment

simonihmig commented Jan 19, 2021

tbartelmess commented Jan 21, 2021

Fix modals not being able to open after being closed once #1393

Fix modals not being able to open after being closed once #1393

Conversation

SanderKnauff commented Jan 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelhan commented Jan 11, 2021

SanderKnauff commented Jan 12, 2021

SanderKnauff commented Jan 12, 2021 • edited

jelhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanderKnauff commented Jan 12, 2021

simonihmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Jan 19, 2021

jelhan left a comment

Choose a reason for hiding this comment

simonihmig commented Jan 19, 2021

tbartelmess commented Jan 21, 2021

SanderKnauff commented Jan 11, 2021 •

edited

SanderKnauff commented Jan 12, 2021 •

edited