WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 3 months ago

#26646 reviewing defect (bug)

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

Reported by: 5um17 Owned by: obenland
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Themes Keywords: has-patch
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 (4)

screenshot-themes.jpg (161.5 KB) - added by 5um17 19 months ago.
Screenshot showing floated elements are not cleared.
appearance-theme.patch (1008 bytes) - added by 5um17 19 months ago.
Patch for the bug
26646.diff (1.4 KB) - added by obenland 4 months ago.
26646.1.diff (1.5 KB) - added by valendesigns 4 months ago.

Download all attachments as: .zip

Change History (27)

@5um1719 months ago

Screenshot showing floated elements are not cleared.

@5um1719 months ago

Patch for the bug

comment:1 @5um1719 months ago

  • Cc 5um17 added
  • Keywords has-patch added

comment:2 @SergeyBiryukov19 months ago

  • Keywords ui-focus added

comment:3 @nacin17 months ago

  • Component changed from Appearance to Themes
  • Focuses administration added

comment:4 follow-up: @obenland9 months ago

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

comment:5 follow-up: @helen9 months ago

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

comment:6 in reply to: ↑ 5 @obenland9 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.

comment:7 in reply to: ↑ 4 @5um178 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.

comment:8 @obenland6 months ago

#26648 was marked as a duplicate.

comment:9 @obenland6 months ago

  • Milestone changed from Awaiting Review to 4.2

comment:10 @slackbot4 months ago

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

comment:11 @DrewAPicture4 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.

@obenland4 months ago

comment:12 @obenland4 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.

comment:13 @afercia4 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.

@valendesigns4 months ago

comment:14 @valendesigns4 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.

comment:15 @slackbot4 months ago

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

comment:16 @slackbot3 months ago

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

comment:17 follow-up: @DrewAPicture3 months ago

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

comment:18 in reply to: ↑ 17 @obenland3 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.

comment:19 @slackbot3 months ago

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

comment:20 @slackbot3 months ago

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

comment:21 follow-up: @helen3 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.

comment:22 in reply to: ↑ 21 ; follow-up: @5um173 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.

comment:23 in reply to: ↑ 22 @helen3 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.

Note: See TracTickets for help on using tickets.