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

[Dialog] Form + scroll in DialogContent #13253

Open
2 tasks done
bogdansoare opened this issue Oct 15, 2018 · 27 comments
Open
2 tasks done

[Dialog] Form + scroll in DialogContent #13253

bogdansoare opened this issue Oct 15, 2018 · 27 comments
Labels
component: dialog This is the name of the generic UI component, not the React module! priority: important This change can make a difference

Comments

@bogdansoare
Copy link

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

I know this has been discussed in #12126, but there is still an issue with the scrolling, the title and the actions aren't fixed and they scroll

Here is the codesandbox: https://codesandbox.io/s/qlo16r5v59

Here is the code

<Dialog open aria-labelledby="form-dialog-title">
  <form
    onSubmit={e => {
      alert("form submit!");
      e.preventDefault();
    }}
  >
    <DialogTitle id="form-dialog-title">Log in</DialogTitle>
    <DialogContent>
      <DialogContentText>
        Please enter your account number and password.
      </DialogContentText>
      <TextField
        autoFocus
        margin="dense"
        label="Account Number"
        type="text"
        fullWidth
      />
      <TextField
        margin="dense"
        label="Password"
        type="password"
        fullWidth
      />
      <div style={{ height: 1000 }} />
    </DialogContent>
    <DialogActions>
      <Button onClick={() => alert("cancel")} color="primary">
        Cancel
      </Button>
      <Button
        type="submit"
        onClick={() => alert("login")}
        color="primary"
        variant="contained"
      >
        Log in
      </Button>
    </DialogActions>
  </form>
</Dialog>
@oliviertassinari oliviertassinari added the support: Stack Overflow Please ask the community on Stack Overflow label Oct 15, 2018
@support support bot closed this as completed Oct 15, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 15, 2018

Should this demo https://material-ui.com/demos/dialogs/#confirmation-dialogs uses a form?

@bogdansoare

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@bogdansoare

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@bogdansoare

This comment has been minimized.

@Ethorsen
Copy link

Ethorsen commented Oct 23, 2018

+1 We face the same issue. I will implement that ugly fix for now.

But it would be nice to fix MUI at the source and allow <Dialog*> elements to be wrapped in a form without affecting layout/scrolling functionality.

@liitfr

This comment has been minimized.

@liitfr

This comment has been minimized.

@jlil

This comment has been minimized.

@jlil

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@hayatae

This comment has been minimized.

@mui mui deleted a comment from hayatae Jul 17, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 17, 2019

@hayatae Interesting, you need to apply this CSS

      <Dialog>
        <form
          onSubmit={(event) => {
            alert('form submit!');
            event.preventDefault();
          }}
          style={{
            overflowY: 'auto',
            display: 'flex',
            flexDirection: 'column',
          }}
        >
          <DialogTitle

on the <form> element if you want to the dialog content to scroll instead of the dialog paper.

Alternatively, you can make the paper to host your form:

      <Dialog
        PaperProps={{
          component: 'form',
          onSubmit: (event) => {
            alert('form submit!');
            event.preventDefault();
          }
        }}
      >
        <DialogTitle

For modern browser, we can use display: contents: #13253 (comment)

@oliviertassinari oliviertassinari added component: dialog This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement and removed support: Stack Overflow Please ask the community on Stack Overflow labels Jul 17, 2019
@support support bot reopened this Jul 17, 2019
@mui mui deleted a comment from support bot Jul 17, 2019
@oliviertassinari oliviertassinari changed the title Form inside dialog breaks scroll [Dialog] Form + scroll in DialogContent Jul 17, 2019
@seamuslowry

This comment has been minimized.

@seamuslowry

This comment has been minimized.

@seamuslowry

This comment has been minimized.

@seamuslowry

This comment has been minimized.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 25, 2020

I don't think that https://material-ui.com/components/dialogs/#form-dialogs is following the best practice, I believe it should have a <form> element. Fixed in #40470

@oliviertassinari oliviertassinari added priority: important This change can make a difference and removed support: question Community support but can be turned into an improvement labels Mar 25, 2020
@DASPRiD
Copy link
Contributor

DASPRiD commented Apr 16, 2020

@oliviertassinari I definitely like the second solution, as one doesn't have to repeat existing CSS. The problem though is that (in the TypeScript definitions) PaperProps does not support form specific attributes like noValidate. For now I solved that by putting a @ts-ignore above it, but I assume this is actually a bug?

@oliviertassinari
Copy link
Member

@DASPRiD noValidate should be supported. Looks like an issue.

@DASPRiD
Copy link
Contributor

DASPRiD commented Apr 16, 2020

@oliviertassinari I think the problem is here:

https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Paper/Paper.d.ts#L5

It's using React.HTMLAttributes instead of React.AllHTMLAttributes.

@DASPRiD

This comment has been minimized.

@Djspaceg
Copy link

I have a different solution to this problem, which requires a slightly newer browser, but doesn't inject styling overrides.

      <Dialog>
        <form
          onSubmit={e => {
            alert("form submit!");
            e.preventDefault();
          }}
          style={{
            display: "contents"
          }}
        >
          <DialogTitle
          ...

This allows the inner flex element of Dialog to still be the operator of the flex-box container rules, and allows the children DialogTitle, DialogContent, and DialogActions to retain their flex-box children rules, allowing for correct scrolling.

display: contents; effectively removes the <form> element from the DOM layout, in the eyes of CSS, so the parent and children can interact as if it wasn't even there. As stated before, this does require a slightly newer browser, but that's ok for my case.

Setting component="form" is, in my opinion, the next best option, even though this causes awkward Typescript issues regarding the event handler in my case.

It'd be great to have first-class support for a "FormDialog" component exported out of MaterialUI which did this component-override in a few lines: (with appropriate typescript mappings)

const FormDialog = (props) => <Dialog {...props} component="form" />;

Or at very least, an official example of the "right" way to do this, with proper Typescript definition, etc.

@michael-land
Copy link
Contributor

michael-land commented Dec 30, 2020

@Djspaceg Elegant solution. I can now remove all the hacks and do it once in theme level:

    MuiDialog: {
      styleOverrides: {
        paper: {
          '& > form': {
            display: 'contents',
          },
        },
      },
    },

@wescopeland
Copy link

wescopeland commented Aug 5, 2021

Beware: display: 'contents' has a major implementation bug with all versions of Safari causing its child content to be inaccessible by assistive technologies.

We are still seeing this issue in 5.0.0-beta.1.

This code does seem to fix it:

export const Form = styled("form")({
  overflowY: "auto",
  display: "flex",
  flexDirection: "column"
})

@trickreich
Copy link

image
overflow-y: auto on DialogContent is causing this bug... what do you guys do in such situations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dialog This is the name of the generic UI component, not the React module! priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests