<Dialog /> is not a controlled component

[hronro]

Example:

import React, { Component } from 'react'
import { render } from 'react-dom'

import Dialog, {
  DialogContent,
} from '@material/react-dialog';

class AlwaysOpenedDialog extends Component {
  state = {
    open: true
  }

  render () {
    return (
      <Dialog
        open={this.state.open}
        onClose={() => {
          // I won't do anything in `onClose` so the dialog is expected to be always opened.

          // In a more normally case, I want to close the dialog conditionally
          // if (this.validate()) {
          //   this.setState({open: false})
          // } else {
          //   this.setState(null)
          // }
        }}
      >
        Hello World
      </Dialog>
    )
  }
}

render(<AlwaysOpenedDialog />, document.getElementById('root'))

When I click the mask, the dialog still closed.

According to the React docs, just like the native input element in React, <input type="text" value={this.state.value} onChange={this.handleChange} />, when you try to type something in the input element, this would trigger onChange event, but if you don't change this.state.value in the onChange event handler, the value of this input element won't changed, just like this input element is being locked.

In my opinion, the correct logic for Dialog component is that, when users try to close the dialog(via mouse click or keyboard), this would trigger onClose event, but finally the dialog is closed or not, is depending on the prop open, which respect React controlled components pattern.

[moog16]

Thanks for opening this issue. I realize that this and other components in this library do not follow the controlled component pattern. This repo's main goal is to implement the foundation/adapter pattern of MDC Web.

RE: When onClose is called. Are you saying you do not expect the onClose method to be called on a mouse click/keyboard event?

[hronro]

@moog16 No, I'm saying when users click to close the dialog, this would only trigger the onClose method, but not close the dialog directly. The dialog is finally closed or not is only depend on the prop open is changed or not.

[moog16]

@foisonocean ahhh gotcha - I understand what you're saying now, sorry for the confusion. I do see how this breaks that pattern as you describe, especially when thinking of the onChange method on a input type='text'. I do not have time to get to this until after April (if that). Would you or anyone else like to take a stab?

[dawsonc623]

@foisonocean If you have not started working on this, I can take a look.

[moog16]

I've also noticed this the controlled component pattern is not working in checkbox either...I haven't checked, but this is probably not the case in many of the other components.

@dawsonc623 if you start working on this, this will be a BreakingChange. I think this should be opt-in for the first two months, with a console.warning that says we're going to deprecate. Does that make sense? Similar to what this issue is referencing.

[dawsonc623]

@moog16 That makes sense. Opt-in might be a little tricky - do we just want to make it so if open is present the component is "controlled" otherwise it behaves as it does today? Apart from adding a temporary prop (which I want to avoid if possible) I am not sure how we could retain the way open and onClose work today and still know when the user expects the component to be controlled.

[moog16]

Ya - I think the only way we could do that is with a temporary prop...If we have that, people could slowly migrate over. Its not the best, but its the simplest thing I can think of. I also don't want to break people that have been using this component, and they should be given ample time to upgrade. So let's go with the temporary prop. Could you make sure to comment next to it and open an issue to remove it as well?

[moog16]

@dawsonc623 please be advised @gugu is updating dialog: https://github.com/material-components/material-components-web-react/pull/779/files. You might want to branch from this branch as his code might get in before yours. I'm not sure how quickly you'll finish.

[dawsonc623]

@moog16 I saw that. I plan on branching off of that branch, and if I happen to finish before that gets merged in we can just wait to merge mine until that one is in. That work is already done, so I do not want to risk mucking up that PR with my work.

[moog16]

Cool - Sounds good!

[moog16]

I opened #785 to eventually follow up on the others as well.

[dawsonc623]

@moog16 I have been running through the flow of how Dialogs are closed, and this is an interesting case. The challenge here is that there is no way to "cancel" a close; once the foundation object knows about the close interaction occurring, unless escapeKeyAction or scrimClickAction are set to empty strings the Dialog will close. There is not really anything the React component can do to stop it at that point. I can think of two solutions to this (if you can think of better ones feel free to share them):

1. Have the React component adopt the logic contained in MDCDialogFoundation's handleInteraction and handleDocumentKeydown methods. This solves the problem by allowing the React component control the exact conditions under which close is called. The major downside is the code duplication. I would like to avoid this solution if at all possible.

2. Refactor the MDCDialogFoundation to allow the close process to be cancelled. Since there is the notifyClosing hook that is fired before any of the actual close logic begins, this could be done by either allowing notifyClosing to return false to cancel, or adding a cancelClose method which could be called from the notifyClosing hook. While semantically it seems weird that a controlled component would have to "cancel" a close versus preventing one from occurring at all, I do like this option - especially compared to the other.

I will keep looking at it and maybe fiddle around with what it would take to implement option 2 in case it helps think of something else that does not require code duplication or reaching into the foundation code (or to get a head start if option 2 is what we decide to go with).

As an aside, I had a little difficulty getting unit tests to run through the Windows Subsystem for Linux on Windows 10, but Karma can be configured to do it. I did not see any documentation related to this, so I am curious if we wanted to add documentation for getting that working (or even add a karma.wsl.conf file for developers using WSL).

[moog16]

@dawsonc623 I'll have to bring this up during our issue triage if it requires MDC web changes. This would also be a pattern change, since there are no cancellable events in Web. And I do agree that it would be a lot nicer to not have to duplicate logic.

RE Windows: We have no confirmed reports of anyone developing on windows. It would great to get some documentation if you get it up and running. :)

[moog16]

@dawsonc623 I haven't forgottten about this...we have a meeting today with the team to discuss. :)

[moog16]

@dawsonc623 - we had a discussion about MDC Web issue #4357 and this issue. I was wondering if you have any bandwidth to experiment with your proposed solution. We liked your #2 idea, but we aren't sure how it will pan out. Let us know :)

[dawsonc623]

@moog16 I am more than happy to dig into it. I will experiment and let you know how it goes (with any luck in PR form :) ).

[moog16]

Sweet! LMK if you need any help with anything. Feel free to ping me here or on our discord channel.

[moog16]

@lucasecdb shared an interesting article highlighting patterns that should be avoided (which I agree with). Dan explains that we shouldn't have state within props, which should result in a controlled component. I just wanted to share this for reference when we get to this.