Page MenuHomePhabricator

Make ul and ol margin values direction aware
Closed, ResolvedPublic

Assigned To
Authored By
Ebrahim
Jul 7 2024, 10:49 PM
Referenced Files
F57086012: image.png
Aug 5 2024, 9:02 PM
F56460338: image.png
Jul 16 2024, 11:34 AM
F56460335: image.png
Jul 16 2024, 11:34 AM
F56460333: image.png
Jul 16 2024, 11:34 AM
F56460331: image.png
Jul 16 2024, 11:34 AM
F56307507: image.png
Jul 9 2024, 9:28 AM
F56276189: image.png
Jul 7 2024, 10:49 PM
F56276191: image.png
Jul 7 2024, 10:49 PM

Description

Suggested user-notice message: mw-content-ltr, mw-content-rtl and mw-content-{{dir}} are no longer needed as they were deprecated since 2022 as T287701, the primarily remaining use was bullets correct placement which now works without the classes (fixed here). If they are used anywhere without the dir attribute, add the attribute and remove the class as that's already broken with the mobile interface / vector-2022.


Feature summary:
For years we had to carefully use mw-content-ltr and mw-content-rtl subsections to make unordered and ordered list margin values work in LTR and RTL directions as expected. Now with the widespread existence of CSS Logical https://drafts.csswg.org/css-logical/#margin-properties such workarounds aren't needed as now we can define styles in a direction aware way so no longer nested .mw-content-rtl .mw-content-ltr {ul,li} is needed.

Use case(s):
For example render this in an English wiki,

<div dir="rtl">
* a
* b
* c
# 1
# 2
# 3
</div>

<div dir="rtl" class="mw-content-rtl">
* a
* b
* c
# 1
# 2
# 3
</div>

You'll get

image.png (248×616 px, 19 KB)

That's why https://github.com/wikimedia/mediawiki/blob/0791048/resources/src/mediawiki.skinning/i18n-all-lists-margins.less existed so one uses mw-content-rtl that bullets will be put at the right place, but now that direction agnostic CSS definitions exists through CSS Logical we can just go without that mw-content-{ltr,rtl} and have,

image.png (180×588 px, 16 KB)

When this purposed change is applied.

The only problem is existence of older browsers that don't yet support CSS Logical so this change actually keeps compatibility margin values so older browser can benefit from and ignore newer CSS Logical margin-inline-{start,end} counterparts and only remaining issue will be support of nested directions for that older browser which I believe given the benifits for newer browser, the fact margin values will be correct even is unpresence mw-content-{ltr,rtl}, is negligible.

The purposed change uses https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start over https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline which is supported on even older browsers.

Benefits:
So in case there are three or more level of nested direction, this can still work and users no longer have to define mw-content-rtl for their content.

This change is partially motivated by the fact I had to use mw-content-{ltr,rtl} in Commons like this https://commons.wikimedia.org/w/index.php?title=Template:PD-Layout&diff=prev&oldid=101923327 so bullets in copyright tags be displayed in the correct place and this change makes us free from such thing.

I think the first step can be applying https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1052443 and then i18n-all-lists-margins.less then we can even remove i18n-all-lists-margins altogether given it was just a workaround as the lack of direction agnostic CSS.

Event Timeline

Change #1052443 had a related patch set uploaded (by Ebrahim; author: Ebrahim):

[mediawiki/core@master] Make ul and ol margin values direction agnostic

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

Ebrahim updated the task description. (Show Details)
Ebrahim updated the task description. (Show Details)

Assuming this is about the MediaWiki core software, thus adding project so this task could be found.

Ebrahim renamed this task from Make ul and ol margin values direction agnostic to Make ul and ol margin values direction aware.Jul 8 2024, 2:04 PM
Ebrahim updated the task description. (Show Details)
TheDJ subscribed.

Tagging web team for visibility. I personally think this is worth it (with fallback margins). I'd actually would very much like to use more of the logical properties, but 97% is I think just a tad too large a group of users to rely on it solely without fallback to commit to it. Especially the fact that Safari only started fully supporting in Safari 15, is problematic, as we have plenty of users with 6 - 7 year old iPads and iPhones.

In this case, there are clear benefits and there are fallback properties.

[..] Especially the fact that Safari only started fully supporting in Safari 15, is problematic, [..]

I understand you noted it generally but I just like to add Safari's support of the specific needed feature here is from Safari 12.2 https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start it seems

[..] In this case, there are clear benefits and there are fallback properties.

Thank you for the support, this is very much helpful for multilanguage wikis like Commons as you already have noted, I had to make sure all Commons templates have such a thing https://commons.wikimedia.org/w/index.php?title=Template:PD-Layout&diff=prev&oldid=101923327 for a long time

Change #1052786 had a related patch set uploaded (by Ebrahim; author: Ebrahim):

[mediawiki/libs/node-cssjanus@master] test: Make sure CSS Logical aren't touched

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

Change #1052786 merged by jenkins-bot:

[mediawiki/libs/node-cssjanus@master] test: Make sure CSS Logical won't be touched

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

Just to note about the change above, I wanted to add a guarantee to CSS Janus that it won't ever touch CSS Logical properties and values and it turned out php-cssjanus which is in use in MediaWiki fetches the tests from node-cssjanus https://github.com/wikimedia/node-cssjanus/blob/master/test/data.json so went to add the tests there first in node-cssjanus, and it went well but the moment I wanted to update the tests in php-cssjanus it turned out node-cssjanus and php-cssjanus have become out of sync since T333276 but CSS Logical related tests already pass unlike calc related tests so hopefully when there will be any major development in php-cssjanus it first can get synced again with node-cssjanus so to have the tests in.

Here is an example of nested more than two layers direction which isn't handled by the current approach,

https://commons.wikimedia.org/wiki/File:Abilene_Daily_Reporter_(Abilene,_Tex.),_Vol._14,_No._280,_Ed._1_Sunday,_June_19,_1910_-_DPLA_-_608547881f115ccfa0e75e56cc36b98c_(page_14).jpg?uselang=fa

That untranslated DPLA tag has bullets with incorrect spacing, it originally didn't even had mw-content-ltr and I just added it, regardless, bullets aren't in the correct place which this change resolves it.

image.png (642×308 px, 51 KB)

Alternatively we can just remove all spacing styles related to UL and OL and let the browsers (useragent stylesheet) https://stackoverflow.com/a/55215684 handle the spacings which they do have direction aware values, that however is more risky I believe but something that can be considered afterward.

Jdlrobson subscribed.

Thanks for the ping. Retagging with CSS Janus which is not maintained by web team and I presume a release will be needed?

Sorry it's late in my day - I now have a bit more context - seems CSS Jansu change is only part of it. Moving back to incoming for further analysis.

Interestingly we already have a good number of {margin,padding}-{block,inline} uses even without fallback mostly in ContentTranslation extension,

image.png (610×180 px, 18 KB)

image.png (590×168 px, 16 KB)

image.png (568×160 px, 18 KB)

image.png (576×168 px, 18 KB)

Change #1052443 merged by jenkins-bot:

[mediawiki/core@master] Make dd, ol and ul margin values direction aware

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

Jdlrobson claimed this task.
Jdlrobson triaged this task as Medium priority.
Ebrahim updated the task description. (Show Details)
Ebrahim updated the task description. (Show Details)
Ebrahim updated the task description. (Show Details)
Ebrahim updated the task description. (Show Details)
Quiddity subscribed.

@Ebrahim Thanks for the suggested Tech News entry! I was uncertain about some details, so I talked to a couple of staff developers, and they suggest that this might not need a Tech News announcement, at least not this week. Therefore I'll place it in the "not ready" column on the workboard, for now.
If it does become necessary/highly-useful in the future, then it might be best to create a wikipage with more details and some examples of the exact changes needed, so that we don't have to squish too much content into the Tech News entry itself. (E.g. Perhaps a page similar to https://www.mediawiki.org/wiki/Heading_HTML_changes or https://www.mediawiki.org/wiki/Recommendations_for_night_mode_compatibility_on_Wikimedia_wikis might be good, or a section (or subpage) of https://www.mediawiki.org/wiki/Directionality_support).
I hope that makes sense, and is ok.
Thanks again!

@Quiddity It feels a bit weird to me that all that Tech News pushed on different wiki are accompanied with mw-content-ltr like this message https://fa.wikipedia.org/?diff=39968093 (please search for mw-content-ltr on that diff) just because of the bullets correct placement yet it isn't noteworthy to mention that somewhere on the same Tech News that the classes are deprecated since 2022 and now absolutely not needed (as mentioned on the first paragraph of the task description Suggested user-notice message part) but it's ok, understandable different priorities, I have to mention that on Wikimedia Commons so that template editors know that's not needed anymore, similar to {{#dir}} addition that's already mentioned on next tech news. I hope however the next Tech News at least won't use the class.

@Jdlrobson This task is in the Verify 1.43.0-wmf.16 deployment. Is this QA testable? If so, can you please provide QA steps? Thanks!

No need to verify this one. We've been using https://pixel.wmcloud.org/ for this purpose.

@Quiddity The new version of tech news still has that now unnecessary mw-content-ltr tag,

image.png (1,194×550 px, 82 KB)

I've removed that in Wikimedia Commons and Meta in templates and Lua modules the far I could, is it possible to do that in the next version of Tech News also? (see also T287701) Thanks

@Ebrahim Hmm, it's not in the source at https://meta.wikimedia.org/wiki/Tech/News/2024/32 (as just delivered) nor is it in the source of this un-translated-but-page-created Central Kurdish page (https://meta.wikimedia.org/wiki/Tech/News/2024/32/ckb?action=edit) so I'd guess it's being added by the MassMessage extension during the delivery process? Hope that helps.

Change #1059965 had a related patch set uploaded (by Ebrahim; author: Ebrahim):

[mediawiki/extensions/MassMessage@master] Don't use mw-content- classes

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

@Quiddity Thank you for the help and giving a direction, uploaded a patch to fix it.

Change #1059965 merged by jenkins-bot:

[mediawiki/extensions/MassMessage@master] Don't use mw-content-{ltr,rtl} classes

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

Jdlrobson reopened this task as Open.EditedSep 13 2024, 12:28 AM
Jdlrobson added a subscriber: Krinkle.

@Krinkle pointed out that this wasn't deprecated correctly in that the styles are being removed for skins that do not use elements so reopening that and marking as blocker for 1.43 release to make sure we get that cleaned up.

@Jdlrobson I'm very sorry for the trouble here, please let me know if anything needs to be done. Hopefully we won't need to go that way but as a last resort perhaps we can just revive styles of the module for this release and remove them on the next release or anything you think is necessary. Thanks

@Jdlrobson I'm very sorry for the trouble here, please let me know if anything needs to be done. Hopefully we don't need to go that way but as a last resort perhaps we can just revive the styles the module had for this release and remove them on the next release or anything you think is necessary. Thanks

No trouble at all - this was my fault during review not yours :-).
We just need to do the following:

/*** To remove deprecation notice please move these styles to your skin or include the elements feature */
ul {
	margin-inline-start: 1.6em;
	margin-inline-end: 0;
}

ol {
	margin-inline-start: 3.2em;
	margin-inline-end: 0;
}

dd {
	margin-left: 1.6em;
	margin-right: 0;
	margin-inline-start: 1.6em;
	margin-inline-end: 0;
	margin-bottom: 0.1em;
}

That way if skins are including i18n-all-lists-margins they don't lose any styling and then can then follow the deprecation message instructions to remove it.

Change #1072809 had a related patch set uploaded (by Ebrahim; author: Ebrahim):

[mediawiki/core@master] Revive functionality of i18n-all-lists-margins

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

Change #1072809 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Revive i18n-all-lists-margins feature of SkinModule

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

Thanks @Ebrahim for the follow up here and sorry again I didn't catch it earlier!