search

fomantic / Fomantic-UI

Fomantic-UI is the official community fork of Semantic-UI
https://fomantic-ui.com
MIT License
3.54k stars 329 forks source link

[Modal] "show" does not recenter modal #2920

Open mvorisek opened 10 months ago

mvorisek commented 10 months ago

Bug Report

Steps to reproduce

  1. open https://dev.atk4.org/demos/_unit-test/late-output-error.php
  2. click first button (Test LateOutputError I: Headers already sent)
  3. notice the modal is NOT vertically centered

The demo closes another modal right before the final modal is shown, but when I put $(this).modal('refresh'); at https://github.com/atk4/ui/blob/5.0.0/js/src/services/modal.service.js#L44 line the problem is gone, so it seems the "show" does not explicitly refresh the modal at all.

Expected result

.modal('show') must always refresh/recenter the modal

Screenshot (if possible)

image

Version

2.9.3

mvorisek commented 10 months ago

The repro demo does no longer reproduce the issue, as we fixed it on our side temporary: https://github.com/atk4/ui/blob/7cc2e65c144480962433dd4aef5ef351380dace2/js/src/services/modal.service.js#L48

To reproduce it, download the demo (html, js, + the loaded page using AJAX) locally and remove the fix.

It seems the "refresh on show" was removed in https://github.com/fomantic/Fomantic-UI/pull/2477/commits (in last commit) - what about reverting it (reintroducing forced refresh on show) until https://github.com/fomantic/Fomantic-UI/issues/2476 is fixed?

UPDATE: I looked into the latest JS code and please have a look at https://github.com/fomantic/Fomantic-UI/issues/2476#issuecomment-1763039845 repro. The repro is changing classes not only style, and such changes should be observeable and causing always a refresh when observeChanges: true.

lubber-de commented 10 months ago

UPDATE: I looked into the latest JS code and please have a look at #2476 (comment) repro. The repro is changing classes not only style, and such changes should be observeable and causing always a refresh when observeChanges: true.

Always triggering a refresh on any classname change within the whole modal will almost always trigger a refresh especially for modules like dropdown or other elements which make use of the transition module / classes. I fear a big performance lag here then. Especially as the "refresh()" method itself also changes classnames inside the modal (e.g. the "scrolling" or "loading" class)

lubber-de commented 8 months ago

Should be fixed by #2969. The new Resizeobserver automatically triggers a refresh when a modal gets shown so a separate refresh on show isnt needed