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] not all changes are observed even with observeChanges: true #2476

Open
mvorisek opened this issue Oct 2, 2022 · 16 comments
Open

[Modal] not all changes are observed even with observeChanges: true #2476

mvorisek opened this issue Oct 2, 2022 · 16 comments
Assignees
Labels
state/has-pr An issue which has a related PR open type/bug Any issue which is a bug or PR which fixes a bug
Milestone

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Oct 2, 2022

Bug Report

When I modify the demo from #2036 with textarea - https://jsfiddle.net/2w493cyo/1/ - and resize the textarea vertically to force the modal height to be greater than the viewport, no scrollbar is shown. But when I change/maximalize browser window size, it trigger some internal change and scrollbar is added, even if nothing in the modal is changed. It seems not enough changes are observed.

Steps to reproduce

  1. open https://jsfiddle.net/2w493cyo/1/
  2. resize textare by dragging the bottom-right corner to make the modal height overflow
  3. notice no scrollbar

Expected result

image

Actual result

no vertical scrollbar after modal height overflow

image

@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 2, 2022
@lubber-de lubber-de removed state/awaiting-investigation Anything which needs more investigation state/awaiting-triage Any issues or pull requests which haven't yet been triaged labels Oct 3, 2022
@lubber-de
Copy link
Member

lubber-de commented Oct 3, 2022

Fixed by #2477 [Edit, no it's not, see comments]

@lubber-de lubber-de added the state/has-pr An issue which has a related PR open label Oct 3, 2022
@lubber-de lubber-de added this to the 2.9.0 milestone Oct 3, 2022
@lubber-de
Copy link
Member

I rejected my proposed changes from the mentioned PR as a fix for this use case does not seem to be trivial (see PR comments)

@lubber-de lubber-de added state/awaiting-investigation Anything which needs more investigation Hacktoberfest Issues for Hacktoberfest! and removed state/has-pr An issue which has a related PR open labels Oct 4, 2022
@lubber-de lubber-de removed this from the 2.9.0 milestone Oct 4, 2022
@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 4, 2022

The #2477 PR discovered observing the changed size is easy, but the problem is a modal refresh (called when a change is observed) is currently problematic, it is breaking/stopping a mouse drag.

https://jsfiddle.net/v2q30wsx/1/ confirms an observer itself isn't causing the drag event to stop.

@lubber-de
Copy link
Member

lubber-de commented Oct 4, 2022

The refresh is the intention of the Mutationobserver. I debugged it down to the setting of the scrolling class which are causing the issue

$dimmable.addClass(className.scrolling);
$module.addClass(className.scrolling);

When this happens, the browser internally seems to adjust the viewport height which in turn seems to break the Observer as its base (the changed style) again changes in the same context, although it should only observe the modals changes, not the dimmer and not body.

I tried to do the refresh asynchronously and also tried to disconnect the observer , then do the refresh, then restart the observer, but this does not fix it. The window resize event also does a refresh (via requestanimationframe, but i tried this in the observer as well without success) but the windows resize event seems to work differently as it does not bug there.

This was really giving me a headache today

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 4, 2022

Not sure if it helps, but as long we need to monitor the modal size change only (and no changes inside), ResizeObserver should fix the recursive firing, the docs https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver say:

ResizeObserver avoids infinite callback loops and cyclic dependencies that are often created when resizing via a callback function

@lubber-de
Copy link
Member

Not sure if it helps, but as long we need to monitor the modal size change only (and no changes inside), ResizeObserver should fix the recursive firing, the docs [developer.mozilla.org/en-US/docs/Web/API/ResizeObserver]

Just tried that, also does not work in firefox blocks drag when reaching end of viewport just as before

          if('ResizeObserver' in window && settings.observeChanges) {
            observer2 = new ResizeObserver(function(mutations) {
              module.refresh();
            });
            observer2.observe(element);
          }

works fine in chrome though

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 4, 2022

When an empty observer callback (/wo modal refresh) is called, the drag is working. So it seems some operation in the modal refresh is causing the drag to stop. Maybe some DOM operation there can be done differently.

@mvorisek
Copy link
Contributor Author

Any attribute in https://github.com/fomantic/Fomantic-UI/blob/afea456/src/definitions/modules/modal.js#L311 should be observed as any attribute can be used in CSS selector, ie. can change modal height.

The "dragging" problem in Firefox is not caused by observer itself. The problem is refresh mutates something when invoked.

@mvorisek
Copy link
Contributor Author

another repro:

  1. open https://ui.atk4.org/demos/data-action/jsactionscrud.php
  2. click "Actions" on any row
  3. click "Edit" in the opened menu
  4. click into the "Parent Folder ID" dropdown
  5. select some value like "workflows"
  6. notice the modal is moved up wrongly (not centered anymore)

@lubber-de
Copy link
Member

lubber-de commented Oct 15, 2023

another repro:

  1. open ui.atk4.org/demos/data-action/jsactionscrud.php
  2. click "Actions" on any row
  3. click "Edit" in the opened menu
  4. click into the "Parent Folder ID" dropdown
  5. select some value like "workflows"
  6. notice the modal is moved up wrongly (not centered anymore)

I tried to reproduce this in a simple jsfiddle using a simplified variant of your modal
https://jsfiddle.net/lubber/mdpcvsx5/6/

When i select anything from the dropdown, the modal does not move. Is something else happening in your code when you select anything from the dropdown?

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 15, 2023

I tried to reproduce this in a simple jsfiddle using a simplied variant of your modal https://jsfiddle.net/lubber/mdpcvsx5/6/

When i select anything from the dropdown, the modal does not move. Is something else happening in your code when you select anything from the dropdown?

here is working repro - https://jsfiddle.net/qLog153p/ - observeChanges: true was missing, use 80% browser zoom level for more visible jump.

@lubber-de
Copy link
Member

lubber-de commented Oct 15, 2023

here is working repro - jsfiddle.net/qLog153p - observeChanges: true was missing.

I still cannot get this to happen (tried edge/chrome/firefox/70% browser zoom, windows 11)
However, i was able to reproduce in your original atk demo website (where i copied the modal html code from), that's why i am asking if your dropdowns onchange or anything else changes anything to the modal after selection.

I just want to figure out the reason before "just" adding the refresh back to the show method as i believe we might have to fix something else (too)
The reason i removed it back then was, that, without a DOM change and observeChanges:true, a refresh isnt really needed as a dom change would already trigger this

@mvorisek
Copy link
Contributor Author

please try different zoom levels in Firefox, the reason is when modal is too big, the centering is not applicable, when the modal is too small, the extra dropdown height has no effect

exact repro steps:

  1. open https://jsfiddle.net/qLog153p/
  2. in Firefox (but I belive the issue is present everywhere), with browser window about 1900 x 1000 px in size
  3. try different zoom levels: 110, 100, 90, 80, 70 % (ie. repeat the steps below for each zoom level)
  4. ctrl + f5 (refresh after browser zoom level is needed!)
  5. click inside the dropdown
  6. click "workflows" item
  7. the dropdown should close
  8. at least in some zoom level, you should be able to notice the modal jumps up

please give it at least several tries (for each fresh refresh and setting a browser zoom level), I have noticed I was not able to reproduce it if the browser window width! was too small, not sure why, maybe it is subject to some race conditions

@ko2in
Copy link
Member

ko2in commented Oct 15, 2023

I reproduced this with 150% zoom level.

modal-demo

@ko2in
Copy link
Member

ko2in commented Oct 15, 2023

But, if I refreshed the page at zoom level 150%, then I couldn't reproduce this anymore. I tried several zoom level and it doesn't move a bit.

I could only reproduced with initial zoom level is less than 150% and the modal shifted to the top when zoom in to 150%.

I tried with the initial zoom level >= 150% and then zoom out to 150% and below and the modal is still working fine.

@lubber-de lubber-de self-assigned this Oct 17, 2023
@lubber-de lubber-de removed the Hacktoberfest Issues for Hacktoberfest! label Nov 17, 2023
@lubber-de lubber-de added this to the 2.9.4 milestone Nov 17, 2023
@lubber-de lubber-de removed the state/awaiting-investigation Anything which needs more investigation label Nov 17, 2023
@lubber-de
Copy link
Member

Finally fixed by #2969. Reason why firefox bugged back then was the ::after selector to keep a bottom margin

@lubber-de lubber-de added the state/has-pr An issue which has a related PR open label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/has-pr An issue which has a related PR open 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