Pinging the last three persons who have worked on the extension @thiemowmde @Umherirrender @kaldari.

[…] this should be handled through a LESS file […]

I'm not sure if this is better. It looks like we would need to generate this file for each and every poem individually. At the moment I can't imagine how we would do this without parsing all pages with poems twice.

the directionality of the indentation should be context specific […]

Indeed. This sounds very much like a bug to me. Unfortunately, the title of this ticket fails to mention this. Instead of describing the problem it proposes a solution. I strongly suggest to change this tickets title, or open a separate one for the RTL support.

In T254051#6179991, @thiemowmde wrote:

[…] this should be handled through a LESS file […]

I'm not sure if this is better. It looks like we would need to generate this file for each and every poem individually. At the moment I can't imagine how we would do this without parsing all pages with poems twice.

I only suggested LESS because it allows you to include rules that would understand directionality of page (just like how MW core handles directional indentations).

Anyway, instead of hard-coding the margin, could we use <dl> and <dd> elements (just like MediaWiki core does for the : indentations, and either let these element use the MW core indentation style (which is 1.6 em, I believe, at least in the Vector skin) or override it to 1 em so it matches current behavior?

the directionality of the indentation should be context specific […]

Indeed. This sounds very much like a bug to me. Unfortunately, the title of this ticket fails to mention this. Instead of describing the problem it proposes a solution. I strongly suggest to change this tickets title, or open a separate one for the RTL support.

The title mentions two problems: (1) the CSS is hard-coded [which is generally bad practice]; (2) the CSS it no context-specific

You could argue these can be broken into two tasks; I would be fine with that. I only reported them together because to fix #2, you would need to fix #1 and in my head, you want to submit one patch that fixes both. But like I said, you should feel free to split it to two tasks if you like.

I only suggested LESS because it allows you to include rules that would understand directionality […]

As far as I know this is not a property of LESS, but something we wo with the CSSJanus library.

could we use <dl> and <dd> elements […]

Poems are not definition lists and should not be marked as such, in my opinion. I think the current styling with meaningless <span> elements is absolutely fine. Unfortunately this ticket fails to explain what issue this creates. As I said: I suggest to either change this ticket to talk about the RTL issue only, or open a separate ticket for the RTL issue.

The title mentions two problems: (1) the CSS is hard-coded […]

So what? What's the real-world issue with this?

(2) the CSS it no context-specific

I don't find the word "context" helpful here. This could mean anything. As I said: I suggest to create a ticket that talks only about the "missing RTL support". At the moment, any : indention inside a <poem> tag is on the wrong side when the text is right-to-left. That's a bug for sure. There are probably a dozen ways to fix this. Phabricator tickets should never sound like they demand a specific solution, unless you or your team plan to implement exactly that.

In T254051#6214924, @thiemowmde wrote:

Poems are not definition lists and should not be marked as such, in my opinion. I think the current styling with meaningless <span> elements is absolutely fine. Unfortunately this ticket fails to explain what issue this creates. As I said: I suggest to either change this ticket to talk about the RTL issue only, or open a separate ticket for the RTL issue.

I agree with your point about <span> elements being a better idea. Regarding the rest of your sentence, this issue is about that RTL issue. More accurately: about the internationalization issue. If you feel like the Task description is unclear, please feel free to revise it.

The title mentions two problems: (1) the CSS is hard-coded […]

So what? What's the real-world issue with this?

There are important reasons why we avoid inline style tags and use CSS. For one: overriding it would require !important in the CSS, which should to be avoided. (If you wonder why someone may want to override it: think of a wiki that prefers the indentation to be in 20pt increments, as opposed to the default). Here are some other reasons.

But, I don't think we should debate this point any further. It is clear that the RTL issue mentioned in this task cannot be resolved while using inline style attributes.

(2) the CSS it no context-specific

I don't find the word "context" helpful here. This could mean anything. As I said: I suggest to create a ticket that talks only about the "missing RTL support". At the moment, any : indention inside a <poem> tag is on the wrong side when the text is right-to-left. That's a bug for sure. There are probably a dozen ways to fix this. Phabricator tickets should never sound like they demand a specific solution, unless you or your team plan to implement exactly that.

By context, I mean the "directionality context of the parent element in which the poem is situated". Here are a few examples:

• On an RTL wiki (e.g. Persian Wikipedia), the indentations of the poem lines should be on the right, but they are not.
• On an LTR wiki (e.g. English Wikipedia), one should be able to include some Persian poem on the page (e.g. using <div class="mw-content-rtl"><poem>...</poem></div>) and the indentation for that section of the page should be on the right, but this also currently is not possible.
• On an RTL wiki (e.g. Persian Wikipedia), one should be able to include some English peom with indentation on the left (e.g. using <div class="mw-content-ltr"><poem>...</poem></div>). This currently would work, but that is just a side-effect of using inline style attributes on the <span> elements.

Those are three different contexts.

I had a very helpful discussion with @Krinkle on IRC. My original thought was to move the style into a standalone CSS file; however, he educated me that it would be yet another entry point for ResourceLoader and we add such entry points sparingly. Therefore, I abandoned my preliminary work on https://gerrit.wikimedia.org/r/604803/

Given that we should stick with inline style attributes, there are at least two options remaining. Option 1 is my favorite, and for it I am submitting a patch shortly.

• Option 1: instead of using margin-left in the style, use margin-inline-start. This is a newer CSS property that is "directionality-aware", i.e. it will add the margin on the left if the element is LTR, and on the right if it is RTL.
• Option 2: some MW core features (e.g. <pre> tags) are directionality aware; apparently, they provided with the directionality of their parent element by the Parser. I am not sure how (haven't found the relevant piece of code).

The advantage of option 1 is it can be easily implemented. The disadvantage is that we will be using an experimental CSS feature that is not supported by some older browsers (including all versions of IE, but not Edge).

The advantage of option 2 is that it will work for every browser. The disadvantage is that we will need to find the MW core mechanism and make it exposed to extensions (which may be quite a bit of work).

Change 604824 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/Poem@master] Intendtation should be based on content directionality

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

this issue is about that RTL issue.

Before, it wasn't. Thanks for updating the title.

It is clear that the RTL issue mentioned in this task cannot be resolved while using inline style attributes.

How is that "clear"?

I played around with the Poem code a bit. It's possible to use $parser->getTargetLanguage()->getDir(). This does, at least, give us the language direction of the page. But it does not understand when, for example, a <poem> tag is wrapped in something like <div dir="rtl"> or marked as <poem dir="rtl">.

margin-inline-start is a wonderful solution. It seems Internet Explorer is the only browser that blocks us from solely relying on this nice property.

What about combining both solutions?

$dir = $parser->getTargetLanguage()->getDir();
echo '<span style="margin-' . ( $dir === 'rtl' ? 'right' : 'left' ) . ': 1em; margin-inline-start: 1em;">';

This makes indentions in <poem> tags work on RTL pages (e.g. in the fa wiki) in all browsers including IE. It still doesn't work in IE when using, for example <poem dir="rtl">. But this was always broken. If we can fix the vast majority of context–browser combinations, why not? This might be the best we can do at the moment.

When both margins fall on the same side (which should be the case in the majority of situations), they basically cancel each other out (1em + 1em is not 2em, but still 1em).

When the margin with the hard-coded "right" or "left" falls on the wrong side, it typically does not do anything visually. It might still make a visual difference when the line is very long and reaches the other side of the container. But this might be acceptable. At least as long as we need to support IE.

@thiemowmde The issue is that indentVerse is called as a callback for preg_replace_callback and the syntax we use right now doesn't allow passing an additional parameter to it (may it be the parser or the directionality).

There are two options; one is to define a static property for the class, initialize it in init and use it in indentVerse(). The other is to rewrite the callback syntax to use an anonymous function so we can pass $parser to indentVerse() as a second parameter. Of these two approaches, the static property approach seems to be cleaner so I will use that.

Change 604824 merged by jenkins-bot:
[mediawiki/extensions/Poem@master] Indentation should be based on content directionality

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

Change 607881 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Sync poemParserTests with core

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

Change 607887 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Match upstream change for margin-inline-start

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

Change 607881 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Sync poemParserTests with core

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

Change 607887 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Match upstream change for margin-inline-start

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

Change 608437 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/vendor@master] Bump Parsoid to v0.12.0-a19

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/ /608437

Change 608437 merged by jenkins-bot:
[mediawiki/vendor@master] Bump Parsoid to v0.12.0-a19

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/ /608437