Follow-up use better technique for vertical centering

This is so much better. Let's keep the reusable class. Moving to Seven as that's where the CSS is.

Looks good to me. Once we get green this is RTBC from me. Unfortunately we need to keep body.install-page in some cases because we need to override stronger selectors. See the comment at the top of the install-page.css

All we need is a review or screenshot on IE9

Great stuff

+++ b/core/modules/system/templates/install-page.html.twigundefined
@@ -20,59 +20,55 @@
+      <div id="content" class="column">
+          {% if title %}<h1 class="title" id="page-title">{{ title }}</h1>{% endif %}
+          {% if messages %}{{ messages }}{% endif %}
+          <div id="content-content" class="clearfix">
+            {{ content }}
+      </div> <!-- /content -->

We should be fixing the spacing here...

The final close div here is closing content-content not content

+++ b/core/modules/system/templates/install-page.html.twigundefined
@@ -20,59 +20,55 @@
-        </div> <!-- /container -->
+    </div> <!-- /container -->

This seems to be closing content not container

... and also we are missing a closing div here...

+++ b/core/modules/system/templates/install-page.html.twigundefined
@@ -20,59 +20,55 @@
+      <div id="content" class="column">
+          {% if title %}<h1 class="title" id="page-title">{{ title }}</h1>{% endif %}
+          {% if messages %}{{ messages }}{% endif %}
+          <div id="content-content" class="clearfix">
+            {{ content }}
+      </div> <!-- /content -->

After fixing the spacing, it looks to me like we're missing a closing div on #content-content. That also fixes the (perceived) missing div at the end.

That patch was missing the CSS file updates. New patch attached.

+++ b/core/themes/seven/install-page.cssundefined
@@ -29,23 +29,25 @@ body.install-page {
+    margin-right: -.3em;

This property seems slightly off, when I compare before/after side by side the after seems to be shifted slightly to the right.

Great! Thanks.

+++ b/core/themes/seven/install-page.cssundefined
@@ -29,23 +29,25 @@ body.install-page {
+    margin-right: -.35em;

-0.35em, might as well get it right the first time.

Also, can you post a -do-not-test patch rolled with git diff -w so the template file changes are clearer?

Thanks, that makes it very very clear what is happening here, and shows what an improvement it is!

Just reupload #13 but with -0.35m instead of -.35em and it should be RTBC.

I'm stealing this patch back. Swoop.

Thanks!

I read this article today: http://html5hub.com/centering-all-the-directions/#i.bwg1ds4z3emh11

Is there a better technique we could use?

I read this article today: http://html5hub.com/centering-all-the-directions/#i.bwg1ds4z3emh11

Is there a better technique we could use?

I read this article today: http://html5hub.com/centering-all-the-directions/#i.bwg1ds4z3emh11

Is there a better technique we could use?

I read this article today: http://html5hub.com/centering-all-the-directions/#i.bwg1ds4z3emh11

Is there a better technique we could use?

#18: drupal-better_vertial_aligning_technique-2032895-18.patch queued for re-testing.

It makes it more readable (it's not -35em), and it's in our coding standards.

Let's try this again from a fresh clone.

The code looks good, the patch applies.

Committed and pushed to 8.x. Thanks!

We noticed a bug with that appears when the page container is longer than the viewport. We attempted to fix it as part of #2094445: Fix installer right padding - make it look balanced but it was more complex than it first appears... I think that the vertical centering introduced here is not capable of accommodating both situations.

Let's give it another try otherwise I think we should revert this commit.

I have the LastPass browser plugin installed and it displays a message at the top which causes a beautifully blue page ;) Aka, the actual content is moved down below what I can see. Was asked to put a screenshot in here.

If I understand correctly, we just want some blue space above and below `#page` when `#page` takes up all or more than the available vertical area. We can accomplish this by adding top padding to the `body` (1em), top margin to `#page` (1em) and relatively positioning `#page` to -1em. Patch attached (I also alphabetized the rules within the `body.install-page #page` selector).

I have the same exact issue as Berdir in #35 (also caused by lastpass) and the patch in #36 does not fix it for me.

It's because lastpass adds a div like this directly after the opening body tag:

<div id="lptopspacer58606835" style="height: 27px;"></div>

Setting display:none; on that fixes the issue, so that's definitely the culprit.

I'm not sure what to do here, plugins shouldn't be adding elements to the page that affects the flow of the DOM. That's asking for trouble. Is it possible to open an issue against the plugin?

This is no longer novice

#36: drupal-better_vertial_aligning_technique-2032895-36.patch queued for re-testing.

I applied the patch from #36 but that does not appear to solve the problem. My understanding is the goal of this issue is to vertically align the #page element, not just add a bit of space (1em) above it which is what #36 does.

From what I can tell, the issue is that the lastpass plugin (and likely other password plugins like it) inject a div as the first element inside the body tag. It ends up right in between the body:before element and the #page element, which causes the :before element to sit above the #page element instead of being in line with it. This is what causes the giant blue gap.

I changed the pseudo class to :after, which fixes the problem. I also removed the negative right margin and instead used font-size: 0px to combat the small horizontal #page shift that the pseudo element causes.

Tested in Chrome & Firefox.

This patch was created as a part of the code sprint at #dcATL

#42 works for me, although I don't use last pass. Tested against freshly pulled 8.x-dev on existing site and viewed in chrome on a chromebook.

It's the install page, i don't mind adding 3 lines of JS to center this thing if the content height is smaller than the screen.

Some feedback from a Design perspective: When I last installed 8.x I think I saw the outcome of the patch:
The box is always centered vertically in the window when it is not higher than the screen.
Is that right?

I have done site designs like this and turned away from if for reasons: It may make the top of the page jump around on different pages depending on their height.
This looks inconsistent and probably not what we want.
So the approach that apparently #36 uses would be more in line with what web pages normally do.

The submit button might be in a different place for every page, but I'm pretty sure it is easier to find if the top of the box and thus the header is always in the same place.

I think the isue started because the box did not have any space above it when it was higher than the screen and touched the top of the browser window. This looks wrong indeed.

If this would not strike up the problem of someone having to do it, it might be up for some quick and dirty user testing. Away with vertical centering, it is so 2000, when webpages lived in little boxes that were smaller than the screen.

I'm a Lastpass user for more than 2 years and I've never seen the Lastpass bar to mess up a page like this.
This is the HTML introduced by Lastpass. It's just creating a empty space that is actually covered by an iframe that is position:absolute'd

<div id="lptopspacer33521325" style="height: 27px;"></div>

The patch #42 by Kendall works perfectly (as you can see in the screenshot). I tested in Chrome.

The only problem...

+++ b/core/themes/seven/install-page.css
@@ -50,6 +50,10 @@ body.install-page {
+    margin-top: 1em;
+    max-width: 770px;
+    top: -1em;
+    vertical-align: middle;

The vertical-align and max-width properties are duplicated, so the duplicated ones are removed in this patch:

The latest patch does not appear to fix the problem, we are still missing some space at the top when the content is taller than the screen.

Thanks Yannick for the fixes and the CSS guidelines links. I didn't know about CSSComb :)
About 2: True, now is really centered. The margin-right was needed.
About 3 . I will prefer that there is smaller top margin

margin-top: 2em;
top: -1em;

makes the same effect. And the less scrolling the better IMHO, but it's good.

I tested throughly the patch in different browsers and only 2 things to say:

* The margin-right works in Webkit browsers, but not Firefox or IE. I've tried different solutions. The easiest: Apply a "width:1px" and it will work.

* This is very very minor thing. But the negative "top" creates a taller viewport somehow, that makes the scrollbar appears when still there is no need (compared to same page of the installer without the patch). That's why also, the smaller the margin, smaller side-effect.

Reroll of #48, addressing these 2 issues and screenshot attached

Is this solution really better than the display: table-cell; it replaced? This CSS seems extremely fragile and tricky to understand.

I agree is getting hairy. What about this?
Here is a patch only touching CSS, no markup, no pseudoselectors and no negative values.
It works in all browsers equally where I tested, even IE9. No conflicts with LastPass bar and neither with Simplytest.me bottom bar.

Vertically aligned with
display:table-cell
The classic for horizontal alignment:
margin: 0 auto;

I applied CSSComb to the selectors I've touched. I removed all the text-align, white-space, etc that was introduced originally in https://github.com/drupal/drupal/commit/cb829e19065ebfe120d36db84c5a64e7...

The width property was already in the CSS. Let's just stick to fixing the problem in the title. We need some width set.

Yep this looks good, RTBC++

Committed 52de922 and pushed to 8.x. Thanks!