Clay: Bug: Clay modal closes when clicking on an element that removes itself

[thektan]

This is with @clayui/modal v3.0.0-milestone.1

Demo: https://codesandbox.io/s/clay-modal-close-bug-3rhwc

To reproduce:

1. Open modal
2. Click on one
3. Click on two

The "Two" button onClick hides itself which causes the modal to close even though it doesn't explicitly call the close function. I believe the modal is closing since it thinks the click happened outside of the modal which automatically closes the modal.

[bryceosterhaus]

codesandbox demos make me so happy.

[matuzalemsteles]

hey @thektan, I gave a quick investigation and it's something very interesting, it ends up closing by the following: When you click on the one button and then on two it updates the state to false and removes the button from the body node and then it happens the trigger of the click event of the document and it finishes that we verify if the event.target exists inside the Modal, as it does not exist, it closes the modal.

if (
    elementRef.current &&
    event.target !== null &&
    !elementRef.current.contains(event.target as HTMLDivElement)
) {...}

I think we can improve our checking to cover these cases.

[julien]

Hey @matuzalemsteles @bryceosterhaus this is still happening to me too.

In my case I'm not really sure how easy it will be to create a demo on codesanbox because I'm using other "non" clay components, but I will try to as soon as I can, but I think you can re-open this one.

Maybe I'm understanding this incorrectly, but why would the modal close itself if you click outside?
Couldn't we add a prop to prevent this?

Thanks

[julien]

Talking with @carloslancha he told me, this had already happened and been fixed in 2.x, I know the code is not the same, but maybe we could check how it was done and try to fix it in the same way for 3.x.

[matuzalemsteles]

Maybe I'm understanding this incorrectly, but why would the modal close itself if you click outside?
Couldn't we add a prop to prevent this?

@julien This is expected behavior of Modal, whenever clicking outside Modal must close it. We could add a rule to disable this but you would have to deal with it.

https://github.com/liferay/clay/blob/master/packages/clay-modal/src/Hook.ts#L81-L90

Talking with @carloslancha he told me, this had already happened and been fixed in 2.x, I know the code is not the same, but maybe we could check how it was done and try to fix it in the same way for 3.x.

We actually followed the same behavior as we did in 2.x but had to change due to not working with the cases of this issue.

[julien]

We could add a rule to disable this but you would have to deal with it.

Yes, that's exactly what I'd like to do.

We actually followed the same behavior as we did in 2.x but had to change due to not working with the cases of this issue.

I think it might be a good to take in account all the use cases and issues that we faced with 2.x (whether they were fixed or not), because it could save some time. So I'm reopening this issue.

[julien]

@matuzalemsteles another thing I wanted to ask concerning these lines, is it normal that we aren't checking that the event has been prevented? I guess that could solve my issue

[matuzalemsteles]

I think it might be a good to take in account all the use cases and issues that we faced with 2.x (whether they were fixed or not), because it could save some time...

Yeah, I'm considering a lot of the problems we had there, but I can't reuse most cases because the library change changes a lot and as we are composing the components, other behaviors appear...

@matuzalemsteles another thing I wanted to ask concerning these lines, is it normal that we aren't checking that the event has been prevented? I guess that could solve my issue

As we wanted Modal to always close when clicking off, we didn't check if the event is prevented. Can we check if this solves your case? I think it would be better than adding an API just for these cases.

[julien]

@matuzalemsteles it's pretty clear to me that @clayui packages are mainly used and going to be used in portal, so I'd expect it to consider those use cases by default, just my 2 cents.

[matuzalemsteles]

@matuzalemsteles it's pretty clear to me that @clayui packages are mainly used and going to be used in portal, so I'd expect it to consider those use cases by default, just my 2 cents.

Sure, let's consider all the use cases and evaluate the best way to solve it... we just don't want to escape the idea of ​​being a generic component library and not bring all the problems we have in the Portal here. Some things we will have to deal with within the Portal. Otherwise it would make more sense for Clay to be inside the Portal infrastructure. Does that make sense to you?

[wincent]

Sure, let's consider all the use cases and evaluate the best way to solve it... we just don't want to escape the idea of ​​being a generic component library and not bring all the problems we have in the Portal here.

I think we're in danger of straying from our mission here; specifically (emphasis added):

To help, support and guide Liferay teams to succeed and deliver robust, safe, performant and dynamic experiences in time and with high quality

To me that makes it pretty clear that Clay should be optimized for delivery of Lexicon applications within Portal. Any more generic reusability of that is a lovely bonus, but if we focus towards being "generic" and explicitly avoid "the problems we have in ... Portal", then that sounds like a failure to fulfil our mission: the problems we have in Portal are exactly the ones that are most important. That product is literally the reason our team exists and why we all have jobs, after all. 😂

[matuzalemsteles]

Oh no, I understand perfectly 😅. EstouI just mean that the generic should support everyone, Liferay-related teams. Be DXP, Analytics Cloud... in the Liferay context.

In the context of this particular problem, it is something that we must carefully evaluate. Because Lexicon expects no more than one Modal to open on screen and expects Modal to close when it is clicked off, in the context of the Portal this is not true 🤷‍♂️. So it wouldn't necessarily be a bug, but we can make it easier for people to keep opening Modal through another Modal. Like I said, let's evaluate what will be the best way to handle this.

• Opening an API to disable may deviate from the standard to support what goes against Lexicon rules and may give the idea that we are supporting this standard.
• We can check if the event was prevented before closing Modal, so we can support people to handle it.
• People can call event.stopPropagation() so it doesn't get to our event... we have to test if this will work as expected.

So that is why I get the idea of ​​checking if the event has been prevented, since we will not necessarily be breaking the rules on our side but we will still be supporting people who already have this standard and at some point in the future they will remove This or Lexicon will adopt some kind of strategy to limit.

[jbalsas]

I think we're pretty much aligned here, it's just a matter on where do we need to tighten the screws.

As a team, we control both Clay as a project and its integration in DXP. So we do have 2 different places where we can tweak some things or provide infrastructure (components, helpers, wrappers...).

The easier Clay is to use, the better for everyone. That being said, it should also be okay to provide additional helpers in portal (at the cost of undermining Clay a little bit).

For this example, maybe we could provide some wrapper function that takes a ClayModal and returns a "StackableClayModal" or something like that. (just a thought).

As @wincent kindly remarked:

To help, support and guide Liferay teams to succeed and deliver robust, safe, performant and dynamic experiences in time and with high quality

Let's always keep that in mind and make sure we use all tools at our disposal and work as a team to make that a reality! ☺️