On Appearance themes page ".themes" div is not covering floated elements.

[5um17]

On theme page, all theme's thumbnail wrapper div are floated left but the parent div (div.themes) do not cover any floated element;
Moreover div.themes is also having 100px bottom padding which is not taking any effect on the page.
In /wp-admin/themes.php have a clearing element <br class="clear" /> inside the div.themes but when rendering is done by tmpl-theme template, the clearing element <br class="clear" /> is being inserted outside of the div.themes.

Also the "add-new-theme" element is being add as last child element of div.themes but last element should be <br class="clear" />
to clear every element, if not then div.add-new-theme will start from new line even though there is room available next to div.theme element.

[5um17]

Screenshot showing floated elements are not cleared.

[5um17]

Patch for the bug

[obenland]

Wouldn't an overflow:hidden; suffice to fix it?

[helen]

I'm curious as to what the bug is here - what practical problem is this causing?

[obenland]

Replying to helen:

I'm curious as to what the bug is here - what practical problem is this causing?

The bottom padding of .themes doesn't get applied.

But it's like the descriptions says, on page load the clearing <br> is the last element of .themes, after the themes are done re-rendering in JS, that element is outside of .themes, losing its effect.

At first I wasn't sure it's actually a JS bug, but it looks like it is.

[5um17]

Replying to obenland:

Wouldn't an overflow:hidden; suffice to fix it?

Yes it is but i think placing the existing clearing element at its right place is good.
I am not sure that 100px bottom padding is needed or not but i am sure that rendering js is not placing <br class="clear" /> indside of div.themes and therefor it is useless element as sibling of div.themes.

[obenland]

#26648 was marked as a duplicate.

[DrewAPicture]

Patch needs a refresh. @obenland: Once there's a new patch, can you please confirm it fixes the problem as you see it and recommend for commit or close? Please and thank you.

[obenland]

In 26646.diff​:

Let's remove those breaks all together and just use an overflow rule, to clear it on both themes screens.

[afercia]

Please consider overflow: hidden will cut-off few pixels of the focus style box-shadows, see screenshot:

Would propose to introduce a new rule in common.css, to use where appropriate:

.contain-floats:after {
	content: '';
	display: table;
	clear: both;
}

For reference: what overflow: hidden does to contain floats? It establishes a new block formatting context, ​see the CSS specs for a definition.

There are also other ways to create a new block formatting context, but then you will have to play with widths, which is not always desirable. For example:

display: table-cell;
width: 100%;

display: inline-block;
width: 100%;

display: table; will work too not because it establishes a block formatting contexts but because it generates an anonymous box with display: table-cell.

Similar issue in #29897.

[valendesigns]

I've added patch 26646.1.diff​ using the clearfix method described above. Though I did not add a new class like it was suggested, because it seems more like an enhancement which probably should have its own ticket.

[DrewAPicture]

@obenland: Can you please take a look at the new patch, 26646.1.diff​ and give your recommendation?

[obenland]

Replying to DrewAPicture:

@obenland: Can you please take a look at the new patch, 26646.1.diff​ and give your recommendation?

The patch looks good. Nice catch @afercia.

I agree with what's been said Slack, that making it a common style is something we should look at earlier in a release, and can be done in 4.3.

[helen]

Punting from 4.2, because:

1. This is not causing functional issues as far as I have been able to ascertain.
2. It looks weird with all that bottom margin applying now, so the visual change needs to be considered.
3. If this is about a holistic change, then let's take it together with general clearfixing: #26396.

[5um17]

Replying to helen:

Punting from 4.2, because:

1. This is not causing functional issues as far as I have been able to ascertain.
2. It looks weird with all that bottom margin applying now, so the visual change needs to be considered.
3. If this is about a holistic change, then let's take it together with general clearfixing: #26396.

I agree with your 2nd and 3rd points but I just want to know, do we only fix functional issues there is nothing to do with CSS standards ? As we all know we need to clear the floated elements for best practice. Apart from this as I commented earlier there is an unused element <br class="clear" /> which have no effect in whole page so I guess we need to fix that.

[helen]

Replying to 5um17:

I just want to know, do we only fix functional issues there is nothing to do with CSS standards ? As we all know we need to clear the floated elements for best practice.

There are a couple of factors there - one is ​our general approach to refactoring, as well as ​our longer-term CSS roadmap. There's any amount of clean up that needs to happen - I like to weigh the short-term effects of doing one thing against the long-term goals.

[afercia]

Core has now a new, generic, CSS .wp-clearfix class introduced in [36171]. Would suggest to use it (see refreshed patch) or close this ticket. cc @obenland :)

[obenland]

Added before and after screenshots. The 100px padding are in place to give the broken themes table some visual separation.26646.3.diff​ adds that to the top and bottom of the no-themes message.

If there are no objections I think this can go in.

[afercia]

Ah! Forgot the broken themes thing :) Looks good to me.

[obenland]

In 36270:

Themes: Clear floated theme cards on Themes page.

Also maintains visual separation for Broken Themes table on searches that
return no results.

See [36171] for .wp-clearfix.

Props 5um17, obenland, valendesigns, afercia.
Fixes #26646.