WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

#26396 closed enhancement (fixed)

Implement a generic clearfix

Reported by: helen Owned by: afercia
Milestone: 4.5 Priority: low
Severity: minor Version: 4.4
Component: Administration Keywords: has-patch
Focuses: ui Cc:

Description

We have a clearfix defined in core, but it's applied to very specific items. It should be a generic class. We also have an old-school br.clear and a .clear class - should look for those and replace them in core wherever it makes sense.

Attachments (7)

26396.patch (451 bytes) - added by afercia 9 months ago.
26396.2.patch (829 bytes) - added by afercia 9 months ago.
26396.3.patch (6.6 KB) - added by afercia 9 months ago.
26396.4.patch (7.1 KB) - added by afercia 9 months ago.
26396.5.remove-old-clearfix.patch (15.2 KB) - added by afercia 9 months ago.
26396.6.remove-old-clearfix.patch (15.3 KB) - added by afercia 9 months ago.
26396.7.remove-old-clearfix.patch (14.1 KB) - added by afercia 8 months ago.

Download all attachments as: .zip

Change History (36)

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


19 months ago

#2 @afercia
19 months ago

  • Focuses ui administration added
  • Keywords ui-fous removed

+1 for this.
A more modern method to contain floated elements would be very useful especially now that core is using a focus style with box-shadows. In many places they're "broken" cause overflow: hidden used to contain floats. See for example: #29897 and #26646. Would suggest:

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

Naming convention to discuss of course.

#3 @joemcgill
19 months ago

For reference: Chris Coyier has kept a pretty good running diary of the various CSS patterns for this here: https://css-tricks.com/snippets/css/clear-fix/

#4 @afercia
19 months ago

thanks @joemcgill. Had a second thought about naming convention, I was wrong. With overflow: hidden still in my mind I was thinking to "contain floats" because that's what overflow: hidden actually does, but with the pseudo-element it's really a "clearfix" :)

#5 @wonderboymusic
12 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

I would love for someone to pick this up - there are divs floating in the breeze all over the place that have no layout, yet contain untold numbers of floated elements

#6 @afercia
12 months ago

Related #28864 among other things tries to introduce a generic rule for this in common.css:

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

#7 @afercia
9 months ago

  • Focuses administration removed
  • Milestone changed from Future Release to 4.5
  • Owner set to afercia
  • Status changed from new to assigned
  • Version set to 4.4

The original clearfix mentioned in the ticket description is now in nav-menus.css. On the other hand, [35673] introduced a new rule in common.css, currently used only for the "navigation tabs" container:

/* contain floats */
.nav-tab-wrapper:after {
	content: "";
	display: table;
	clear: both;
}

I'd suggest to just add a generic class name, maybe "wp-clearfix"?

/* modern clearfix */
.wp-clearfix:after,
.nav-tab-wrapper:after {
	content: "";
	display: table;
	clear: both;
}

cc @michaelarestad cause I know he's passionate about CSS naming conventions :)

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


9 months ago

#9 follow-up: @michaelarestad
9 months ago

@afercia You know me so well. I agree that .wp-clearfix is an ideal class name. Once in place, we could remove the need for .nav-tab-wrapper and any other classes using the rules for a clearfix.

@afercia
9 months ago

#10 in reply to: ↑ 9 @afercia
9 months ago

Replying to michaelarestad:

we could remove the need for .nav-tab-wrapper and any other classes using the rules for a clearfix.

Thinking probably it's not so easy. For example:

  • plugins already use the "Tabs" so removing the clearfix from .nav-tab-wrapper will break their layout since it's unlikely plugins authors are going to add .wp-clearfix
  • stuff that can be enqueued in the frontend (e.g. Editor, Media Modals) would need a clearfix rule in a file that's available in the frontend

I'd say that for now the most important thing is introducing the new clearfix and use it for new developments. See proposed patch. Any thoughts more than welcome.

#11 follow-up: @michaelarestad
9 months ago

I'd say that for now the most important thing is introducing the new clearfix and use it for new developments. See proposed patch. Any thoughts more than welcome.

Ah. I agree with that. The patch looks good, though makes me a bit sad that we need to keep the extra class in there.

#12 in reply to: ↑ 11 @afercia
9 months ago

Replying to michaelarestad:

though makes me a bit sad that we need to keep the extra class in there.

Yep :( maybe one more reason to seriously consider @helen 's idea to introduce a deprecated.css

@afercia
9 months ago

#13 @afercia
9 months ago

The refreshed patch moves the new clearfix rule to the "utility classes" section.

@afercia
9 months ago

#14 follow-up: @afercia
9 months ago

Thinking it would be better to start using the new CSS class in the cleanest possible way so what about something like 26396.3.patch where:

  • Tabs output by core will use wp-clearfix
  • Tabs output by plugins will fallback to a slightly less-ideal clearfix method overflow: hidden

The CSS would be cleaner and the back-compatibility rules ready to be moved in a deprecated file. Any thoughts more than welcome.

#15 in reply to: ↑ 14 ; follow-up: @michaelarestad
9 months ago

Replying to afercia:

Thinking it would be better to start using the new CSS class in the cleanest possible way so what about something like 26396.3.patch where:

  • Tabs output by core will use wp-clearfix
  • Tabs output by plugins will fallback to a slightly less-ideal clearfix method overflow: hidden

I generally stay away from overflow: hidden because it could have unexpected results. I'm not sure we get a clear benefit from doing this other than completely separating the clearfix. My preference would be to either duplicate the clearfix to keep the clearfix class separate from other classes or just go with something like 26396.2.patch.

I lean toward the duplication of rules. I'm also wondering if we should start separating out utility classes like wp-clearfix from common.css into their own file.

#16 in reply to: ↑ 15 @afercia
9 months ago

Replying to michaelarestad:

I lean toward the duplication of rules.

Thanks Michael. OK, wiill refresh the patch.

I'm also wondering if we should start separating out utility classes like wp-clearfix from common.css into their own file.

Good point :)

@afercia
9 months ago

#17 @afercia
9 months ago

  • Keywords has-patch added; needs-patch removed

OK so in order to keep the clearfix class separate from other classes and at the same time be as clean as possible and preserve back-compat I'd propose:

  • for modern browsers, use .nav-tab-wrapper:not(.wp-clearfix):after
  • for IE 8, no need to be 100% clean so I'd propose to always establish a new block formatting context

See refreshed patch.

Open to suggestions and improvements. Can also be refined later. For now, I'd propose to commit what we have and move on since this is blocking other tickets (see for example #28864).

Will keep this ticket open for further patches to replace the old clearfix and all the br.clear and div.clear. Patches welcome :)

#18 @afercia
9 months ago

In 36171:

Introduce a new generic CSS clearfix utility class.

.wp-clearfix is now the recommended way to clear and contain floated elements.
Adds back compatibility for the .nav-tab-wrapper navigation tabs.

See #26396.

#19 @afercia
9 months ago

Patch 26396.5.remove-old-clearfix.patch is a first attempt to remove the old clearfix applied to specific items across the admin. Some testing would be nice :)

Last edited 9 months ago by afercia (previous) (diff)

#20 @paulwilde
9 months ago

@afercia <div id="widgets-right wp-clearfix"> should be <div id="widgets-right" class="wp-clearfix"> in src/wp-admin/customize.php.

Last edited 9 months ago by paulwilde (previous) (diff)

#21 @afercia
9 months ago

@paulwilde thanks! Patch refreshed.

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


9 months ago

#23 @afercia
8 months ago

Refreshed patch after recent changes to the markup. Patches like this one tend to stale quickly so I'd propose to commit now and if something breaks I think there's enough time to fix it at this point of the release cycle. By the way, looks good to me.

#24 @afercia
8 months ago

In 36422:

After [36171] remove all the occurrences of the old CSS clearfix.

The old clearfix was applied to very specific items and defined multiple times
across CSS files. Uses the new generic .wp-clearfix utility class instead.

See #26396.

#25 follow-up: @lkraav
8 months ago

What's the ticket for dropping the legacy box model flow breaking floating strategy and moving to modern grid approach like flexbox instead?

#26 in reply to: ↑ 25 @afercia
8 months ago

Replying to lkraav:

What's the ticket for dropping the legacy box model flow breaking floating strategy and moving to modern grid approach like flexbox instead?

@lkraav there isn't one, as far as I know. Please do feel free to open a new ticket if you think starting a discussion about flexbox is worthy. I'm afraid there's no consensus about the adoption of the flexbox model for core, yet. Also, core still supports IE 8 and 9.

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


7 months ago

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


7 months ago

#29 @jorbin
7 months ago

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

Calling this complete for 4.5. If there are other changes needed in 4.5, we can reopen it as a task.

Note: See TracTickets for help on using tickets.