Page MenuHomePhabricator

Support diffs copy and paste for RTL languages
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT

Description

What is the problem?

I am struggling to get some of the changes made for https://phabricator.wikimedia.org/tag/mediawiki-page-diffs/ working on an RTL wiki.

For example, trying to select text from one side of the diff actually selects it from the other (the feature introduced in T285793).

I also notice that diff-right and diff-left attributes (from T285776) are not flipped in RTL.

I have tried setting the user's interface language and $wgLanguageCode to ar.

This is on my local bare-metal machine with wikidiff2 1.12.0 installed (compiled locally).

Environment

Wiki(s): local wiki MediaWiki 1.37.0-alpha (930841b) 15:14, 1 September 2021 with wikidiff 1.12.0.

Event Timeline

Thanks @dom_walden!

For example, trying to select text from one side of the diff actually selects it from the other (the feature introduced in T285793).

This is bad, and I'm working on a fix. I think it should be possible to fix this in mw core alone, without touching wikidiff2.

I also notice that diff-right and diff-left attributes (from T285776) are not flipped in RTL.

I think this is just a naming issue -- "left" here just stands for "deleted", "right" for "added". The classes could be renamed without any issue (perhaps "diff-side-added" and "diff-side-deleted"). This requires a change in both wikidiff2 and mw core.

I have tried setting the user's interface language and $wgLanguageCode to ar.
This is on my local bare-metal machine with wikidiff2 1.12.0 installed (compiled locally).

FTR, I can reproduce the same issue with the "vanilla" diff formatter (i.e. without wikidiff2), using ?uselang=ar.

Amire80 renamed this task from Support for RTL languages to Support diffs copy and paste for RTL languages.Sep 16 2021, 1:33 PM
Daimona set the point value for this task to 2.Sep 16 2021, 1:36 PM

So... The diff code itself would work just fine on RTL wikis, since as I said above, "left" and "right" are just a (admittedly subpar) naming scheme, but don't actually expect things to be on the left/right. The real issue is that MW uses cssjanus to adapt stylesheets to RTL. The following style (which is the core of the diff-locking thing):

.diff[ data-selected-side='left' ] .diff-right,
.diff[ data-selected-side='right' ] .diff-left {
	-webkit-user-select: none;
	-moz-user-select: none;
	-ms-user-select: none;
	user-select: none;
	cursor: text;
}

is flipped by cssjanus in the following way:

/* Lock selection to a single side */
.diff[ data-selected-side='right' ] .diff-right,
.diff[ data-selected-side='left' ] .diff-left {
	-webkit-user-select: none;
	-moz-user-select: none;
	-ms-user-select: none;
	user-select: none;
	cursor: text;
}

i.e. the value of the "data-selected-side" attribute is switched, but not the diff-right/diff-left classes, hence making text impossible to select.

Renaming the classes and the attribute will fix everything.

Change 721524 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Rename diff-related classes for LTR compatibility

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

Change 721547 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/php/wikidiff2@master] Rename diff-related classes for LTR compatibility

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

Change 721531 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@REL1_37] Rename diff-related classes for LTR compatibility

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

Change 721524 merged by jenkins-bot:

[mediawiki/core@master] Rename diff-related classes for LTR compatibility

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

Change 721531 merged by jenkins-bot:

[mediawiki/core@REL1_37] Rename diff-related classes for LTR compatibility

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

Change 721547 merged by jenkins-bot:

[mediawiki/php/wikidiff2@master] Rename diff-related classes for LTR compatibility

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

Whoops, seems like I forgot to move this to QA.

Is this going to get signed off as done? It's tagged against the 1.37 release (which should have shipped by now), so if we're going to tell people to use a new version of wikidiff2 we're going to want to move very sharply.

Is this going to get signed off as done? It's tagged against the 1.37 release (which should have shipped by now), so if we're going to tell people to use a new version of wikidiff2 we're going to want to move very sharply.

This is actually only for the built-in PHP diff formatter (TableDiffFormatter.php), which made it impossible to select anything on diffs when the interface is in an RTL language.

CC @NRodriguez ^^

I have tested this on RTL wikis (arwiki, hewiki, fawiki) on Firefox and Chrome.

The new wikidiff2 features work the same in RTL wikis as in LTR wikis. I have not observed any RTL-specific bugs.

Some of the testing was documented here. I also repeated those same tests on Chrome (but didn't document them).

Test environment Various beta wikis.
Test browsers: Firefox 78 and Chromium 87.

Marking as done! Thanks for all the thorough testing and bug squashing