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

Modals launching other Modals no longer work #17716

Closed
germain-gg opened this issue Jun 21, 2021 · 5 comments · Fixed by matrix-org/matrix-react-sdk#6238
Closed

Modals launching other Modals no longer work #17716

germain-gg opened this issue Jun 21, 2021 · 5 comments · Fixed by matrix-org/matrix-react-sdk#6238
Assignees
Labels
A-Feedback-Reporting Reporting process for bugs, debug logs (rageshakes), suggestions P1 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression X-Release-Blocker

Comments

@germain-gg
Copy link
Contributor

germain-gg commented Jun 21, 2021

Screen Shot 2021-06-21 at 16 36 02

@germain-gg germain-gg added P2 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect labels Jun 21, 2021
@aaronraimist aaronraimist added A-Feedback-Reporting Reporting process for bugs, debug logs (rageshakes), suggestions X-Regression labels Jun 21, 2021
@aaronraimist
Copy link
Collaborator

also if you try to reproduce this, the sign out button will stop working until you refresh the page

@HarHarLinks
Copy link
Contributor

related or not related, but also happened to me here: #17593 (comment)

@t3chguy t3chguy self-assigned this Jun 21, 2021
@t3chguy
Copy link
Member

t3chguy commented Jun 21, 2021

Regressed by matrix-org/matrix-react-sdk#6185

Actually this looks related to the React 17 upgrade

@t3chguy
Copy link
Member

t3chguy commented Jun 21, 2021

So I think we're hitting some race inside the guts of React.
If I add a setImmediate on the ReactDOM.render call or I set up a breakpoint then it works fine.

Index: src/Modal.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/Modal.tsx	(revision 54c3832b5b9dde5981eb1934f599e7b75eac00e8)
+++ src/Modal.tsx	(date 1624303471241)
@@ -385,7 +385,7 @@
                 </div>
             );
 
-            ReactDOM.render(dialog, ModalManager.getOrCreateContainer());
+            setImmediate(() => ReactDOM.render(dialog, ModalManager.getOrCreateContainer()));
         } else {
             // This is safe to call repeatedly if we happen to do that
             ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateContainer());

Not sure what the best way to proceed is here

This probably affects other modals which open other modals so marking as Major

Yup, just found that the Keywords modal doesn't work either, this one has an error though:

Warning: render(...): It looks like the React-rendered content of this container was removed without using React. This is not supported and will cause errors. Instead, call ReactDOM.unmountComponentAtNode to empty a container.

Both appear to be fixed by the setImmediate

@t3chguy t3chguy removed their assignment Jun 21, 2021
@t3chguy t3chguy added the S-Major Severely degrades major functionality or product features, with no satisfactory workaround label Jun 21, 2021
@t3chguy t3chguy changed the title Clicking 'debug logs' link in the feedback modal does not do anything Modals launching other Modals no longer work Jun 21, 2021
@aaronraimist aaronraimist removed the S-Minor Impairs non-critical functionality or suitable workarounds exist label Jun 22, 2021
@SimonBrandner SimonBrandner added P1 and removed P2 labels Jun 22, 2021
@t3chguy
Copy link
Member

t3chguy commented Jun 22, 2021

Actually the keywords modal was only broken because I attempted to visit send debug logs. So this looks to affect non-priority non-static modals when one closes itself and then opens another in the very same tick.

@t3chguy t3chguy added S-Minor Impairs non-critical functionality or suitable workarounds exist and removed S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Jun 22, 2021
@germain-gg germain-gg self-assigned this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Feedback-Reporting Reporting process for bugs, debug logs (rageshakes), suggestions P1 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression X-Release-Blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants