Button spacing CSS is too fragile

simple improvement

Could we maybe get some updated screenshots to show the "after"? I tested this locally but couldn't really tell a difference. (Not sure if it's caching related or what.)

I have tested patch locally and attach screenshots before and after applying patch.

Its look like button is overlap with text. Can we make them separate so that it looks attractive ?

Please review patch, Adding margin top and bottom on button.

Lets not just randomly add margin top and margin bottom. These button classes are used all over the place. Often with different margins on top depending on what is around the element.

refer my patch in comment 10

Drupal has already written css commonly for all buttons. Its not good idea to alter them which might cause other buttons. The problem is only when it comes inside details-wrapper. So i have added extra css for details-wrapper buttons.

I can confirm that last patch (#10) is best solution. It was tested and is working as intended. Using this class does not appear to produce any bugs.

This looks like it needs more review to me, there's a few different options available.

10: button-spacing-css-is-too-fragile-2160621-10.patch queued for re-testing.

Ok the patch in #10 removes horizontal spacing from buttons in details-wrapper.

This is ok but then the buttons inside a details-wrapper and inside an inline-container also have no more spacing when floating against an inline element. ( very dirty to fix )

So instead of removing margin like in #10, I would suggest to add margin to every button inside a container where its elements are inline (container-inline & form-actions) and when any kind of element comes before the button. ( similar fix as in #2 )

As far as I could see, these are the pages where this issue is most relevant:
/admin/config/search/settings
/admin/structure/block/manage/bartik_content

So in my patch i only give horizontal padding to buttons inside "container-inline" and "form-actions", everywhere else buttons don't need extra horizontal margin.

This is also tested on mobile.

See screenshots for fixed area's

@MathieuSpil Thanks, those annotations on the screenshots are really helpful.

Please do something similar to the ones in the issue summary so we can see what the problem was.
(So we can compare before-the-patch and after-the-patch issue screenshots.)

17: css-button-spacing-2160621-17.patch queued for re-testing.

Recreated the patch + updated summary.

Make one more small modification (buttons no longer have margin to the right)

It does get affected, because on some admin pages (see below) other elements besides buttons will receive the floating property, what will result in the button floating against the other element (mostly a dropdown as far as I can see)
- admin/content/comment
- admin/config/search/pages

But I am starting to doubt is .image-button. Because I am not sure where this is beïng used, Overall this seems me the most robust solution because of low css specificity.

Follow-up on last comment:
- admin/config/regional/translate is a edge case where it should keep on working.

Recreated the patch and replaced:

.container-inline * + .button,
.container-inline * + .image-button,

with:

.form-item + .button,
.form-item + .image-button,

Because the margin-left should only apply when the button is floating, and for this may be the use case inside .container-inline, but it is sometimes also the use case inside a table (admin/config/media/image-styles/manage/large)

So in conclusion: the button is always floated next to a form-item, (like on image-style edit page) OR floated next to another element inside a form-actions container.

Only problem I see here is that the specificity of the margin-left of buttons inside .form-actions is slightly higher than the margin-left of the buttons that are just normally next to .form-item

Thanks @MaieuSpil for contribution.
I have tested the latest patch css-button-spacing-2160621-31.patch. I have checked buttons on other pages also like blocks, content, modules. It is working fine for me. Attaching screenshots before and after applying the patch.

Hi LewisNyman, Please find the attached screenshots for other pages.

Tested latest patch, working fine for me.

+++ b/core/modules/system/css/system.theme.css
@@ -103,15 +103,11 @@ abbr.ajax-changed {
   margin-left: 1em;
-  margin-right: 1em;

Now that we only have a margin-left, this needs RTL styling too.

@LewisNyman @vidushi Find one issue on import file page admin/config/development/configuration/full/import, Upload button has margin-left: 1em;. Please find attached screenshot. Adding patch for the same. Hope it will work.

Thanks @herom for updating, Adding style for RTL also. Please review it.

Tried the last patch, but it didn't work on Chromium 39.0.2171.65 .

i had tested this patch on my local machine

@LewisNyman Could you please suggest any other method to solve this problem ?
@Vally79 @Sumit kumar Can you guys clear cache and retest it.

@lewisNyman Actually i was testing with #27 and #31.

Looks like your patch #2 is resolving the problem. I have just checked your patch, Working fine from myside.

Others Please review #2 patch, and Give you inputs.

@LewisNyman i have tested your patch #2 on my local. I have checked all the links in the site where buttons are placed. I haven't found any button related issue.

But i have some query related to delete button. Delete button should look like a button but its looking like a link. I am adding a screenshot of that.

It should look like a link?

#37 is resolving the button spacing issue.

@Vidushi Please check this Link for D8 UI reference.

Has somebody reviewed the #2 patch. Guys do review this patch and give your feedback.

I have also tested the patch #2 on my local machine and i have checked all the buttons in the site and its also working fine for me.

I tested patch from #2 and #31. Here's what I found out:

Before:

#2 & #31 after:

So both of them fixes the issue described on the issue summary. How ever patch #2 causes some extra issues on floating buttons.

Before:

After #2:

After #31:

I didn't find any other problems on #31 so I think it should be instead.

Adding the RTL bit to #31.

The reason why I was adding a new patch was because of the following:
If we add margin to the left on inline-buttons, one way or the other it will break.
Lets imagine 10 inline-buttons where the last 9 have margin-left. Sooner or later those buttons will have to be shown on multiple lines, and so the second line will start with margin-left. You can also translate this to: if the first element inside a inline-container is very wide, there is a big chance the following element won't need margin-left.

I think that the logic should go as follows: every button inside an inline-container, or even more general, every element directly under an inline container, should have margin-right.

Hmmm, maybe when only your last element jumps to the second line? (and without the margin-right it would be on the same line)
So maybe, we should make an exception for all last elements of such a containers
This way, when the first element is the last element, there is still no added margin?

Yea, will patch this evening (with RTL-support( so we can give this another round of testing.

Core module Views has rather strange CSS applied to it, so it doesn't really breaks, it just doesn't make it better either.
Also when editing an image style, the wrapper around the select causes troubles.
Also /admin/config/regional/translate implements it's own logic.

Basically, my code doesn't break the custom code, it only allows them to refactor it's own CSS (but I think that this is out of scope for this or not?)
So:

.container-inline .form-item, // This makes sure random form-items get the margin.
.dropbutton-wrapper, // Because dropbutton is no inline container, no form-item or no button.
.button { // Because buttons often come next to each other
  margin-right: 1em;
}

// Because we want to cancel out the last element in here.
.container-inline .form-item:last-child,
.dropbutton-wrapper:last-child,
.button:last-child {
  margin-right: 0;
}

[dir="rtl"] .container-inline .form-item,
[dir="rtl"] .dropbutton-wrapper,
[dir="rtl"] .button {
  margin-right: 0;
  margin-left: 1em;
}

[dir="rtl"] .container-inline .form-item:last-child,
[dir="rtl"] .dropbutton-wrapper:last-child,
[dir="rtl"] .button:last-child {
  margin-left: 0;
}

Now I suggest that we keep this as core code(I know it isn't pretty), so we can steadily refactor the rest of the code to follow this principle? There is already a lot of CSS written in exactly this way So if we give this as a default, hopefully every module can cheat of one of 3 ways to apply the margins.

Also some silly stand-alone code can now be deleted:

/**
 * Improve spacing of cancel link.
 */
#edit-cancel {
  margin-left: 10px; /* LTR */
}
[dir="rtl"] #edit-cancel {
  margin-left: 0;
  margin-right: 10px;
}

After this we will be able to refactor a lot of code I think. We should also keep in mind to change .container-inline class with .form--inline later on (#55).

I reviewed the patch and works ok, I attached some screenshots.

RTL Page:

Node add page:

Regional translate page:

Recent log messages:

Also I made a minor update solving "trailing whitespace" issue and adding a selector for the image styles issue, I don't know if that is a right solution.

#2229187: SiteEffect: Automated frontend regression testing might be useful here. But I dont know if it can do a rtl install...

Patch passed.

@LewisNyman
Hmmm, I think it might not belong in this patch indeed, maybe leave this out of the patch, and create a follow up on module-level?

I ran a couple of tests on the patch with SiteEffect. Lot of changes are of course detected, I have to go through the whole issue here to get an idea of which button spacings are actually supposed to change. One that doesn't look like it should change is the input field for the height on image resolution form inputs, e.g. on /admin/config/people/accounts/fields/user.user.user_picture

The gif here is probably easier to get https://www.dropbox.com/sh/mvwdhrbyxahj6dj/AABDjQrNiVwScQXMEdZCLFGfa?dl=0

I'll take a closer look at the test results later on.

Thanks for the feedback @ti2m:

It seems to me that they are using a field-suffix to insert a seperator 'x'.
In the logic that field-suffix, should belong more to the previous field, instead of the field after it.
So following that logic, the patch is doing its job?

So I suppose that we should modify the css of this specific module

I uploaded more gifs, not all, but one of each "type of change" that I found. Most look like intended changes, there is on more issue with an input filed, the search button in the search block and I'm not sure about the one with "Apply / Cancel / Remove". Am running tests on RTL right now, will let you know about the results.

RTL seems to introduce the same changes that LTR brings. Here the 'x' separator is at the left of both, the width and height input form. So there is a problem with the separator in general. The RTL run failed to create some gifs so I have to rerun it, but in general it doesn't look like anything gets screwed up.

According to #71 we've introduced a visual regression so putting back to needs work.

This issue was discussed at a Bug Smash Initiative group triage meeting. mstrelan said that this issue is possibly still relevant and it may effect #3198236: "Add another" buttons could use vertical margin.

Adding related items, adding template to the Issue Summary and updating tags.

Adding credit for the triage.