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

[modal] m2.onShow is emit before m1.onHide since Fomantic v2.9.0 #2499

Closed
mvorisek opened this issue Oct 14, 2022 · 13 comments
Closed

[modal] m2.onShow is emit before m1.onHide since Fomantic v2.9.0 #2499

mvorisek opened this issue Oct 14, 2022 · 13 comments
Labels
needs-more-info state/awaiting-investigation Anything which needs more investigation type/bug Any issue which is a bug or PR which fixes a bug

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Oct 14, 2022

Bug Report

UPDATE: see #2499 (comment) post below first

in 2.8.8 it was working, verified by reverting only the Fomantic-UI dist files

Steps to reproduce

  1. open https://dev.agiletoolkit.org/demos/form/form.php (since atk4/ui@e712791 the JS files are not minified :) )
  2. click Handler Safety tab
  3. click Modal Test button
  4. click Save button (inside the opened modal)
  5. notice dimmed background but no modal window/content

Expected result

modal is open as in https://ui.agiletoolkit.org/demos/form/form.php (notice ui. instead of dev. subdomain)

Actual result

blured background, no modal shown

Version

2.9.0

@mvorisek mvorisek added state/awaiting-investigation Anything which needs more investigation state/awaiting-triage Any issues or pull requests which haven't yet been triaged type/bug Any issue which is a bug or PR which fixes a bug labels Oct 14, 2022
@jamessampford
Copy link
Contributor

The Dev link opens the modal for me ok?

@lubber-de
Copy link
Member

I also cannot reproduce. The modal opens just fine

@mvorisek
Copy link
Contributor Author

I am sorry about the initial description, I missed last important step to reproduce.

tested in Firefox and Chrome, in both reproduced

@jamessampford
Copy link
Contributor

Also fine in Chrome (Android/Windows) and Chromium Edge ... Suggest creating a jsfiddle showing the issue

@mvorisek
Copy link
Contributor Author

@jamessampford please retest with the fixed steps to reproduce (in the issue description), I can reproduce it even in Android 9 mobile browser

also make sure you are testing it on the dev. subdomain which is the only one which uses Fomantic-UI v2.9.0

@jamessampford
Copy link
Contributor

That makes a bit of difference 😂

@lubber-de
Copy link
Member

lubber-de commented Oct 14, 2022

This needs local debugging, sorry i cannot figure out the reason from the somehow obfuscated webpack modified source and the related backend ajax calls.

Please try to isolate the issue into a simple jsfiddle. It seems to be something related to allowMultiple together with some api callbacks (?) and/or transition and/or custom code (?)

What you can try:

  • enable debug and verbose ( i did this in console, what it tells us is that the second modal does not get animated because the previous modal is , by whatever reason, still visible, but has inline style opacity:0 (i dont think, this comes from FUI)
  • try to use 2.9.0 but the modal.js from 2.8.8. If this helps, we at least know it's not related to transition or dimmer. If the error still occurs, try to use transition.js from 2.8.8

@lubber-de lubber-de added needs-more-info and removed state/awaiting-triage Any issues or pull requests which haven't yet been triaged labels Oct 14, 2022
@mvorisek
Copy link
Contributor Author

the problem is present since 883f912 (#2213)

the error modal (after Save button is pressed) is created at https://github.com/atk4/ui/blob/c535b9b45804394905e27506dae7c3df31f9f6d3/js/src/services/api.service.js#L198

in our setup some Fomantic-UI modal settings are overrided in https://github.com/atk4/ui/blob/c535b9b45804394905e27506dae7c3df31f9f6d3/js/src/services/modal.service.js

@mvorisek
Copy link
Contributor Author

Hi @lubber-de, looking deeped into the problem the 883f912 change has reordered onShow/onHide in our application.

Event order with v2.8.8:

image

Event order with v2.9.0:

image

onHidden/onVisible events are missing because of consequence of this issue, our app returns false in m1.onHide as m2.onShow was fired first and it thinks m1 is not the topmost modal because of it

I tried to create a jsfiddle for this issue, I was able to create jsfiddle for related onVisible/onHidden problem - #2515 - but not for onShow/onHide.

The problem from this issue is reproducible using the steps provided in this issue description. Would you be please so kind to look at it? once the problem is known, the jsfiddle from #2515 can be probably used as a base

@mvorisek mvorisek changed the title [modal] Modal is not showing after Fomantic-UI upgrade from 2.8.8 to 2.9.0 [modal] m2.onShow is emit before m1.onHide in when m1.allowMultiple is set Oct 19, 2022
@mvorisek mvorisek changed the title [modal] m2.onShow is emit before m1.onHide in when m1.allowMultiple is set [modal] m2.onShow is emit before m1.onHide since Fomantic v2.9.0 Oct 19, 2022
@lubber-de
Copy link
Member

As said in th other thread, each modal events do basically not depend on other modals. And if you return false in onhide the related modal stays visible, which was the intention of the PR to make such callbacks cancellable.

We should focus on one issue (this one) instead of creating new issues for the same problem, please.

@lubber-de
Copy link
Member

Btw_ quick thought: The use case "allowMultiple: false" in the second modal, while it was openend out of a modal where "allowMultiple:true" was set, is not supported (and does not make much sense IMHO), as it tries to close all other modals, even if the previous modal had allowMultiple: true. We should either try to fetch this somehow.

@mvorisek
Copy link
Contributor Author

if you return false in onhide the related modal stays visible, which was the intention of the PR to make such callbacks cancellable

this is a consequence of this issue, if onHide would be fired before onShow, the atk4/ui will return correct onHide result

As said in th other thread, each modal events do basically not depend on other modals.

modals are dependent on each other, call order determines how the modals are stacked, if only a single modal is to be shown (allowMultiple: false), then m1.onHidel must be definitely called before the m2.onShow

can this be easily fixed?

The use case "allowMultiple: false" in the second modal, while it was openend out of a modal where "allowMultiple:true" was set, is not supported (and does not make much sense IMHO), as it tries to close all other modals, even if the previous modal had allowMultiple: true. We should either try to fetch this somehow.

IMHO allowMultiple should determine if the already displayed modals should be kept displayed or hidden

imagine 3 modals:

m1 with allowMultiple: false
m2 with allowMultiple: true
m3 with allowMultiple: false

m1.show - show m1
m2.show - keep all modals (m1) shown, show m2
m3.show - hide all modals (m2), open m3

once user returns from m3, the previous modal stack (m2) should be fully restored

but this seems like a feature request, for atk4/ui we need to probably handle it by ourself and set allowMultiple everywhere

@mvorisek
Copy link
Contributor Author

Thank you for guiding me into the right direction. I am closing this issue as with allowMultiple: true everywhere the onShow/onHide order issue is gone and mixed (false/true) allowMultiple is not supported. Maybe a console warning for mixed usage should be shown.

Currently, nesting with invisible non-front modals is not supported natively and a custom code like https://github.com/atk4/ui/blob/6d98fafb8d21db20bb8461b306ca3f64e9123282/js/src/services/modal.service.js#L41-L63 is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-info state/awaiting-investigation Anything which needs more investigation type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants