angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.37k stars 6.75k forks source link

MdDialog not resizing height to contents on open #2351

Closed jt-helsinki closed 7 years ago

jt-helsinki commented 7 years ago

Bug, feature request, or proposal:

Bug

What is the expected behavior?

trying to create an MdDialog using the Material 2 library. Expect the dialog to automatically size to the contents of the dialog.

What is the current behavior?

It's all working perfectly except for the height which comes out at 100% of the browser window height.

What are the steps to reproduce?

If I set the height using a MdDialogConfig object then the height works properly. However this is not much good as if the form layout changes, the height will not adjust automatically thus running the risk of form elements being cut off.

export class MainHeader implements OnInit {
    constructor(private dialog: MdDialog,
                private viewContainerRef: ViewContainerRef) {

    }

    ngOnInit() {
        this.config = new MdDialogConfig();
        this.config.viewContainerRef = this.viewContainerRef;
    }

    openLoginModal() {
        this.dialogRef = this.dialog.open(LoginModal, this.config);

        this.dialogRef.afterClosed().subscribe((result: string) => {
            // do something 
            this.dialogRef = null;
        });
    }

}

If I don't pass in the config, same result.

What is the use-case or motivation for changing an existing behavior?

Using forms inside dialog.

Which versions of Angular, Material, OS, browsers are affected?

Angular 4.0.0-beta.1 Material 2.0.0-alpha.11.3

Is there anything else we should know?

I thought this might be a stylesheet problem so I commented out the styles and the problem persisted.

To double check, I added a border to a <div> which surrounds the form. The form is the correct size as expected.

crisbeto commented 7 years ago

I'm not exactly sure what you're trying to achieve. What does your dialog look like and how do you expect it to look? Currently the dialog resizes, but horizontally. If you set a max-height on your component, it should start resizing vertically.

jt-helsinki commented 7 years ago

For example, the application has a modal dialog with some md-inputs, md-selects (etc) and buttons. There are only a few components stacked on top of each other which amount to 350-400 pixels in height. Despite this, the modal opens at 100% of the height of the browser window.

There are also multiple other data entry screens which are contained inside other dialogs. The set up/tear down of each dialog follows the basics listed above. They also open at 100% height.

What I am expecting is for the modal to open at a similar height as the total height of the stacked components (accounting for padding etc) contained inside the dialog window.

Assuming each component is 25px in height and I have 4 components I would see a height of 100px plus any padding around the components.

crisbeto commented 7 years ago

Could you post an example on Plunkr? You can use this one as a base http://plnkr.co/edit/o077B6uEiiIgkC0S06dd

PhilippeBoisney commented 7 years ago

Same problem here ! =/ Quick fix is to set MDialogConfig

let config = new MdDialogConfig();
config.height='150'
jt-helsinki commented 7 years ago

Thanks @PhilippeBoisney. I'm glad it's not just me. For simple cases this works but when the dialogs are responsive this may not work.

@crisbeto I've tried to recreate the problem inside a plunk but I can't reproduce it - The modal is correctly right sizing upon open. (http://embed.plnkr.co/o077B6uEiiIgkC0S06dd/) However having said this, the application I am working on is not a plunk. It's far more invovled with many more interactions.

Given that I've removed the styles and tried this, I do think that it is a md2 problem. Moreover, it appears that the size of the dialog is related to the screen height. When commenting out the stylesheets the result is not pretty but one thing that remains is that the dialog is 100% height.

crisbeto commented 7 years ago

I understand, but it's a bit hard to know what you're fixing when there isn't something visual to work with.

jt-helsinki commented 7 years ago

This is what it looks like. Does this help?

screen shot 2016-12-22 at 20 34 30

And with a div with a border wrapping the form. screen shot 2016-12-22 at 20 39 04

crisbeto commented 7 years ago

It does, thanks. I'll look into it.

crisbeto commented 7 years ago

I still can't reproduce it just by using the dialog as usual. The only ways I can get a similar result are:

  1. If I set the height in the config to 100% (you mentioned that it's not what you're doing).
  2. If I manually remove the align-items: center; from the overlay wrapper.

My guess is that it has something to do with number 2. Can you show me a screenshot of what the DOM looks like when you have an open dialog? To jump to the proper element, you can paste this into the console:

inspect(document.querySelector('.cdk-global-overlay-wrapper, .md-global-overlay-wrapper'))
PhilippeBoisney commented 7 years ago

For me it looks like that :

<div class="md-global-overlay-wrapper" style="justify-content: center; align-items: center;">
  <div id="md-overlay-0" class="md-overlay-pane" dir="ltr" style="position: static;">
    <md-dialog-container class="md-dialog-container" role="dialog">
      <focus-trap><!--template bindings={
  "ng-reflect-ng-if": "true"
}--><div tabindex="0"></div> 
        <div> <!--template bindings={
  "ng-reflect-portal": null
}--><loading-dialog>
       <p>Simple test</p>

    </loading-dialog> 
        </div> <!--template bindings={
  "ng-reflect-ng-if": "true"
}--><div tabindex="0"></div> 
      </focus-trap> 
    </md-dialog-container>
  </div>
</div>

and the screen output :

capture d ecran 2016-12-24 a 09 35 12
crisbeto commented 7 years ago

This looks right. What browser are you using and what version of Material? Also while we're at it, it may be worth trying to delete the node_modules/@angular and running an npm install. We had some weird styling issues after we released one of the alpha versions which got fixed by doing a clean install.

PhilippeBoisney commented 7 years ago

I'm using Chrome of course ;) => Version 55.0.2883.95 and @angular/material@2.0.0-beta.1 I've done a clean install too but don't seems to work... =/

PhilippeBoisney commented 7 years ago

Ok it's very weird... I reinstall all my npm modules, but with this config: https://github.com/jelbourn/material2-app/blob/master/package.json

And it's works... So I will try to find out which one package cause troubles with dialog.

PhilippeBoisney commented 7 years ago

With all angular's modules updated (2.4.1), here is what I got:

capture d ecran 2016-12-24 a 10 42 18

And with angular's modules with this version 2.0.0 and other modules from this config:

capture d ecran 2016-12-24 a 10 58 19
jt-helsinki commented 7 years ago

Hi, sorry to chime in so late. I'm also using Chrome (v 54.0.2840.98).

Here is my DOM for the Dialog plus a bit more.

<div class="md-overlay-container">
    <div id="md-overlay-2" class="md-overlay-pane" dir="ltr" style="left: 611.672px; top: 179px;"></div>
    <div class="md-global-overlay-wrapper" style="justify-content: center; align-items: center;">
        <div id="md-overlay-3" class="md-overlay-pane" dir="ltr" style="position: static;">
            <md-dialog-container class="md-dialog-container" role="dialog">
                <focus-trap><!--template bindings={
  "ng-reflect-ng-if": "true"
}-->
                    <div tabindex="0"></div>
                    <div> <!--template bindings={
  "ng-reflect-portal": null
}-->
                        <standard-category-add-edit-modal>
                            <form autocomplete="off" novalidate="" role="form"
                                  class="ng-untouched ng-pristine ng-valid">
                                some text, form controls and other bits and pieces.
                            </form>
                        </standard-category-add-edit-modal>
                    </div> <!--template bindings={
  "ng-reflect-ng-if": "true"
}-->
                    <div tabindex="0"></div>
                </focus-trap>
            </md-dialog-container>
        </div>
    </div>
    <div class="md-overlay-backdrop md-overlay-dark-backdrop md-overlay-backdrop-showing"></div>
</div>

And the generated styles for the same. Could the height: 100%; be the culprit?

<style> md-dialog-container {
    box-shadow: 0px 11px 15px -7px rgba(0, 0, 0, 0.2), 0px 24px 38px 3px rgba(0, 0, 0, 0.14), 0px 9px 46px 8px rgba(0, 0, 0, 0.12);
    display: block;
    padding: 24px;
    border-radius: 2px;
    box-sizing: border-box;
    overflow: auto;
    width: 100%;
    height: 100%;
}

@media screen and (-ms-high-contrast: active) {
    md-dialog-container {
        outline: solid 1px;
    }
}  </style>

From my package.json

  "dependencies": {
    "@angular/common": "2.4.1",
    "@angular/compiler": "2.4.1",
    "@angular/core": "2.4.1",
    "@angular/forms": "2.4.1",
    "@angular/http": "2.4.1",
    "@angular/material": "^2.0.0-alpha.11",
    "@angular/platform-browser": "^2.4.1",
    "@angular/platform-browser-dynamic": "^2.4.1",
    "@angular/router": "^3.3.0",
    "animate.css": "3.1.1",
    "class-transformer": "^0.1.1",
    "core-js": "^2.4.1",
    "flexboxgrid-sass": "~6.2.0",
    "font-awesome": "^4.6.1",
    "hammerjs": "^2.0.8",
    "ie-shim": "^0.1.0",
    "reflect-metadata": "^0.1.8",
    "rxjs": "5.0.0-rc.4",
    "systemjs": "0.19.40",
    "zone.js": "^0.7.2"
  }

Keep in mind I have tried completely removing the styles by commenting them out. I've also tried deleting and reinstalling node_modules.

crisbeto commented 7 years ago

Sounds weird @PhilippeBoisney, I'm using Angular 2.4.1 when testing locally. One weird thing in the HTML from @paistipoikka is that there's an extra md-overlay-pane, although that should really get in the way.

jt-helsinki commented 7 years ago

@crisbeto sorry, you beat me to it. I just edited my previous comment to add the generate styles.

extra md-overlay-pane

uurrgghhh...there are a lot of dialogs and md-tabs. in this particular page. Having said this, even on pages that are more simple the problem is the same.

although that should really get in the way.

Do you mean "should not" get in the way? I hope so.

crisbeto commented 7 years ago

Alright, this shouldn't be necessary, but can you try adding this to your CSS:

.cdk-overlay-pane, .md-overlay-pane {
  height: auto !important;
}

I added the !important just in case. The overlay pane is what gives the dialog container it's height.

PhilippeBoisney commented 7 years ago

Here is my package.json that working (dialog height is normal) :

{
  "name": "myapp",
  "version": "0.0.0",
  "license": "MIT",
  "angular-cli": {},
  "scripts": {
    "start": "ng serve",
    "lint": "tslint \"src/**/*.ts\"",
    "test": "ng test",
    "pree2e": "webdriver-manager update",
    "e2e": "protractor"
  },
  "private": true,
 "dependencies": {
    "@angular/common": "^2.0.0",
    "@angular/compiler": "^2.0.0",
    "@angular/core": "^2.0.0",
    "@angular/forms": "^2.0.0",
    "@angular/http": "^2.0.0",
    "@angular/platform-browser": "^2.0.0",
    "@angular/platform-browser-dynamic": "^2.0.0",
    "@angular/material": "2.0.0-alpha.10",
    "@angular/router": "^3.0.0",
    "core-js": "^2.4.0",
    "rxjs": "5.0.0-beta.12",
    "ts-helpers": "^1.1.1",
    "zone.js": "^0.6.21"
  },
  "devDependencies": {
    "@angular/compiler-cli": "^2.0.0",
    "@angular/platform-server": "^2.0.0",
    "@types/hammerjs": "^2.0.32",
    "@types/jasmine": "^2.2.30",
    "angular-cli": "^1.0.0-beta.16",
    "codelyzer": "~0.0.26",
    "firebase-tools": "^3.0.7",
    "jasmine-core": "2.4.1",
    "jasmine-spec-reporter": "2.5.0",
    "karma": "0.13.22",
    "karma-chrome-launcher": "0.2.3",
    "karma-jasmine": "0.3.8",
    "karma-remap-istanbul": "^0.2.1",
    "protractor": "4.0.3",
    "ts-node": "1.2.1",
    "tslint": "3.13.0",
    "typescript": "^2.0.2"
  }
}

and the one that not working (dialog height seems to fit screen height)...

{
  "name": "myapp",
  "version": "0.0.0",
  "license": "MIT",
  "angular-cli": {},
  "scripts": {
    "start": "ng serve",
    "lint": "tslint \"src/**/*.ts\"",
    "test": "ng test",
    "pree2e": "webdriver-manager update",
    "e2e": "protractor"
  },
  "private": true,
 "dependencies": {
   "@angular/common": "2.4.1",
   "@angular/compiler": "2.4.1",
   "@angular/core": "2.4.1",
   "@angular/forms": "2.4.1",
   "@angular/http": "2.4.1",
   "@angular/material": "^2.0.0-beta.1",
   "@angular/platform-browser": "2.4.1",
   "@angular/platform-browser-dynamic": "2.4.1",
   "@angular/router": "3.4.1",
   "@types/hammerjs": "^2.0.33",
   "core-js": "^2.4.1",
   "hammerjs": "^2.0.8",
   "jquery": "^3.1.1",
   "rxjs": "5.0.2",
   "ts-helpers": "^1.1.1",
   "zone.js": "^0.7.4"
 },
 "devDependencies": {
   "@angular/compiler-cli": "2.4.1",
   "@types/jasmine": "2.5.38",
   "@types/jquery": "^2.0.34",
   "@types/node": "^6.0.42",
   "angular-cli": "1.0.0-beta.22-1",
   "codelyzer": "~2.0.0-beta.1",
   "jasmine-core": "2.5.2",
   "jasmine-spec-reporter": "2.7.0",
   "karma": "1.3.0",
   "karma-chrome-launcher": "^2.0.0",
   "karma-cli": "^1.0.1",
   "karma-jasmine": "^1.0.2",
   "karma-remap-istanbul": "^0.4.0",
   "protractor": "4.0.14",
   "ts-node": "1.7.2",
   "tslint": "^4.0.2",
   "typescript": "~2.0.10",
   "webdriver-manager": "11.1.0"
 }
}
jt-helsinki commented 7 years ago
.cdk-overlay-pane, .md-overlay-pane {
  height: auto !important;
}

@crisbeto I tried adding the this to both the application styles and the dialog component styles. Unfortunately it didn't work.

styles: [`.cdk-overlay-pane, .md-overlay-pane {
  height: auto !important;
}`]
jt-helsinki commented 7 years ago

@crisbeto did you have any more ideas about this?

crisbeto commented 7 years ago

Well, the alternative is to try this one out, although it would be a lot easier if there was a Plunkr that shows the issue:

.cdk-overlay-pane, .md-overlay-pane {
  flex: 1 1 0;
}
jt-helsinki commented 7 years ago

Because this is contained within a large application creating a plunk is not so straight forward.

It turns out that if I overwrite the default cdk-global-overlay-wrapper style with the following style then the dialog resizes properly.

.cdk-global-overlay-wrapper {
  height: auto !important;
}

However the dialog is still not centred in the middle of the screen. Does this offer any clues?

crisbeto commented 7 years ago

Yes, I understand, but there should be some minimal way of reproducing it. Regarding overriding the overlay wrapper's height, that's not really a fix since the wrapper is supposed to cover the entire viewport.

jt-helsinki commented 7 years ago

Regarding overriding the overlay wrapper's height, that's not really a fix since the wrapper is supposed to cover the entire viewport.

Using this fix still sees the wrapper covering the entire viewport as per normal. It's just the dialog that right sizes. If this is not meant to happen then maybe this is a clue.

I'll try again to recreate the problem.

jt-helsinki commented 7 years ago

@crisbeto

I finally got a chance to tear down the app down to something that is a bit simpler. This zip file demonstrates the Dialog problem. It also demonstrates problems with md-select heights displaying the incorrectly. Have a look and I can create a new issue for the md-select if it is in fact unrelated to the first problem with the dialog.

material problems.zip

note, you will see this project uses angular-flexlayout. With or without, the dialog and select problems remain.

in the assets-->styles main.scss you can uncomment the material styles and this will change the dialog to the correct height but not centred on the page.

Hopefully this helps.

crisbeto commented 7 years ago

Thanks @paistipoikka, I could reproduce it from that scenario. Looking into why it's happening.

jt-helsinki commented 7 years ago

Thanks @crisbeto

The surprising thing is that I can't reproduce it any other way. Whatever has happened seems to be interfering with a number of the styles such as those used with the <md-select>.

Cheers

crisbeto commented 7 years ago

It's really weird, also considering that I removed a lot of the code from your case, making it pretty much just the dialog toggle and the dialog itself and the issue still happens. You can work around it temporarily by setting the md-dialog-container { height: auto; } in your CSS.

jt-helsinki commented 7 years ago

md-dialog-container { height: auto; }

Did that work for you? Not for me. I used:

.cdk-global-overlay-wrapper {
  height: auto !important;
}

That worked with the exception that the dialog was aligned with the top of the window.

I'm really wondering if the md-select problem is part of the same issue. Do you want a seperate issue raised?

m0tter commented 7 years ago

+1 For the same problem I'm running beta.1 and angular 2.4.2

md-dialog-container { height: auto; } didn't work for me. .cdk-global-overlay-wrapper did

"dependencies": { "@angular/common": "2.4.2", "@angular/compiler": "2.4.2", "@angular/core": "2.4.2", "@angular/forms": "2.4.2", "@angular/http": "2.4.2", "@angular/material": "2.0.0-beta.1", "@angular/platform-browser": "2.4.2", "@angular/platform-browser-dynamic": "2.4.2", "@angular/router": "3.4.2", "core-js": "2.4.1", "reflect-metadata": "0.1.9", "rxjs": "5.0.3", "systemjs": "0.19.41", "zone.js": "0.7.4" }

m0tter commented 7 years ago

This custom css fixed the problem for me md-dialog-container { height: inherit !important;} and setting encapsulation: ViewEncapsulation.None

jt-helsinki commented 7 years ago

@m0tter yes, that worked. In fact, I removed the setting encapsulation: ViewEncapsulation.None and it worked as intended in my application. It seems md-dialog-container { height: inherit !important;} is enough to fix this as a workaround.

Does your application require the encapsulation setting?

m0tter commented 7 years ago

I think I found the problem. I was missing <!DOCTYPE html> from index.html

I can now remove the css hack and it works as intended. Can someone please test it.

crisbeto commented 7 years ago

Great find @m0tter! That's definitely the cause. I'll see if we can work around this, otherwise we can log a warning if you're missing the doctype.

jt-helsinki commented 7 years ago

@m0tter

Yes, that worked on this end too. Thanks so much for chasing that down. Fantastic!

centaure commented 7 years ago

Inserting <!DOCTYPE html> worked for me too. Thank you.

m0tter commented 7 years ago

Glad to hear that worked :)

VitalyVrublevskyy commented 7 years ago

@crisbeto Actually, I still reproduce it including all combinations of mentioned workarounds:

Lib version: "@angular/material": "^2.0.0-beta.1"

PS. Looks like problem hidden in <cdk-focus-trap> elem.

crisbeto commented 7 years ago

Try removing those CSS workarounds @VitalyVrublevskyy and only add the doctype.

VitalyVrublevskyy commented 7 years ago

Actually, I did. I had initialy doctype. Anyway, I can reproduce it on the up to date material2 demo app. image

Patch of related changes MDDialog_height_Issue.patch.zip

crisbeto commented 7 years ago

That looks to be working properly to me. You're setting the height to 500px which is what it's doing. If you want it to scale based on the content, just leave the height out.

VitalyVrublevskyy commented 7 years ago

Another axample: image

How about <md-dialog-actions>. It should be the very bottom of the dialog window. Does it make sense?

crisbeto commented 7 years ago

It makes sense, although it's unrelated to this issue. What you're talking about should be fixed with https://github.com/angular/material2/pull/2546

Edit: It should've been fixed, but I hadn't looked at that PR in a while and it's broken, at the moment. I'll clean it up.

VitalyVrublevskyy commented 7 years ago

@crisbeto Just a little question: Does it any option to show dialog vertically as it, without any scroll. I mean to show in full height and remove the scrolling in overlay, and apply it to the all window height? Now we have max-height: 80vh (including your last changes in https://github.com/angular/material2/pull/2546) or 65vh in past

crisbeto commented 7 years ago

@VitalyVrublevskyy it won't scroll if you don't use the <md-dialog-content> element. It'll stretch based on the content until it hits 80% of the viewport height.

VitalyVrublevskyy commented 7 years ago

@crisbeto However in your case we can't scroll using default window scroll. I just little extend your demo-jazz-dialog example. Could you please take a look on https://material2-demo-314e9.firebaseapp.com/dialog ? Git Patch attached to the comment. High_dialogue_without_scroll.patch.zip

crisbeto commented 7 years ago

Ah, sorry. What I mentioned should work once I finish reworking the positioning logic. You can get your example to work by adding a max-height to .mat-dialog-container.

TalosProgramming commented 6 years ago

Ok sorry if this sounds silly, but I just lost 20 mins on this problem, so I write this just so no one ever feels that dumb ever again :

Try closing the console, it affects the dialog ^^

angular-automatic-lock-bot[bot] commented 5 years ago

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.