Page MenuHomePhabricator

Parsoid adds mw-content-ltr / mw-content-rtl class to <body> tag unconditionally whereas core parser doesn't seem to
Closed, DeclinedPublic

Description

Looking at visual diff output differences, the presence of this class on <body> in Parsoid output is a source of major CSS-based diffs in navboxes. With a quick git grep in mediawiki, it looks like this is added to UI messages, some special pages and maybe some skin output. So, maybe Parsoid needs to look at where and when this class is needed rather than add it unconditionally on the body tag.

Event Timeline

ssastry renamed this task from Parsoid adds mw-content-ltr / mw-content-rtl class to <body> tag unconditionally to Parsoid adds mw-content-ltr / mw-content-rtl class to <body> tag unconditionally whereas core parser doesn't seem to.Jul 23 2020, 3:09 PM
ssastry triaged this task as Medium priority.

.mw-content-ltr is added to #mw-content-text not body, but my understanding is that this class was to support older browsers (IE6) which didn't support attribute selectors. I think its redundant now and we should probably move away from it in favor of .mw-content-text [dir="ltr"] and .mw-content-text[dir="rtl"].

@Mooeypoo does this sense or are there good reasons to keep it?

cscott subscribed.

I'm going to reopen this as a child of T264123, instead of closing as a dup, since I think this is a more actionable task than the parent.

There's an interface in Parser.php to specify the wrapper and the wrapper class, but it's fairly new (past 2-3 yrs) so it wouldn't surprise me at all to find that it is being misused in various special pages and UX elements etc. They probably need to be hunted down and the differences fixed one by one.

.mw-content-ltr is added to #mw-content-text not body, but my understanding is that this class was to support older browsers (IE6) which didn't support attribute selectors. I think its redundant now and we should probably move away from it in favor of .mw-content-text [dir="ltr"] and .mw-content-text[dir="rtl"].

@Mooeypoo does this sense or are there good reasons to keep it?

I'd like to add a moment of thought to adding .mw-content-text to the class collection.
We're currently featuring on Vector skin:

  • div.content-container
    • main#content.mw-body
      • Several direct child elements div.mw-body-content, among those div#bodyContent.mw-body-content
        • div#mw-content-text.mw-content-ltr|-rtl

I'm not opposing a change here, but we should, if we touch this, not fortify this naming mess by pursuing with adding a messy class name. if possible.

Further explored in T264173: A better CSS content class naming structure (for Vector)

Possibly related (even though task description is slightly confusing right now T150843: Add CSS class or DOM id for page content

Following up on T258719#6346946

The body tag will have either a rtl or ltr class depending on the /UI/ language
A div element with ID #mw-content-text wraps the parser generated content and has the mw-content- prefixed class

For example on
https://he.wikipedia.org/wiki/%D7%A8%D7%99%D7%99%D7%9B%D7%9C_-_%D7%A4%D7%A1%D7%A0%D7%AA%D7%A8_-_%D7%A9%D7%99%D7%A8%D7%99%D7%9D_(%D7%A1%D7%99%D7%91%D7%95%D7%91_%D7%94%D7%95%D7%A4%D7%A2%D7%95%D7%AA)?uselang=en&safemode=1 while the content is in Hebrew, the site UI is in English. The body tag communicates the UI language, the parser content element dictates the content language.

As far as I understand Parsoid has no notion of site UI, so should not add mw-content-ltr / mw-content-rtl class to <body> tag . It should add it to a div that's a child of the body if it wants to be consistent.

Thanks for the feedback @Jdlrobson. After reading the comments, and based on working with visual diffs, I see what the problem is. Parsoid's output is equivalent to content of <div class='mw-parser-output'>..</div> of the core parser, but we stuff that output in a <body> tag. As part of readviews, where Parsoid's output is now going to be part of some ParserOutput equivalent and go through OutputPage class, this will all get fixed.

For visual-diffing, I anyway strip out all the chrome and pare down to div#bodyContent.

I am going to decline this ticket as a result since there isn't anything to do maybe beyond adding a <div class='mw-parser-output'>..</div> wrapper and completing the readview integration via ParserOutput & OutputPage.

Change 654310 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[integration/visualdiff@master] Pretty much revert f01a5bc

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

Change 654310 merged by Subramanya Sastry:
[integration/visualdiff@master] Pretty much revert f01a5bc

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