Page MenuHomePhabricator

Personal links and tabs are reversed in the Vector skin
Closed, ResolvedPublic

Description

The personal links and the page tabs (Page, Talk etc.) are reversed in the Vector skin when the UI language is RTL. This is a recent regression. It is already deployed in Commons. Example: https://commons.wikimedia.org/wiki/Main_Page?uselang=he .


Version: unspecified
Severity: blocker
See Also:
T48947: Vector: Horizontal nav elements should be flipped with CSS instead of in HTML
T56673: LESS compiler should preserve the position of CSSMin / CSSJanus annotations

Details

Reference
bz55779

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 2:39 AM
bzimport set Reference to bz55779.
Amire80 created this task.Oct 16 2013, 9:26 AM

Related (and probably the root cause for this mess): bug 46947.

Adding release manager and person doing most deployments. LangEng advises to NOT deploy further.

Can you please confirm that https://gerrit.wikimedia.org/r/#/c/85920/ is indeed causing the issue? One could test out by checking out the previous commit (08cf0b1395^) look at the page, then check the commit ( 08cf0b1395 ) and look at the page.

Screenshots of each steps highlighting the issue would be nice as well. Most people are confused with expected rendering on RTL wiki (including me).

You can look at attachments to bug 55629 (the first and the second one).

Verified, 08cf0b1395 is indeed the culprit.

Created attachment 13503
Incorrect rendering as of 08cf0b1395 (lang=he)

Attached:

Created attachment 13504
Correct rendering as of 08cf0b1395^ (lang=he)

Attached:

I suspect this is because of how lessphp handles comments such as /* @noflip */ when they come before the selector. All comments outside the braces get moved to before all the CSS rules.

Change 90133 had a related patch set uploaded by Jdlrobson:
Move noflip annotations into rules themselves

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

Can someone verify the above patch has the desired effect? It was indeed the noflip annotations. I've moved them next to the rules they flip...

Bug 54673, which is the second underlying issue here ;) ("LESS compiler should preserve the position of CSSMin / CSSJanus annotations") has been reopened.

Change 90133 merged by jenkins-bot:
Move noflip annotations into rules themselves

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

Worked around.

Also, while the fix is a workaround, I think it's also a genuine improvement of the code in general (explicit is better than implicit), so personally I don't feel bad about merging it. :)

Change 90169 had a related patch set uploaded by Bartosz Dziewoński:
Move noflip annotations into rules themselves

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

Change 90169 merged by jenkins-bot:
Move noflip annotations into rules themselves

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

To summarize:

This works correctly, if there is only one instance of each property in the block (a property might be repeated several times for browser hacks, see bug 61941):

.class {
    /* @noflip */
    margin-left: 1em;
    /* @noflip */
    margin-right: 2em;
}

This doesn't generally work, although sometimes it does due to pure chance:

/* @noflip */
.class {
    margin-left: 1em;
    margin-right: 2em;
}

(I meant to post this on bug 54673, sorry.)

Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptMay 3 2017, 10:39 PM