When the "Breadcrumb" block is removed, there's no top padding for the main content

#4 patch applied successfully. I have attached before and after screenshots.

Adding top padding works fine on Appearance, Extend pages, but it also increases the space on sections where help text is shown after nav menu such as Block page and Appearance Settings.

This patch does not resolve spacing issue for Configuration page. For this, we need to add margin in the compact-link class under stable theme.

Your thoughts?

I didn't find such issue with seven theme with drupal version 8.3.x-dev.
Attached is the screnshot for the same.

Updated patch because #10 failed to apply on 8.3.x branch.

Good find, we should definately fix this. We first fix in the latest dev version and then consider backporting to 8.3, so moving to the 8.4.x-dev queue.

I'm looking at this today.

I would like to expand on this a bit. Patch #14 works as intended, but I have found more admin pages where there is a top margin missing.

See screenshot.

I wonder if a better way to approach this is to add the top margin to either:

<div class="region region-content">

or <div id="block-seven-content" class="block block-system block-system-main-block">, as that might cover all instances.

I will look into this, and see if it may have negative knock-on effects.

Opinions welcomed.

I think the best approach here is to remove the two additional rules in patch #14 and add a top margin to the .region-content div.

The rules in patch #14 seem no longer relevant as I believe a rule has been added or amended in elements.css to add margins to all paragraph elements.

See screenshot

Looks like your proposal is the better approach. I think a patch would be a good next step :)

To summarise, I removed both rules added by patch #14, as explained above.
I added a new rule to layout.css, adding 10px margin top to .region-content

Patch and inter-diff reflect this.

Unassigning myself and updating Issue summary

Looks like this place to add margins does not take messages and help texts into account?

Indeed, there now is no space between the Help text block and the Page title, if the page doesn't have Primary or Secondary tabs.

Check for example admin/structure/taxonomy:

Also on admin/structure/display-modes, the table of submenu items is squash against the page header:

Status update:
I wasn't specific enough in my comments above. I actually deleted a file as part of my solution and did not include it in my patch.
New patch uploaded, for review again.

Thanks for reviewing :)

Added new line at end of file

Removed reference to help.css file from libraries.yml as reflected in new patch and inter-diff.

Ran test locally this time.

I applied patch #34 works fine for me. Adding before after screenshots.

I have captured some screenshots, i need some suggestions whether this is desired results or not.

Applied patch in #34 locally and everything looks good to me. Setting this to RTBC.

This does change the styles on some pages which don't suffer from this exact bug. Would be great to get sign-off from the UX maintainer and a bit more manual testing to see how widespread this change is.

Before:

After:

just removing Needs Manual Testing tag as it is not relevant at the moment.

This seems to continue to look consistent, I am not too worried about the additional padding - when breadcrumbs are there.

I had thoroughly checked the site with breadcrumbs and without breadcrumbs and i found there is no big difference with breadcrumbs padding if breadcrumbs are there. I captured screenshots for every page before and after applying the patch including breadcrumbs and without breadcrumbs.
PFA the screenshots for more clarity. Hope it helps.

Hidden a few files so the actual patch and interdiff are easier to find for committers.

Screenshots all look good (and look like a lot of work, too!) - We've had the usability review and the patch still applies. Setting to RTBC...

Wow, thanks for all the manual testing!

It does look like Seven has an assumption that the breadcrumbs are displayed. It's also good that this even allows us to clean up some unneeded CSS by putting it in the spot where it belongs. Excellent. Since this is a change to Seven, which is an internal theme, it's allowed to change it in minor releases, and this is a clear visual improvement.

The screenshots in #43 confirm that the case where the breadcrumbs block is or isn't present works consistently across several pages. Thanks also for cropping the screenshots to just the important part; that's definitely helpful.

I think what we need to have screenshots of is whether different combinations of the blocks that are usually at the top of the page, e.g. with help and without breadcrumbs, with help and with breadcrumbs, etc.. We should also try placing the blocks we're removing the padding from on different parts of the page. They're not necessarily at the top, so we could be sort of making the same mistake the original code does by assuming a certain block placement. We don't need to test these combinations on as many pages as in #40 -- just testing a few different things once each should be good.

Thanks also @rachel_norfolk for fixing the file attachments; it would have been hard to find the patch otherwise. :)

Just for my own understanding, can someone explain to me why we remove

-.compact-link {
-  margin: 0 0 0.5em 0;
-}
-

I'm trying to see the relevance if the main purpose is to add some margin above .region-content.

I have rerolled the patch for Drupal 8.5.x.

Applied the patch from #49 on 8.5.6, a similar result appears before and after applying the patch. Especially in the case of breadcrumbs block disabled, there is a very minimal spacing between tabs and helping text. It seems top padding for the content is missing.

Also, try to apply the same patch on 9.1.x but it's failing to apply.

Please refer to the screenshot attached.

Rerolling patch in #49 for 9.1.x-dev

That's kind of typical issue in Drupal design. To ensure vertical rhythm components (blocks, fields, form elements, etc) should never define external geometry, i.e. margins. That's a job for container elements.

Verified and tested by applying the patch #58 for 9.1.x-dev.It was applied successfully.

Verified and tested by applying the patch #58 for 9.1.x-dev.It was applied successfully.

Tested the patch given in #58, It seems top padding for main content is still missing after applying the patch in case of without breadcrumbs.

Please refer to SS attached for ref.

I took a different approach than in #34. Here is a patch for the same.

@himanshu_sindhwani thanks for the patch. Patch applied and changes are reflecting as expected.

Top padding for main content is now appearing after applying the patch with breadcrumbs disabled. Also, attached the SS how it looks with changing the order of help text and breadcrumbs.

Verified and tested patch #66. Looks good to me.
Refer #69 screenshots for the reference.

Test fails randomly. So setting it to needs review.

@himanshu_sindhwani I have tested your #66 patch and your patch will resolve the given issue.

Need minor updates in this patch.

1. can you use only the first-child of p to remove the margin-top value?
.help p:first-child

2. can you update .region.region-breadcrumb class with .region-breadcrumb (to reduce selector specificity).

Like .region-breadcrumb + .page-content .help p:first-child

Thanks!

@pankaj.singh your patch updated with changes suggested by @sd9121 seems working fine,

Verified and tested patch #76. Looks good to me.

Fixing test case review the patch.

Tested the latest patch #80 on 9.1, looks good to me
This can be moved to RTBC

I have applied patch #80 on 9.2 working fine sharing screenshot...

Patch #80 applied cleanly and working as expected , there's top padding for the main content. Screenshot for reference.

Verified and tested patch #80.
Patch applied successfully and looks good to me.

Testing Steps:
# Apply Seven theme
# Goto: /admin/appearance
# Check the necessary results.

Expected Results:
# User should see some top padding when there are no breadcrumbs.

Actual Results:
# When the User removes the "Breadcrumb" block in Seven Theme, there's no top padding for the main content

Looks good to me.
Can be a move to RTBC.

Still, there is no spacing if we remove the breadcrumb. Adding after patch screenshot for reference.
Moving to NW

@Gauravmahlawat

I have Verified and tested patch #80. Patch applied successfully and looks good to me.

Attached screenshot here. please verify it.

Move to RTBC

has a margin of 1em from elements.css which is different from the .help styles. We should probably still override the margin to set it to 10px to match the old behavior.

Hi @lauriii here I have kept the 10px(old behaviour) of .help class. Please review the merge request !940.

Thank you for the updated patch.

@tanubansal @chetanbharambe @Sakthivel M - Please note that if you manually test and review the code and truly think the issue should be RTBC because it passes all the "core gates", then you should change the status to RTBC yourself during the comment rather than write something like " Can be a move to RTBC". This helps move the issue along faster.

I have reviewed the code and most recent comments as well as manually tested by 1) removing everything from the breadcrumb region and 2) putting something other than the breadcrumb in the region.

Marking RTBC based on:

1. Issue summary, title, and tags seem okay.

2. Code is clear and addresses only the issue in the issue summary.

3. Most recent feedback in #90 appears to be addressed. Note that I'm not a frontend developer anymore but this seems clear to me.

4. Assumes no manual tests need to be added.

5. Patch fixes the issue in the issue summary. See screenshots below.

Before patch:

After patch (with breadcrumb)

After patch (without breadcrumb)

Left a comment on the MR

Tried the patch #80 (2886816-80.patch) with the 9.5.x-dev version. working fine looks good for RTBC.
screenshot attached with patch and without patch.
2886816-before-patch.png
2886816-before-patch.png
2886816-after-patch.png
2886816-after-patch-2.png

The Seven theme has been removed from Drupal 10 core. I confirmed that this issue only affects Seven and no other themes included with Drupal core, so I am moving this to the contributed Seven project.

Patch #80 tested and applied successfully for drupal 9 version.
Screenshots attached for reference.

I am working on it.

@avpaderno, Please review the changes in the MR