Vector: Horizontal nav elements should be flipped with CSS instead of in HTML
Closed, ResolvedPublic

Description

Vector first reverses the order of horizontal nav elements in HTML and then prevents flipping of float:left rules in CSS to make the interface looks the same in LTR and RTL environments.

Similar craziness is done to the search form - the input and the button are rendered in different order in LTR and RTL.

This causes T36587 and led to issues like T57779 in the past.

VectorTemplate.php
// Reverse horizontally rendered navigation elements
if ( $this->data['rtl'] ) {
  $this->data['view_urls'] =
    array_reverse( $this->data['view_urls'] );
  $this->data['namespace_urls'] =
    array_reverse( $this->data['namespace_urls'] );
  $this->data['personal_urls'] =
    array_reverse( $this->data['personal_urls'] );
}
...
  // If there's a series of elements, reverse them when in RTL mode
} elseif ( $this->data['rtl'] ) {
  $elements = array_reverse( $elements );
}
... etc
components/tabs.less
div.vectorTabs {
  /* @noflip */
  float: left;
  ...
  ul {
    /* @noflip */
    float: left;
    ...
    li {
      /* @noflip */
      float: left;
... etc

Why would anyone do that is just mind-boggling to me. I assume it's a workaround for some IE6 bugs.

Details

Reference
bz46947
There are a very large number of changes, so older changes are hidden. Show Older Changes
bzimport raised the priority of this task from to Normal.Nov 22 2014, 1:17 AM
bzimport set Reference to bz46947.
bzimport added a subscriber: Unknown Object (MLST).
matmarex created this task.Apr 6 2013, 1:34 PM

You should be careful to check, but this should be something we can get rid of now because it was actually an IE 5.5 workaround, and we no longer need to provide support for that browser since it's below the 0.1% market share threshold (currently at 0.05%) according to http://stats.wikimedia.org/wikimedia/squids/SquidReportClients.htm

Monobook in IE 5.5 would simply display the tabs in the wrong order - but I wasn't willing to accept that for Vector, so I ended up with this hack. Let's verify it's ok to remove for IE 6, and if all looks clear, get rid of this.

Change 82100 had a related patch set uploaded by Matmarex:
Rewrite rendering of Vector simple search

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

(The patch above only pokes with the search, not with the navigation.)

nzmoihue wrote:

This is definitely correct, important and needed. After this is fixed I should revert this https://commons.wikimedia.org/w/index.php?title=MediaWiki:Gadget-MyUploads.js&diff=101977822&oldid=101398730 so please fix it soon

Change 82100 merged by jenkins-bot:
Rewrite rendering of Vector simple search

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

Isarra moved this task from Backlog to Implementation changes on the Vector board.Apr 7 2016, 1:50 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 7 2016, 1:50 AM
Krinkle removed a subscriber: Krinkle.Apr 8 2016, 12:03 AM

This seems to be fixed, please can you help confirm @Mooeypoo?

Jdlrobson lowered the priority of this task from Normal to Low.Jul 21 2016, 8:34 PM
Jdlrobson added a project: Technical-Debt.
Jdlrobson moved this task from To Triage to Triaged but Future on the Readers-Web-Backlog board.

@matmarex can you pleas say me the difference between rtl and ltr environments and I think the code "float: left " just moves the elements in the respective navigation bar to left but array_reverse() actually converts the order of the elements in the array and prints them in the navigation bar and which flipping mentioned means?

Here are examples of wikis:

I'm sorry, I don't really understand the rest of your question.

thankyou @matmarex for saying the difference the question I asked is float property of the css is defined for the entire ul of the nav elements so that float left mean that they appear on the left side instead of any other place on the screen and also array_reverse() is actually flipping the elements in the nav bar so that they print in the reverse order( i.e flipped) so the actual flipping is done by html code ?

@Rammanojpotla: To avoid misunderstandings, I'd recommend to break your comments / questions into shorter sentences. And elaborate where exactly (file names) you find what exactly (element names). :)

Yes. In this case, the flipping is done in the HTML code. But normally, the HTML would be identical for left-to-right and right-to-left languages, and instead the float: left would automatically be changed into float: right in the CSS code when the user is viewing the page in a right-to-left language. See https://www.mediawiki.org/wiki/ResourceLoader/Features#Flipping.

Change 368759 had a related patch set uploaded (by Rammanojpotla; owner: Rammanoj):
[mediawiki/skins/Vector@master] Vector: Horizontal nav elements should be flipped with CSS instead of in HTML

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

@matmarex I have submitted a patch basically removed /*noflip*/ in css and html code which causes the flipping of the nav elements in rtl mode can you review it once?

It doesn't seem to work correctly – the patch causes several layout changes:

Before
After
Rammanojpotla added a comment.EditedJul 31 2017, 4:44 PM

can you say me how to test the rtl model in localhost I dont have it in my localhost so that I could test it next time and I think there is some problem with float property of css is not working and I have removed the html code so there is no flipping in it

Just change your user language in preferences to e.g. Hebrew (he) or Arabic (ar).

@Rammanojpotla You can also use uselang to switch the language in that one view. Something like https://localhost:8080/?uselang=he.

I think I have made the change it looked the fallowing way in my localhost

TheDJ added a subscriber: TheDJ.Aug 1 2017, 2:55 PM

@matmarex we might also need staged rollout of this due to CSS caching perhaps ? or is the cache time so small these days that such is no longer required ?

Change 368759 abandoned by Rammanojpotla:
Vector: Horizontal nav elements should be flipped with CSS instead of in HTML

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

Change 379577 had a related patch set uploaded (by Rammanojpotla; owner: Rammanoj):
[mediawiki/skins/Vector@master] Vector: Horizontal nav elements should be flipped with CSS instead of in HTML

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

This is a image of it in my localhost

Change 379577 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Vector: Flip horizontal nav elements with CSS

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

matmarex added a subscriber: greg.Sep 28 2017, 1:48 AM

@matmarex we might also need staged rollout of this due to CSS caching perhaps ? or is the cache time so small these days that such is no longer required ?

Yes, we should have done that.

To clarify, due to our configuration, on Wikimedia wikis page views by anonymous users might be served with the latest CSS (which is cached for ~5 minutes) but outdated HTML (which can be cached for a week or so, I believe; it used to be around 30 days). This will cause the page tabs and personal links to be shown in reverse order. (Page HTML for logged in users is never cached, only anonymous users are affected.)

For example, this is how Hebrew Wikipedia's main page (https://he.wikipedia.org/wiki/עמוד_ראשי) looks for me right now when logged in and anonymous:

Logged inAnonymous

Looks at these areas specifically to notice they are flipped:

The anonymous view's HTML is cached (if I scroll further down, the "Today in history" section has a date of 27 September, while in the logged in view it's already shown as 28 September).

To avoid this issue, the new CSS would have to be compatible with both the old and new HTML. Essentially you'd need to add an extra class="foo" in the page HTML somewhere, then duplicate all the CSS for the navigation to have a separate version for .vectorTabs (old HTML) and .foo .vectorTabs (new HTML) etc.


However, at this point the milk is already spilt, at least for the projects which get the new MediaWiki version early (Wednesdays instead of Thursdays), like Hebrew Wikipedia and all non-Wikipedia projects. Their HTML cache now contains pages rendered by both old and new software and it is probably impossible to clean up this mess without purging the entire cache (which might also be impossible). Our RTL readers will have to live with reversed tabs for a couple of days, I'm afraid.

@greg We might want to halt the 1.31.0-wmf.1 deployment so that remaining RTL wikis avoid the issue, and improve the CSS as I outlined above before we resume it. However if purging the HTML cache (for all RTL wikis) is feasible, we should do that instead. Can you have someone from Operations figure out if they can do that? (In fact, do we even have a convenient list of all RTL wikis?)

@Rammanojpotla Please don't feel bad about this, I caused a very similar issue a few years back with one of my own first patches to MediaWiki, except mine resulted in far more noticeable problems and they affected all wikis ;) (T44452)

Rammanojpotla added a comment.EditedSep 28 2017, 1:56 AM

@matmarex does the patch sent by me cause any issue? And I think patch is already merged then this issue can be closed right?

@Rammanojpotla Yes, but don't worry, we'll take care of it and it's not a very large problem anyway. Bugs happen, and it isn't your fault, we should have caught this when reviewing your patch.

@matmarex does this issue gets closed with this patch or will it be unclosed as some of other issues?

I'll close it when we fix the problem I described.

Change 381153 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@master] Fix reversed nav elements when viewing cached HTML

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

Change 381161 had a related patch set uploaded (by Tim Starling; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@wmf/1.31.0-wmf.1] [1.31.0-wmf.1] Fix reversed nav elements when viewing cached HTML

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

Change 381153 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Fix reversed nav elements when viewing cached HTML

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

Change 381161 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.31.0-wmf.1] [1.31.0-wmf.1] Fix reversed nav elements when viewing cached HTML

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

Tim reviewed, backported and deployed the patch.

@BBlack I've been told you're the person to ask about purging HTML caches. We would want to purge all cached entries that were generated between 2017-09-27 19:00 UTC (deployment of 1.31.0-wmf.1 to group1) and 2017-09-28 07:20 UTC (deployment of the fix) on the following wikis:

arwikibooks
arwikimedia
arwikinews
arwikiquote
arwikisource
arwikiversity
arwiktionary
dvwiktionary
fawikibooks
fawikinews
fawikiquote
fawikisource
fawikivoyage
fawiktionary
hewiki
hewikibooks
hewikinews
hewikiquote
hewikisource
hewikivoyage
hewiktionary
pnbwiktionary
pswikibooks
pswiktionary
sdwikinews
sdwiktionary
urwikibooks
urwikiquote
urwiktionary
yiwikisource
yiwiktionary

These are all wikis currently on 1.31.0-wmf.1 (from [operations/mediawiki-config]/wikiversions.json) that are using an RTL language (from $rtl variable in MessagesXx.php files in MediaWiki). hewiki is probably the largest of those.

Is this easily doable?

@BBlack I've been told you're the person to ask about purging HTML caches.

purgeList.php has a --all option, which purges all pages. However, it works by querying for every page title.

The batch size can be increased, but even so, it can probably be done much faster (and perhaps more targeted) at the Varnish level.

We can probably do something like that, but it's not generally simple. The meaning of "generated" might need clarification. Generated as in "Varnish fetched it from MW", or generated as in "When the parsercache entry was created"?

On the other hand, since none of the listed wikis are very high on the popularity list, it might be simpler if we just did a one-shot purge of all content for them regardless of date.

greg added a comment.Sep 29 2017, 5:41 PM

@BBlack: up to you I'd say.

We can probably do something like that, but it's not generally simple. The meaning of "generated" might need clarification. Generated as in "Varnish fetched it from MW", or generated as in "When the parsercache entry was created"?

As in "Varnish fetched it from MW". The parser cache is okay, the problematic piece of HTML is outside of the page contents and a part of the skin.

matmarex closed this task as Resolved.Oct 3 2017, 5:05 PM
matmarex removed a project: Patch-For-Review.

Since it doesn't seem like we're going to be purging any caches (it's in fact probably not worth the effort, I don't think there have been any complains about the issue), and all of the code patches are merged, let's close this again.

Change 382187 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@master] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Change 382187 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Change 382201 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@wmf/1.31.0-wmf.1] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Change 382202 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@wmf/1.31.0-wmf.2] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Change 382201 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.31.0-wmf.1] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Change 382202 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.31.0-wmf.2] Do not special-case ULS and "Not logged in" in RTL in personal bar

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

Mentioned in SAL (#wikimedia-operations) [2017-10-04T18:22:04Z] <niharika29@tin> Synchronized php-1.31.0-wmf.1/skins/Vector/: Do not special-case ULS and Not logged in in RTL in personal bar T48947, T177312 (duration: 00m 51s)

Mentioned in SAL (#wikimedia-operations) [2017-10-04T18:23:13Z] <niharika29@tin> Synchronized php-1.31.0-wmf.2/skins/Vector/: Do not special-case ULS and Not logged in in RTL in personal bar T48947, T177312 (duration: 00m 50s)