Page MenuHomePhabricator

Implement a core 'clearfix' mixin in mediawiki.mixin and evaluate deprecation/removal of 'visualClear' class
Closed, ResolvedPublic2 Estimated Story Points

Description

In course of Desktop Improvements project's T240489: [Epic] Determine the optimum Vector DOM structure for a11y and performance I've re-encountered the horrific .visualClear class.
It mostly results in having an empty div DOM element with above class, to clear floated elements above. An anti-pattern for presentational purpose only.

For separating presentation and structure, I propose to

  • add a 'clearfix' mixin to mediawiki.mixins that
    • allows to choose between collapsing margins (display: block) or non-collapsing margins (display: table) for modern browsers to not rely on extra DOM element, but be used on container element for floated child elements

Such 'clearfix' mixin would also enable us to exchange the containing internals with display: flow-root; once widely enabled, leaving the public mediawiki.mixins syntax intact.

Different implementations and their behavior: https://codepen.io/DemianX0/pen/PoZjBxx
List of places where .clearfix is used: T254195#6255319

Related Objects

StatusSubtypeAssignedTask
ResolvedGoalovasileva
OpenNone
Resolvedovasileva
ResolvedSpikeovasileva
ResolvedSpikephuedx
Resolvedovasileva
OpenSpikeNone
ResolvedJdrewniak
ResolvedSpikeovasileva
Resolvedovasileva
ResolvedBUG REPORTmatmarex
Resolvedovasileva
ResolvedJdlrobson
Resolvedphuedx
Resolvednray
ResolvedMayakp.wiki
ResolvedMayakp.wiki
Stalledovasileva
OpenNone
ResolvedEdtadros
OpenNone
OpenNone

Event Timeline

Volker_E created this task.Jun 1 2020, 8:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 1 2020, 8:48 PM

Change 601497 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] mediawiki.mixins: Add .mixin-clearfix()

https://gerrit.wikimedia.org/r/601497

Change 599611 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] Use core .mixin-clearfix() instead of DOM element

https://gerrit.wikimedia.org/r/599611

Related tasks: T156561 and comment T114071#1792256 on .visualClear.

Demian added a subscriber: Demian.EditedJun 25 2020, 1:47 AM

Various uses of clearfix and synonyms:

https://gerrit.wikimedia.org/g/mediawiki/extensions/ContentTranslation/+/e058638ce115f66fb42b499ae89dc4ca2ab3e13a/modules/ui/styles/mw.cx.mixins.less#97
https://gerrit.wikimedia.org/g/mediawiki/extensions/UploadWizard/+/d1fb717939352c12a3d5dc4ab5d43efe2fbf89ba/resources/uploadWizard.css#578
https://gerrit.wikimedia.org/g/mediawiki/extensions/LinkedWiki/+/56016932c0d946d4b4f85c730202b98bc7d906dc/resources/bootstrap/dist/bootstrap.light.css#5947
https://gerrit.wikimedia.org/g/mediawiki/skins/mediawiki-strapping/+/3df08e62a2e3e36f2008d7727d33a7c4f10ec551/bootstrap/css/bootstrap-responsive.css#15
https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/5e6e294083675b0f16feee26ac74fea7b404717d/minerva.less/minerva.mixins.less#14
https://github.com/thaider/Tweeki/blob/master/bootstrap/css/bootstrap.css#L6442
https://gerrit.wikimedia.org/g/mediawiki/extensions/DonationInterface/+/4bc8170d9c5d3ea7856ca2217b9aeb053b46f969/adyen_gateway/forms/iframe/css/screen.css#290
https://gerrit.wikimedia.org/g/mediawiki/extensions/GettingStarted/+/86c292473bebb07d9385f8a002b8d1ba7f63c400/resources/lightbulb/lightbulb.common.less#7
https://gerrit.wikimedia.org/g/mediawiki/skins/Bouquet/+/193caab0cb289969ed8ddfcb1dde116bd14c2821/resources/style.css#872
https://gerrit.wikimedia.org/g/mediawiki/skins/Nimbus/+/e07d984a78b9f76e0ff3d3ea3ef1fa25240ec64e/resources/styles/skin.nimbus.hacks.clear.less#11
https://gerrit.wikimedia.org/g/mediawiki/extensions/MsCalendar/+/6c082694ad88e177aeacf24c560d7640e28f5761/resources/css/MsCalendar.css#291
https://gerrit.wikimedia.org/g/mediawiki/extensions/WikibaseLexeme/+/440602ca5da2db3453c0e07d3711798f86d2bd3c/resources/lexeme.less#1
https://gerrit.wikimedia.org/g/mediawiki/services/mobileapps/+/a413db4f02f9c61517c03221fd626989a6959b72/private/styles/minerva/minerva.less/minerva.mixins.less#15
https://gerrit.wikimedia.org/g/oojs/ui/+/037b996c59c5e0680fb64798dcb104d6563df4c1/src/styles/common.less#137
https://gerrit.wikimedia.org/g/mediawiki/skins/WikimediaApiPortal/+/3f9d1407cc75eae121194a25333975d02af7dc2e/dist/bootstrap.css#2290
https://github.com/jthingelstad/foreground/blob/master/assets/stylesheets/foundation.css#L96

Source: https://codesearch.wmflabs.org/search/?q=%5C.clearfix&i=nope&files=&repos=
They all seem to be a variant of the pseudo-element clearfix reloaded or very similar.

There's some visualClear without pseudo-elements: https://codesearch.wmflabs.org/search/?q=%5C.visualClear&i=nope&files=&repos=

http://nicolasgallagher.com/micro-clearfix-hack/ should be noted here as the margin-top non-collapsing version, using display: table.
https://www.cssmojo.com/the-very-latest-clearfix-reloaded/ collapses margin-top, but not margin-bottom.

Codepen demonstrating the different behavior of margin-top and margin-bottom collapsing:
https://codepen.io/DemianX0/pen/PoZjBxx

Minimal, simplified version:

.clearfix {
	&:after {
		clear: both;
		content: '';
		// Margin collapsing is a feature, not a bug, hence relying on `block`.
		display: block;
	}
}

.clearfix( false ) {
	&:after {
		clear: both;
	}
	&:before,
	&:after {
		content: '';
		display: table;
	}
}
Demian added a comment.EditedJun 28 2020, 12:01 AM

When I look at the generated CSS, I start wondering if it's worth making this a mixin. Classes can be inherited just like mixins with the added benefit of being available in HTML. Correction: not in less-php in this format.

.clearfix:after {
  clear: both;
  content: '';
  display: block;
}
.clearfix-nocollapse:after {
  clear: both;
}
.clearfix-nocollapse:before,
.clearfix-nocollapse:after {
  content: '';
  display: table;
}
Dudewithswag updated the task description. (Show Details)

Change 601497 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.mixins: Add .mixin-clearfix()

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /601497

Demian added a subscriber: Jdrewniak.EditedJun 29 2020, 8:33 PM

Change 601497 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.mixins: Add .mixin-clearfix()
https://gerrit.wikimedia.org/r/c/mediawiki/core/ /601497

@Jdrewniak It was forgotten to give credit to my work in this patch. Now that the commit message cannot be modified, what do you suggest to make up for that missing bit?

Demian updated the task description. (Show Details)Jun 29 2020, 8:44 PM
Volker_E updated the task description. (Show Details)Jun 29 2020, 10:46 PM

Change 601497 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.mixins: Add .mixin-clearfix()
https://gerrit.wikimedia.org/r/c/mediawiki/core/ /601497

@Jdrewniak It was forgotten to give credit to my work in this patch. Now that the commit message cannot be modified, what do you suggest to make up for that missing bit?

Repeating from the patch set: What is the unmet expectation here?

Demian added a comment.EditedJun 29 2020, 11:02 PM

Repeating from the patch set: What is the unmet expectation here?

It is customary to give credit to the person doing the hard part of the work, in this case 100% of the code. Even for less, colleagues have given credit in the commit comment. The commit comment now cannot be changed. What do you suggest?

Putting aside some thoughts about ideation and contributions to that task (see description history and patch set history), repeating, what means "give credit", what is the expectation here?

what means "give credit"

I'm surprised that needs an explanation. In open source software that's like 1+1. "Give attribution" is a synonym.

what is the expectation

I assume you've seen the examples of attributing one's work. There are some examples in recently reviewed patches. It's written in the commit comment as I've stated above.

This patch is needed for the Vector work now, but it was in limbo for 3 weeks, so I've spent the necessary hours to test 5+ different implementations in different use-cases and came up with the solution you've committed.
In this case as I've provided a full implementation from scratch with new ideas, the simplest appropriate action would have been to ask me to submit it as a patch. That's how I handle when someone helps me do my work.

Aklapper removed Dudewithswag as the assignee of this task.Jun 30 2020, 7:46 AM
Aklapper updated the task description. (Show Details)
Aklapper added a subscriber: Dudewithswag.

Change 599611 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use core .mixin-clearfix() instead of DOM element

https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /599611

nray added a subscriber: nray.Jul 1 2020, 11:30 PM

@Volker_E In reference to I673c28c2d7da4f96c31121d9aec6558e390de24e, I'm thinking that the clearfix is still needed by the content container in non-legacy Vector. Check out the sign up page on beta currently:

https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Special:CreateAccount&returnto=Main+Page&useskinversion=2

Local screenshot for reference:

Nice find @nray !

This seems unexpected.
Can we add the clear fix to the login form styles and continue to take this approach for other interfaces as we discover them?

@Jdlrobson No, that's far too risky with our plethora of interfaces – special pages, gadgets other user generated content.

Change 608976 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] [less] Add clearfix mixin to modern .mw-body-content as well

https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /608976

@Volker_E In reference to I673c28c2d7da4f96c31121d9aec6558e390de24e, I'm thinking that the clearfix is still needed by the content container in non-legacy Vector. Check out the sign up page on beta currently:
https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Special:CreateAccount&returnto=Main+Page&useskinversion=2

For completeness, on that page .mw-ui-container would need a .clearfix, that has its children (#userloginForm, .mw-createacct-benefits-container) floated.
HTML: https://gerrit.wikimedia.org/g/mediawiki/core/+/182c3789f740b650d2844a4e174e4914ba39248a/includes/specialpage/LoginSignupSpecialPage.php#632
Need Less conversion: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/182c3789f740b650d2844a4e174e4914ba39248a/resources/src/mediawiki.special.userlogin.signup.styles/signup.css#9

@Jdlrobson No, that's far too risky with our plethora of interfaces – special pages, gadgets other user generated content.

In what use-case?

@Volker_E In reference to I673c28c2d7da4f96c31121d9aec6558e390de24e, I'm thinking that the clearfix is still needed by the content container in non-legacy Vector. Check out the sign up page on beta currently:

https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Special:CreateAccount&returnto=Main+Page&useskinversion=2

Local screenshot for reference:

Similar issue with Wikidata content: https://wikidata.beta.wmflabs.org/wiki/Lexeme:L5

Jdlrobson added a comment.EditedJul 2 2020, 3:45 PM

@Jdlrobson No, that's far too risky with our plethora of interfaces – special pages, gadgets other user generated content.

I don't agree. The whole point of pulling it out of modern was to fix these interfaces. If not now, when are we going to do it? I don't think that's in the spirit of this project which has been "doing things properly". Do we have patches/tasks to add the clearfix rule to wikidata and login form?

ovasileva triaged this task as High priority.Jul 2 2020, 4:49 PM
Jdlrobson claimed this task.Jul 6 2020, 5:16 PM
ovasileva set the point value for this task to 2.Jul 6 2020, 5:20 PM

what means "give credit", what is the expectation here?

Follow-up to T254195#6266214

As this wasn't answered, to clarify what's the common practice of attribution:

  • A short note in the commit message: "Research and implementation : @Demian"
  • Or a similar short note in a code comment.

The first opportunity was missed despite the clear request in gerrit. I was expecting this is remedied without the need for further requests.

I think we have to assume good faith here as well as note that attributing people in commit messages is not something that often happens in Wikimedia patchsets (particularly for small tasks).

Have you considered Jan might not have looked at your "research"? FWIW I certainly haven’t and the solution to use pseudo elements is very well documented and seemed pretty clear cut and obvious to me.

I believe this task is done as written so I'm moving to sign off.

I think we have to assume good faith here

Obviously it was a mistake, but I clearly and explicitly asked him to do so. Given that I've been repeatedly instructed to answer his questions - before I got to doing so -, I expect my requests would be responded to as well. That did not happen.

the solution to use pseudo elements is very well documented

We all based our mixins on the known pseudo-element solutions. Volker's PS1 from a month ago missed the :before pseudo-element (comment).
My original work, however, was:

  • writing 4 different Less implementations in codepen (comment), of which one was chosen
  • and testing for different use-cases to make sure the code added to core actually works.

Have you considered Jan might not have looked at your "research"?

As the exact solution was copied from my "research", only inverting nocollapse -> collapse, why is this question asked?

Jdlrobson closed this task as Resolved.Jul 8 2020, 9:28 PM

Thanks all. For any follow up work in the other skins (if you care about them) please open tasks but from my perspective this is done.

Esanders reopened this task as Open.Sep 21 2020, 12:13 PM
Esanders added a subscriber: Esanders.

This change was not a no-op for the old skin and caused this regression: T263445

The old DOM node only applied to the main content area, however .mw-body-content can be be used in various places multiple times on the page. I think we should amend to preserve the old behaviour.

Change 628789 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/Vector@master] Follow-up I673c28c2: Only apply clearfix to main content area

https://gerrit.wikimedia.org/r/628789

Jdlrobson removed Jdlrobson as the assignee of this task.Sep 21 2020, 4:29 PM

Change 628789 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Follow-up I673c28c2: Only apply clearfix to main content area

https://gerrit.wikimedia.org/r/628789

visualClear isn't used any more in Vector.

Volker_E removed a subscriber: Dudewithswag.
Jdrewniak closed this task as Resolved.Oct 5 2020, 3:05 PM
Jdrewniak claimed this task.