Skip to content

[fixed] Prevent unintended register and unregister #808 #1040

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fixed] Prevent unintended register and unregister #808 #1040

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions src/components/ModalPortal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export default class ModalPortal extends Component {
}

componentWillUnmount() {
if (this.state.isOpen) {
if (this.props.isOpen) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a question for you, @diasbruno, as it's not strictly related to this change -- it looks like the only actual use of this.state.isOpen is this block:

    // Focus only needs to be set once when the modal is being opened
    if (
      this.props.shouldFocusAfterRender &&
      this.state.isOpen &&
      !prevState.isOpen
    ) {
      this.focusContent();
    }

Is that correct? 🤔 Is there a way we could accomplish the programmatic focus of the content without maintaining a this.state.isOpen variable alongside this.props.isOpen? The simplification may be nice, if so.

Copy link
Collaborator

@diasbruno diasbruno Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state inside the modal is used to manage the timed transition (sync'd with CSS).

When the isOpen flag is changed from true to false, the internal state is true.
react-modal will wait the css transition N milliseconds, then set to false
(same when you are openning).
When the transition has finished, we then do the tear down any other resources
(applied css names to document.body and others).

I always wanted to implement the transition mechanism using css transition events,
but never had a change to see the support for it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh! Obrigado pela explicação. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always wanted to implement the transition mechanism using css transition events,
but never had a change to see the support for it.

It would be more reliable than setting timers to manage the internal state.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be cool! I did see this v4 issue about that: #993

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proposed fix does not work if the modal is closed with a timeout. isOpen will be false when we get here, so afterClose() won't run and the body class will be left on the DOM.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proposed fix does not work if the modal is closed with a timeout. isOpen will be false when we get here, so afterClose() won't run and the body class will be left on the DOM.

@gillesbouvier-qz Could you please provide me with an example? It would be helpful. Thanks!

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you foregoing support for this?

isOpen && (
  <Modal
    isOpen={isOpen}
    closeTimeoutMS={300}
    onRequestClose={() => {
      setIsOpen(false);
    }}
    ...
  />
)

If you still want to support the above, I believe isOpen will be false during unmounting and afterClose() won't run leaving the body class in place (and onAfterClose() won't run either). Using the isOpen props seems unsafe to me, as only the modal component should control when it is open or closed.

Maybe I'm missing something?

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this solve both problems at once?

Suggested change
if (this.props.isOpen) {
clearTimeout(this.closeTimer);
if (this.props.isOpen || this.closeTimer) {
this.closeWithoutTimeout();
}

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It solves the problem for in my testcase:

ModalPortal.js:164 CLOSING MODAL WITH TIMEOUT 300
ModalPortal.js:177 CLOSE WITH TIMEOUT
ModalPortal.js:322 UN-MOUNTING
ModalPortal.js:184 CLOSE WITHOUT TIMEOUT
ModalPortal.js:100 AFTER CLOSE

the idea being that we always want afterClose() to run at least once even when unmounting.

this.afterClose();
}
clearTimeout(this.closeTimer);
Expand Down Expand Up @@ -227,11 +227,11 @@ export default class ModalPortal extends Component {
};

open = () => {
this.beforeOpen();
if (this.state.afterOpen && this.state.beforeClose) {
clearTimeout(this.closeTimer);
this.setState({ beforeClose: false });
} else {
this.beforeOpen();
if (this.props.shouldFocusAfterRender) {
focusManager.setupScopedFocus(this.node);
focusManager.markForFocusLater();
Expand Down Expand Up @@ -268,24 +268,21 @@ export default class ModalPortal extends Component {

closeWithTimeout = () => {
const closesAt = Date.now() + this.props.closeTimeoutMS;
this.setState({ beforeClose: true, closesAt }, () => {
this.closeTimer = setTimeout(
this.closeWithoutTimeout,
this.state.closesAt - Date.now()
);
});
this.setState({ beforeClose: true, closesAt });
this.closeTimer = setTimeout(
this.closeWithoutTimeout,
this.props.closeTimeoutMS
);
};

closeWithoutTimeout = () => {
this.setState(
{
beforeClose: false,
isOpen: false,
afterOpen: false,
closesAt: null
},
this.afterClose
);
this.setState({
beforeClose: false,
isOpen: false,
afterOpen: false,
closesAt: null
});
this.afterClose();
};

handleKeyDown = event => {
Expand Down