This isn't for user content. This is for clearing floats within the skin itself?

Or... what? I don't understand. You still need to make an element as a spacer for a lot of this... yes, flexbox is a thing, but we're not gonna all of a sudden convert every skin to it now, why would we do that?

Skins should instead copy across the method to their own skin or use the LESS mixin instead

...what? Why is that any better? How is this not just breaking things for the sake of breaking things?

...what? Why is that any better? How is this not just breaking things for the sake of breaking things?

Let me start by saying I'm open-minded here. You can convince me to adapt how I'm thinking about this.

The main issue here is that with T89981 there will no longer be shipping the .visualClear style without a deprecation warning so it's a bit odd for a function to exist that just provides a simple piece of HTML with a simple class, but for no CSS to be associated with it.

I'm not strongly opposed to keeping it, but perhaps if we do keep it, it would make sense for the function to be modified and also apply an inline style attribute clear: both; ?

The PHP method for completion is this code:

	/**
	 * Get a div with the core visualClear class, for clearing floats
	 *
	 * @return string html
	 * @since 1.29
	 */
	protected function getClear() {
		return Html::element( 'div', [ 'class' => 'visualClear' ] );
	}

A modified version if we did decide to keep it would be

	protected function getClear() {
		return Html::element( 'div', [ 'class' => 'visualClear', 'style' => 'clear:both' ] );
	}

Would that work for you?

That aside, I want to note that all these helper functions add up. Skin is 2684 lines of code, SkinTemplate is 1290 lines of code and BaseTemplate is 468 lines. I'm keen to simplify all these classes to make the code more easier to reason with. I'm more concerned with the former two however so I am not averse to changing my mind here.

Having this helper function in core doesn't seem helpful from my perspective. It's not doing anything complicated. Can you help me understand why you find it valuable? I felt it would be better for skins to maintain this themselves (rather than rely on some code in core that's subject to breakage as we're discovering now)

I just want to note that if the HTML approach is preferred over mixin approach, skins would still be able to copy across the PHP function and following CSS rule:

.visualClear {
        clear: both;
}

If the concern is I am adding a burden on maintainers I am happy to own supplyinga nd reviewing all the patches here :-)

But why remove it from core? If skins aren't using it, then this doesn't affect them. If skins are using it, they're often using a combination of getClears and applying the class to specific elements where a full separate clear isn't needed.

And why remove an effective standard? Users also can use the visualClear class for in-page content. Site admins can use it for site customisations. Randomly taking it away from everyone, forcing skins to roll their own, means stuff will stop working on various sites, as well as randomly complicating the skins themselves and forcing maintainers to implement unexpectedly needed fixes... and we may also just wind up with multiple new ones that are all slightly different as a result as well. That's the opposite of what we want; we want to minimise surpise by sticking to singular solutions, especially if they've already been established for years.

Please put the CSS back and leave the function be.

Extensions also use the visualClear class. Yes, they could also define their own versions, but... why?

https://codesearch.wmcloud.org/extensions/?q=visualClear&i=nope&files=&excludeFiles=&repos=

In T280609#7021994, @Isarra wrote:

This isn't for user content. This is for clearing floats within the skin itself?

This was wrong. It's for everything. I just... slightly broke my brain when I first saw this (and a related task that mentioned user use specifically).

In T280609#7022205, @Isarra wrote:

Extensions also use the visualClear class. Yes, they could also define their own versions, but... why?

https://codesearch.wmcloud.org/extensions/?q=visualClear&i=nope&files=&excludeFiles=&repos=

This. So much this.

.visualClear is used in all social tools; the old, custom .cleared class was intentionally dropped finally circa August 2015 in favor of something that would always be present in all sane skins (!).
MediaWiki-extensions-BibManager and MediaWiki-extensions-ConfirmAccount also seem to use the class, for example.

I don't think it matters how you render <div class="visualClear"></div>, whether manually, via BaseTemplate#getClear or some other way, but the class (definition) definitely needs to stay.

In T280609#7039423, @ashley wrote:

I don't think it matters how you render <div class="visualClear"></div>, whether manually, via BaseTemplate#getClear or some other way, but the class (definition) definitely needs to stay.

This. BaseTemplate::getClear is kinda... it doesn't really matter. It can easily be in the skin or done manually or just skipped, these days. I'm not really sure what the point of removing it really is, nor the point of keeping it. But the class definition is a long-standing standard that should be consistent regardless of skin, because things that aren't skins expect it.

We've long had problems with (content) styles that should be consistent and are generally expected regardless of skin not being available or usable in core (either there was no core module that included the styles at all, despite them being fairly low-level and universal, or the module that did also included visual styles that clashed with other skins). I'm a little concerned that the list of things where this is the case just seems to be getting longer.

Also if there was any doubt that this specifically is in fact widely used onwiki as well:

https://fr.wikipedia.org/w/index.php?search=%22.visualClear%22&title=Special%3ASearch&profile=advanced&fulltext=1&ns0=1&ns1=1&ns2=1&ns3=1&ns4=1&ns5=1&ns6=1&ns7=1&ns8=1&ns9=1&ns10=1&ns11=1&ns12=1&ns13=1&ns14=1&ns15=1&ns100=1&ns101=1&ns108=1&ns109=1&ns118=1&ns119=1&ns710=1&ns711=1&ns828=1&ns829=1

I'd file a task specifically about putting the .visualClear definition back into some relevant cross-skin module/feature, but I'm still not really up to speed on what the relevant projects are, or what the common modules/features are. Or if any even exist anymore.

Firstly, we have a long timeline here. Skins that want to support this rule can. Minerva has never supported visualClear and will continue to not support it. The crux of the issue is that if skins want to support it they need to explicitly add a rule as we deprecate the legacy bundle of styles that many skins use.

I don't mind if we keep BaseTemplate::getClear but if we do we need to maybe rethink it's documentation or how it gets styled.

For Vector this is how I'm looking at it:

1. When T89981 gets wrapped up the legacy feature will be soft deprecated
2. As a Vector maintainer, I will enable the features that make sense to my skin, and copy across any classes that didn't make the cut. In the case of visualClear, Vector will probably continue to support it via a dedicated (and documented) rule for a transitional period noting the extensions it's there for.
3. T89981 will be hard deprecated. (Probably 1.37, maybe 1.38)
4. We will delete the code.

Extensions also use the visualClear class. Yes, they could also define their own versions, but... why?

There is a problem here - those extensions are assuming that they are running in a skin loading mediawiki.skinning.elements' or the legacy code via some other mechanism. That's just not true.

This is one line of pretty obvious CSS. All these little lines of CSS adds up, so why not just add it. If the rule is repeated, gzip will mean no extra bytes.

Extensions using this class are currently rendering broken in a bunch of skins including Minerva (and apps if applicable) and the answer here is simple as outlined above.

But the class definition is a long-standing standard that should be consistent regardless of skin, because things that aren't skins expect it.

The BaseTemplate::getClear is not a public method, so this method/class was never intended to be used by extensions. When I add classes to skins, I expect those to stay internal to the skin, not to become public APIs that other extensions use.

I think if we want to support a clear rule, it might make sense to add a public Html::getClear method which returns`<div style="clear:both;"></div>` and use that instead of the existing BaseTemplate::getClear. This way skins and extensions can use it with the desired behaviour and it is more future proof.

Please put the CSS back and leave the function be.

The problem is that there is nowhere obvious for the .visualClear CSS rule to live. All styles in ResourceLoaderSkinModule are meant to be optional. The styles here are not to support extensions which may or may not be installed. I'm not adding a mandatory module just for one simple line of CSS. That's what CSS mixins are for IMO.

The problem is that there is nowhere obvious for the .visualClear CSS rule to live.

Please see T280723#7047590. If there are no features for common skin-agnostic content, why are there features at all? What is the point of all this?