WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 5 months ago

#26646 closed defect (bug) (fixed)

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

Reported by: 5um17 Owned by: obenland
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.8
Component: Themes Keywords: has-patch has-screenshots commit
Focuses: ui, administration Cc:

Description

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.

Attachments (8)

screenshot-themes.jpg (161.5 KB) - added by 5um17 2 years ago.
Screenshot showing floated elements are not cleared.
appearance-theme.patch (1008 bytes) - added by 5um17 2 years ago.
Patch for the bug
26646.diff (1.4 KB) - added by obenland 15 months ago.
26646.1.diff (1.5 KB) - added by valendesigns 15 months ago.
26646.2.diff (2.1 KB) - added by afercia 5 months ago.
26646.3.diff (2.1 KB) - added by obenland 5 months ago.
before-26646.3.png (40.3 KB) - added by obenland 5 months ago.
after-26646.3.png (46.1 KB) - added by obenland 5 months ago.

Download all attachments as: .zip

Change History (35)

@5um17
2 years ago

Screenshot showing floated elements are not cleared.

@5um17
2 years ago

Patch for the bug

#1 @5um17
2 years ago

  • Cc 5um17 added
  • Keywords has-patch added

#2 @SergeyBiryukov
2 years ago

  • Keywords ui-focus added

#3 @nacin
2 years ago

  • Component changed from Appearance to Themes
  • Focuses administration added

#4 follow-up: @obenland
20 months ago

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

#5 follow-up: @helen
20 months ago

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

#6 in reply to: ↑ 5 @obenland
20 months ago

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.

#7 in reply to: ↑ 4 @5um17
19 months ago

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.

#8 @obenland
17 months ago

#26648 was marked as a duplicate.

#9 @obenland
17 months ago

  • Milestone changed from Awaiting Review to 4.2

This ticket was mentioned in Slack in #core by drew. View the logs.


15 months ago

#11 @DrewAPicture
15 months ago

  • Keywords needs-refresh added
  • Owner set to obenland
  • Status changed from new to reviewing

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
15 months ago

#12 @obenland
15 months ago

  • Keywords needs-refresh removed

In 26646.diff:

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

#13 @afercia
15 months ago

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

https://cldup.com/3q20tzKt8N.png

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.

#14 @valendesigns
15 months ago

  • Keywords needs-testing added

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.

This ticket was mentioned in Slack in #core by afercia. View the logs.


15 months ago

This ticket was mentioned in Slack in #core by drew. View the logs.


14 months ago

#17 follow-up: @DrewAPicture
14 months ago

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

#18 in reply to: ↑ 17 @obenland
14 months ago

  • Keywords commit added; needs-testing removed

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.

This ticket was mentioned in Slack in #core by obenland. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by obenland. View the logs.


14 months ago

#21 follow-up: @helen
14 months ago

  • Keywords commit removed
  • Milestone changed from 4.2 to Future Release

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.

#22 in reply to: ↑ 21 ; follow-up: @5um17
14 months ago

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.

#23 in reply to: ↑ 22 @helen
14 months ago

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
5 months ago

#24 @afercia
5 months ago

  • Milestone changed from Future Release to 4.5

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
5 months ago

#25 @obenland
5 months ago

  • Keywords has-screenshots commit added

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.

#26 @afercia
5 months ago

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

#27 @obenland
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.