Divs in the container-inline wrapper should be inline-block instead of inline

Tested and fixes the issue.

@webchick mentioned this issue in IRC, I think we'll need more screenshots because the .container-inline wrapper is used all over the place, including in Views.

It turns out the screenshots here are from Views, oops!

Searching through core to find instances of "container-inline" will help to find where this markup is used, it would be nice to find and take screenshots of as many of those as possible and make sure we're not visually breaking anything with this change.

(See also #2099579: Discuss automated visual regression testing)

Comment tags were not rendered in the summary update so I have removed the inapplicable sections in prep for the Austin sprint.

I'll review this patch and take some screenshots now.

I applied the patch, the patch applied cleanly, but didn't seem to make any difference (cache was cleared). The fix appears to already be in core.

I'm going to take an attempt at this.

Working on issue for Austin DrupalCon Sprint.

Did a search of the core directory and found the following files to review:

$ find core -type f -exec grep -l "container-inline" {} \;
core/modules/action/src/Form/ActionAdminManageForm.php
core/modules/comment/src/Form/CommentAdminOverview.php
core/modules/datetime/datetime.module
core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDatelistWidget.php
core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDefaultWidget.php
core/modules/dblog/src/Form/DblogFilterForm.php
core/modules/editor/editor.admin.inc
core/modules/editor/src/Form/EditorImageDialog.php
core/modules/field/src/Plugin/views/field/Field.php
core/modules/field_ui/src/Form/FieldEditForm.php
core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
core/modules/locale/src/Form/TranslateFilterForm.php
core/modules/path/src/Form/PathFilterForm.php
core/modules/search/search.module
core/modules/search/src/Form/SearchPageForm.php
core/modules/search/src/SearchPageListBuilder.php
core/modules/simpletest/src/Form/SimpletestResultsForm.php
core/modules/system/css/system.module.css
core/modules/system/css/system.theme.css
core/modules/system/src/Plugin/views/field/BulkForm.php
core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php
core/modules/views_ui/admin.inc
core/modules/views_ui/css/views_ui.admin.theme.css
core/modules/views_ui/src/Form/Ajax/AddHandler.php
core/modules/views_ui/src/ViewAddForm.php
core/modules/views_ui/views_ui.theme.inc
core/themes/seven/seven.theme

I confirmed the above list
grep -rl 'container-inline' core/

Instances at admin/config/search/pages

Search block:

core/modules/search/search.module

Found an instance where it breaks the UI.

This in the Search Page Form: core/modules/search/src/Form/SearchPageForm.php

These all look good. Sorry about the retina-fied screenshot sizes.

Here is a patch changing "display:inline" to "display:inline-block".

I have been testing this - the one bug I noticed was on the search form. I have created a patch to fix this. I have added screen shots to show before/after I applied patch #22 as well as a screen shot with my fix.

New patch that isn't corrupt :)

Let's see what the testbot says ;)

+++ b/core/modules/system/css/system.module.css
@@ -264,6 +264,10 @@ tr .ajax-progress-throbber .throbber {
+.container-inline .form-type-search{

Missing space before opening curly bracket.

Third time lucky!

It's RTBC for me. Go testbot go!

Congrats @lilGemVinny for your first contribution and thank you so much ;)

Bot failure? Looks good still.

Test bot you're drunk, go home!

+++ b/core/modules/system/css/system.module.css
@@ -264,6 +264,10 @@ tr .ajax-progress-throbber .throbber {
+/* Inline search form */
+.container-inline .form-type-search {
   display: inline;
 }

Are we sure this is the correct approach?

Perhaps we need to adjust bartik's search form styling?

I removed the Novice tag cause this issue needs a decision and has no novice task now.

Thanks Alex, good point. Here's a screenshot of the search form in Stark without the code:

.container-inline .form-type-search {
display: inline;
}

So it looks like we don't need this code to sit in system. It can sit in Bartik. Here's an updated patch. I also noticed that the ID selector for Bartik was totally wrong so I had to update that as well.

It seems it works.

Well now the search form styling has changed. I think we need need a separate issue for search block styling in bartik and we should only fix the inline-ness here. If you have two search blocks on the same page in bartik they have different buttons - lol.

Ok, in that case the simplest fix is the patch previous to mine. Marking #29 back to RTBC

I'm really concerned that this patch is going to introduce visual inconsistencies all over the place. See the before and after of the node type vertical tabs. Yes "Authored on" with the patch now has padding but none of the other form elements do making this look really inconsistent.

Maybe the only sensible thing to do is to postpone it to 8.1.x at this stage? The risk for regression is very high here.

Opening this baby back up!

Why i am getting this error even after " git pull " . Do i need to manually copy these folders from 8.0.x or what ? Can any one help me please.
Also there is no vendor folder in 8.2.x. Most of the time i am getting this type of error while trying to work with 8.2.x.

Rerolling...
@dbjpanda that means that the patch you are trying to apply is trying to make changes to files that are no longer there. This is mainly because of the changes that happened since the patch was created. See https://www.drupal.org/patch/reroll for how to reroll patches ;)

Here is the reroll.

The patch is much smaller because the search form is now properly styled on bartik/css/components/search-form.css, so I believe those changes were no longer necessary.

We may not be able to change stable here but the rest looks great. Could use a few screenshots to see if it breaks anything but in general this makes some of the block attributes like width, padding, margin etc work as expected.

Taking a look. NOLA DrupalCon sprint. Will run a fresh grep and some updated screenshots.

grep -rl 'container-inline' core/ (on 8.1.1)
core/modules/comment/src/Form/CommentAdminOverview.php
core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
core/modules/action/src/Form/ActionAdminManageForm.php
core/modules/simpletest/src/Form/SimpletestResultsForm.php
core/modules/system/css/components/container-inline.module.css
core/modules/system/system.libraries.yml
core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
core/modules/system/src/Plugin/views/field/BulkForm.php
core/modules/system/tests/themes/test_theme/test_theme.info.yml
core/modules/search/src/SearchPageListBuilder.php
core/modules/search/src/Form/SearchPageForm.php
core/modules/views_ui/css/views_ui.admin.theme.css
core/modules/views_ui/views_ui.theme.inc
core/modules/views_ui/src/Form/Ajax/AddHandler.php
core/modules/views_ui/src/ViewAddForm.php
core/modules/views/src/Plugin/views/field/Field.php
core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php
core/modules/node/node.module
core/modules/locale/src/Form/TranslateFilterForm.php
core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
core/modules/dblog/src/Form/DblogFilterForm.php
core/modules/editor/editor.admin.inc
core/modules/editor/src/Form/EditorImageDialog.php
core/modules/path/src/Form/PathFilterForm.php
core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
core/themes/bartik/templates/block--search-form-block.html.twig
core/themes/stable/css/system/components/container-inline.module.css
core/themes/stable/css/views_ui/views_ui.admin.theme.css
core/themes/stable/stable.info.yml
core/themes/seven/css/components/search-admin-settings.css
core/themes/seven/seven.theme
core/themes/classy/classy.libraries.yml
core/themes/classy/templates/block/block--search-form-block.html.twig
core/themes/classy/templates/form/datetime-form.html.twig
core/themes/classy/css/components/container-inline.css

I agree with @alexpott, this introduces style inconsistencies that may be a problem given how many places the .container-inline class appears. I'm including a more recent before/after where you can see the change (Bartik) in the 'authored on' fields.

Thanks @studiozut for that!

+++ b/core/themes/bartik/css/components/header.css
@@ -165,10 +165,10 @@
+.region-header #search-block-form .form-text {

From the divs_in_the-2226317-49.patch, lines 169 and 172: I'm not sure why this is part of the container-inline issue, maybe it should be addressed separately?

+++ b/core/themes/bartik/css/components/header.css
@@ -165,10 +165,10 @@
-.region-header #block-search-form {
+.region-header #search-block-form {
...
-.region-header #block-search-form .form-text {
+.region-header #search-block-form .form-text {

I agree that this is out of scope for this issue, it was there before so I re-rolled it without giving it much thought. I have now removed them from this patch.

I agree that there're going to need to be a lot of adjustments across the board to make the change towards inline-block. Starting funnily enough with the search block which is broken with this patch.

#block-search-form is no longer, and now the form has an id of #search-block form. So those lines should probably be removed from there since they do nothing, a nice novice issue ;)

Latest patch should be the starting point, next steps would be:

1. Identify everything that breaks visually with this change
A good starting point might be double checking the files that use this (#53), and see where these are displayed to check for inconsistencies.

2. Fix it.

There are several issues being created after applying the patch #59. This is making many of the Divs inline-block where this is not necessary.

Screen shot attached please check.

Updated new patch with fixing the issue pointed in earler comment #62. Added new line for search form which fixed the alignment breaking issue.

now its working fine tested by applying above patch.

I don't think we can make the change like this at this point in the Drupal 8 cycle as it might break contrib and custom themes that have extended classy or stable. We shouldn't be making this change to stable. The question is should we be making this change to core's CSS. I'm going to ping @Cottser for an opinion.

Yep I agree with @alexpott, we can't change Stable or Classy for this. We can change the core CSS, Bartik, and Seven.

Removing the screenshot and manual testing tags until we have a new patch to test.

@Cottser, @alexpott you're right of course - previous patch was changing stable...

Here is a patch that only changes core's css.

So this patch should have no visual regressions, since both Bartik and Seven have classy as base theme, classy's base theme is the default which is stable, and stable overrides this change.

@cottser Sorry my miss, written code in theme not in core.

Applied patch and manually tested.Its working fine.

inline-block makes sense for something called container-inline. Too risky to commit to core themes but a good change for core CSS itself.

Committed 40136a9 and pushed to 8.2.x. Thanks!

in Drupal 8.6.9
not fix /core/themes/stable/css/system/components/container-inline.module.css

@agrozyme, ^ that should probably be opened as a new issue against core , ideally with screenshots showing the error in the search form.