Page MenuHomePhabricator

Need to restore RTL handling
Closed, ResolvedPublic3 Estimated Story Points

Description

...

  1. You've commented out the code I wrote for rtl wikis, because "CSSJanus should handle it", or something like this, but it doesn't. The GW appears wrong for rtl wikis on ltr language page and for ltr wikis on rtl language page.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 663067 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GlobalWatchlist@master] Restore RTL handling for non-Vue display

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

@DannyS712, you've asked me to review the code, but I'm not a developer on that level. May I call to a real developer that understands a lot in RTL?

@DannyS712, you've asked me to review the code, but I'm not a developer on that level. May I call to a real developer that understands a lot in RTL?

Sure, but I was mostly just cc-ing you - its essentially copied from the user script version

So, you don't need me for +1? If so, there is no need to call him.

So, you don't need me for +1? If so, there is no need to call him.

No harm if you want to review yourself (or have someone else do so) but since this was essentially copied from the user script it is known to work already

I don't. I know it works, it's my code. I just was affraid you can't proceed to the deployment without enough reviews.

I don't. I know it works, it's my code. I just was affraid you can't proceed to the deployment without enough reviews.

+1 is never a bad thing, but only +2 is enough to merge, and only one +2 review is needed

I see. Well, I haven't +2, of course, so I'll just take a look to the code and wait.

Well, @DannyS712, I think the patch will fix the problem of rtl wikis on ltr language page.
But I do not believe it will fix the problem of ltr wikis on rtl language page.
Please open the watchlist with uselang=he. As you can see, all the wikis are aligned to right. It should not happen without the patch. Maybe there is a problem with css selectors on the line 62, thay must align all the text to the left without the patch.

Well, @DannyS712, I think the patch will fix the problem of rtl wikis on ltr language page.
But I do not believe it will fix the problem of ltr wikis on rtl language page.
Please open the watchlist with uselang=he. As you can see, all the wikis are aligned to right. It should not happen without the patch. Maybe there is a problem with css selectors on the line 62, thay must align all the text to the left without the patch.

Can you leave such reviews on the commit?
Also, does this affect the user script version?

I do not know how to do this, never tryed.
The script works fine for both cases, up to now.
Unfortunately I can't check the selectors using F12 tools by myself, probably until Thursday evening.

DannyS712 set Final Story Points to 3.
DannyS712 set the point value for this task to 3.
DannyS712 removed Final Story Points.

Well, @DannyS712, I still have no normal F12 tools, but try to replace the line 62 selector with

.ext-globalwatchlist-feed-site

Well, @DannyS712, I still have no normal F12 tools, but try to replace the line 62 selector with

.ext-globalwatchlist-feed-site

Done in patchset 3

Well, @DannyS712, I still have no normal F12 tools, but try to replace the line 62 selector with

.ext-globalwatchlist-feed-site

Done in patchset 3

Great. Are you going to merge it today on testwiki, do we can check if it works?

Well, @DannyS712, I still have no normal F12 tools, but try to replace the line 62 selector with

.ext-globalwatchlist-feed-site

Done in patchset 3

Great. Are you going to merge it today on testwiki, do we can check if it works?

I can't merge it, since its my patch, but once its merged it can be tested on the beta cluster to confirm whether it works, and if it is we backport to testwiki.

I can't merge it, since its my patch, but once its merged it can be tested on the beta cluster to confirm whether it works, and if it is we backport to testwiki.

If it could be tested on beta cluster, I would find the problem much earlier. We need at least testwiki to get data from both ltr and rtl wikis.

I can't merge it, since its my patch, but once its merged it can be tested on the beta cluster to confirm whether it works, and if it is we backport to testwiki.

If it could be tested on beta cluster, I would find the problem much earlier. We need at least testwiki to get data from both ltr and rtl wikis.

See https://meta.wikimedia.beta.wmflabs.org/wiki/Special:SiteMatrix - there are hebrew and arabic testwiki, so it should be possible to test there

This comment was removed by IKhitron.

See https://meta.wikimedia.beta.wmflabs.org/wiki/Special:SiteMatrix - there are hebrew and arabic testwiki, so it should be possible to test there

Thanks. There is no hebrew wiki, it's just a stub, but I've managed to use the arabic one, and the english one as well. The uselang parameter also works. I've prepared everything, and I can check the result indeed after the merge on beta.

Change 663067 merged by jenkins-bot:
[mediawiki/extensions/GlobalWatchlist@master] Restore RTL handling for non-Vue display

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

See https://meta.wikimedia.beta.wmflabs.org/wiki/Special:SiteMatrix - there are hebrew and arabic testwiki, so it should be possible to test there

Thanks. There is no hebrew wiki, it's just a stub, but I've managed to use the arabic one, and the english one as well. The uselang parameter also works. I've prepared everything, and I can check the result indeed after the merge on beta.

Its merged, should be live on the beta cluster to test (check https://meta.wikimedia.beta.wmflabs.org/wiki/Special:Version under global watchlist what the version is, it'll link to the last merged change that is deployed)

Thanks @DannyS712 and @Legoktm. It was definitely merged, and it's much better now, but not enough.
See how it looks on LTR wikis:


Looks fine, English on the left, Arabic on the right.
See how it looks on RTL wikis:

Looks wrong, English on the right, Arabic on the left - an opposite from needed.

Ack. So I think the CSS solution just isn't going to work, instead I think we should identify whether the remote wiki is LTR or RTL, and then wrap the contents in <div dir="ltr"> or <div dir="rtl"> accordingly.

Now that I typed it, I'm not sure looking at the directionality of the wiki is right, shouldn't we look at the directionality of the user's chosen interface language on that wiki? My interface language is "English", so I would expect my watchlist to be LTR, even on RTL wikis.

Ack. So I think the CSS solution just isn't going to work, instead I think we should identify whether the remote wiki is LTR or RTL, and then wrap the contents in <div dir="ltr"> or <div dir="rtl"> accordingly.

Now that I typed it, I'm not sure looking at the directionality of the wiki is right, shouldn't we look at the directionality of the user's chosen interface language on that wiki? My interface language is "English", so I would expect my watchlist to be LTR, even on RTL wikis.

For LTR interface/uselang it should be LTR, for RTL it should be RTL.
A couple of hours from now I will play with all this, trying to find a working solution.

This comment was removed by Iniquity.
This comment was removed by IKhitron.

Found the problem, trying to solve.

Well, can you try to change the code to

.ext-globalwatchlist-feed-site {
	/* @noflip */ text-align: left;
	/* @noflip */ direction: ltr;
}
/* Handling of rtl wikis */
div[ id^='ext-globalwatchlist-feed-site-he_' ],
div[ id^='ext-globalwatchlist-feed-site-ar_' ],
div[ id^='ext-globalwatchlist-feed-site-azb_' ],
div[ id^='ext-globalwatchlist-feed-site-ckb_' ],
div[ id^='ext-globalwatchlist-feed-site-dv_' ],
div[ id^='ext-globalwatchlist-feed-site-fa_' ],
div[ id^='ext-globalwatchlist-feed-site-glk_' ],
div[ id^='ext-globalwatchlist-feed-site-ks_' ],
div[ id^='ext-globalwatchlist-feed-site-lrc_' ],
div[ id^='ext-globalwatchlist-feed-site-mzn_' ],
div[ id^='ext-globalwatchlist-feed-site-nqo_' ],
div[ id^='ext-globalwatchlist-feed-site-pnb_' ],
div[ id^='ext-globalwatchlist-feed-site-ps_' ],
div[ id^='ext-globalwatchlist-feed-site-sd_' ],
div[ id^='ext-globalwatchlist-feed-site-ug_' ],
div[ id^='ext-globalwatchlist-feed-site-ur_' ],
div[ id^='ext-globalwatchlist-feed-site-yi_' ] {
	/* @noflip */ text-align: right;
	/* @noflip */ direction: rtl;
}

?

Well, can you try to change the code to

.ext-globalwatchlist-feed-site {
	/* @noflip */ text-align: left;
	/* @noflip */ direction: ltr;
}
/* Handling of rtl wikis */
div[ id^='ext-globalwatchlist-feed-site-he_' ],
div[ id^='ext-globalwatchlist-feed-site-ar_' ],
div[ id^='ext-globalwatchlist-feed-site-azb_' ],
div[ id^='ext-globalwatchlist-feed-site-ckb_' ],
div[ id^='ext-globalwatchlist-feed-site-dv_' ],
div[ id^='ext-globalwatchlist-feed-site-fa_' ],
div[ id^='ext-globalwatchlist-feed-site-glk_' ],
div[ id^='ext-globalwatchlist-feed-site-ks_' ],
div[ id^='ext-globalwatchlist-feed-site-lrc_' ],
div[ id^='ext-globalwatchlist-feed-site-mzn_' ],
div[ id^='ext-globalwatchlist-feed-site-nqo_' ],
div[ id^='ext-globalwatchlist-feed-site-pnb_' ],
div[ id^='ext-globalwatchlist-feed-site-ps_' ],
div[ id^='ext-globalwatchlist-feed-site-sd_' ],
div[ id^='ext-globalwatchlist-feed-site-ug_' ],
div[ id^='ext-globalwatchlist-feed-site-ur_' ],
div[ id^='ext-globalwatchlist-feed-site-yi_' ] {
	/* @noflip */ text-align: right;
	/* @noflip */ direction: rtl;
}

?

I didn't think about the interaction with CSSJanus flipping stuff, since that wasn't the case for the user script.

I didn't think about the interaction with CSSJanus flipping stuff, since that wasn't the case for the user script.

So, can you do this?

Change 663393 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GlobalWatchlist@master] Add @noflip commands for CSS

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

Change 663393 merged by jenkins-bot:
[mediawiki/extensions/GlobalWatchlist@master] Add @noflip commands for CSS

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

Change 663397 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GlobalWatchlist@wmf/1.36.0-wmf.30] Restore RTL handling for non-Vue display

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

Change 663399 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GlobalWatchlist@wmf/1.36.0-wmf.30] Add @noflip commands for CSS

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

@DannyS712, I'm here, but where is the jenkins-bot?

@DannyS712, I'm here, but where is the jenkins-bot?

The beta cluster now shows the result with the @noflip comments added - does this look good to you?

Are you sure? The jenkins-bot did not leave a comment this time to say it's already there, and nothing changed visually.

Are you sure? The jenkins-bot did not leave a comment this time to say it's already there, and nothing changed visually.

Its changed visually for me - try https://meta.wikimedia.beta.wmflabs.org/wiki/Special:GlobalWatchlist?uselang=he&debug=true

Maybe I needed a deep refresh once more. It works fine now in all cases.
On LTR interface:


On RTL interface:

In that case, these changes will be backported - thanks for confirming

Change 663397 merged by jenkins-bot:
[mediawiki/extensions/GlobalWatchlist@wmf/1.36.0-wmf.30] Restore RTL handling for non-Vue display

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

Change 663399 merged by jenkins-bot:
[mediawiki/extensions/GlobalWatchlist@wmf/1.36.0-wmf.30] Add @noflip commands for CSS

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

DannyS712 claimed this task.
DannyS712 removed a project: Patch-For-Review.
DannyS712 moved this task from In progress to Done on the MediaWiki-extensions-GlobalWatchlist board.
DannyS712 set Final Story Points to 4.

If I understand properly, I should see the results on testwiki now?

If I understand properly, I should see the results on testwiki now?

Yes - though if your javascript is cached, adding ?debug=true to the url should fix it

Confirm that it works on testwiki.