Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

CSS; height as inherit leads to layout issues (Chrome / IE11) #4397

Closed
Londovir opened this issue Sep 1, 2015 · 7 comments
Closed

CSS; height as inherit leads to layout issues (Chrome / IE11) #4397

Londovir opened this issue Sep 1, 2015 · 7 comments
Assignees
@topherfangio
Labels
needs: feedback The issue creator or community need to respond to questions in this issue needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: team discussion This issue requires a decision from the team before moving forward.
Milestone
Deprecated

Comments

@Londovir
Copy link

Londovir commented Sep 1, 2015

Ever since the recent commit of code a few days ago that caused height : inherit to be bound to the .layout class, I've been struggling with layout issues. For instance, the following simple layout causes unnecessary scrolling in Chrome and IE11:

<body>
    <md-toolbar>
    </md-toolbar>
    <div layout="row">
    </div>
</body>

See this CodePen: Example

If I use F12 tools, and disable the height : inherit, all returns to normal as expected.

I'm not sure if this is as expected/designed and I'm just not designing properly, or if this is an unintended consequence. For instance, I understand in the example above that the div will inherit the height of the body, which the MD CSS gives a 100% height and min-height. So, in this case, this causes an unnecessary scrolling since the height of the md-toolbar is not being subtracted from the page height, and the div therefore runs longer than the viewport by that much. I can use more involved flex layouts and designs to account for this, but that leads to other issues.

One such issue is when I use md-icon. I've seen cases where an md-icon that is nested into some combinations of layout="row" or layout="column" loses the height of 24px set in the MD CSS because the .layout inherit style overrides it.
@ghost
Copy link

ghost commented Sep 1, 2015

+1 broke my layouts. My navbar is now 50% of the page

@ThomasBurleson ThomasBurleson added this to the 0.11.0 milestone Sep 1, 2015
@icchio
Copy link

icchio commented Sep 1, 2015

+1

@topherfangio
Copy link
Contributor

@Londovir If you add layout="column" to the <body> tag, it should work as expected. Can you create a codepen of the icon issue so that we can take a look?

@robertbaker @icchio Can you guys add some demos of your specific cases so that we can see the best way to fix it?

The height: inherit was added because a lot of other people were having issues getting the layouts to work properly, so I would love to find a solution that makes everyone happy rather than reverting the change.

@Londovir
Copy link
Author

Londovir commented Sep 1, 2015

@topherfangio I've updated my CodePen from the original link to show the icon issue. You can disregard the md-icon issue as it is a non-issue because there's no reason to have a layout="column" on the md-icon itself. It's more of an academic point in that the height : inherit is enough to override the md-icon CSS, but that's about it. Removing the layout on the md-icon is enough to solve the visual issue in IE11.

However, I've shown another layout issue this change has made. If you look at my CodePen and examine the right panel, there is a red line of text which should be just beneath the md-select on the page. It's not getting there because the height of the div containing that md-select is being inherited, causing it to grow overly tall. The main div is layout="column", which should lay out the children vertically downward, so by wrapping the text, md-select, and md-button into a row layout, it should lay those out on a row, and the next div I would have thought would come next below that. In fact, if I get rid of the inherited height using F12, then it tidies up as I would have expected.

So, in this case, trying to use layout="column" on the right side has led to all of my problems, and now I'm forced to re-examine layout which was fine before I updated the CSS to the newest MD. I would have suggested that if this change was needed because flex wasn't working correctly, that either an opt-in attribute or class was used instead of making a global CSS change, or some better guidance is offered in documentation on how to handle all of these side effects. As it stands, I now have to go and update every use of layout in my projects because of the height inheritance change, which means the easier choice for me at the moment is to simply comment out the height: inherit line completely. Or, barring that, to skip using the layout attributes and go with hand-coded flex instead.

@topherfangio
Copy link
Contributor

@Londovir The main issue we were attempting to solve is #4022. In this case, devs were having the opposite problem in that they had to add height hacks to every ancestor of a nested component to get the column layout working properly.

If you change it from .layout: inherit to .layout-column: inherit, does that help fix your layouts? I don't think we actually need it for the row layouts, so it may make some changes.

@topherfangio
Copy link
Contributor

Just talked with the team, we are trying to push 0.11 out this morning, so we're going to go ahead and revert this change since it is causing more issues than expected and we will reach out to users to try to find a better fix. Revert should be pushed momentarily.

@Londovir
Copy link
Author

Londovir commented Sep 1, 2015

Understood & appreciated. FWIW, it seems like the .layout-column: inherit change does appear to help clean up some/most of the situation on my end. I still need to revisit a number of pages in my projects before I signal complete victory at the moment. Thanks!

topherfangio added a commit that referenced this issue Sep 1, 2015
@topherfangio
revert(layout): Remove height: initial from layout.
44a7e52 
Originally, it seemed that adding the `height: initial` made no
significant change to the layout system except to fix #4022,
however, multiple users have experienced major issues with
their layouts: #4397.

We are reverting so that we can take some more time to work
with the community to find a better fix.

Reverts commit 303ab0d
@topherfangio topherfangio modified the milestones: 0.12.0, 0.11.0 Sep 1, 2015
@topherfangio topherfangio added needs: team discussion This issue requires a decision from the team before moving forward. needs: feedback The issue creator or community need to respond to questions in this issue needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community labels Sep 1, 2015
@topherfangio topherfangio mentioned this issue Sep 1, 2015
Closed
kennethcachia pushed a commit to kennethcachia/material that referenced this issue Sep 23, 2015
@topherfangio @kennethcachia
revert(layout): Remove height: initial from layout.
5626012 
Originally, it seemed that adding the `height: initial` made no
significant change to the layout system except to fix angular#4022,
however, multiple users have experienced major issues with
their layouts: angular#4397.

We are reverting so that we can take some more time to work
with the community to find a better fix.

Reverts commit 303ab0d
@ThomasBurleson ThomasBurleson modified the milestones: 1.0-rc1, 1.0-rc2, 1.0-rc3 Oct 27, 2015
@topherfangio topherfangio modified the milestones: 1.0-rc6, 1.0-rc3 Nov 3, 2015
@topherfangio topherfangio modified the milestones: 1.0-rc6, 1.0-rc8 Nov 23, 2015
@topherfangio topherfangio removed this from the 1.0-rc6 milestone Nov 23, 2015
@ThomasBurleson ThomasBurleson modified the milestones: 1.0-rc8, 1.1.0, Backlog Jan 5, 2016
@ThomasBurleson ThomasBurleson modified the milestones: Backlog, Deprecated Apr 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@topherfangio topherfangio

Labels
needs: feedback The issue creator or community need to respond to questions in this issue needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: team discussion This issue requires a decision from the team before moving forward.
Projects
None yet
Milestone
Deprecated
Development

No branches or pull requests
4 participants
@topherfangio @ThomasBurleson @icchio @Londovir