Page MenuHomePhabricator

Right-to-Left directionality problem with refs
Closed, ResolvedPublicBUG REPORT

Description

Bidirectional sentences don't have correct ref directionality in mobile-html, as opposed to the browser version of Wikipedia.

For example, in the article "ירושלים" in Hebrew Wikipedia, after the words "ברום של 570", appearing in the introduction, there is a ref, and its number appears to the right of "570", while is should be displayed on its left, as Hebrew is read from right-to-left.

https://he.wikipedia.org/api/rest_v1/page/mobile-html/%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D

Works fine on Windows in Internet Explorer and Chrome; on MacOS in Safari and Chrome; and in Android on Chrome.

https://he.m.wikipedia.org/wiki/%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D

Steps to reproduce

  1. Write a sentence involving Hebrew words, followed by English words or numbers (any "[a-zA-Z0-9]+"), and then immediately by a reference.
  2. Watch the article

Expected results

Ref number will be on the left of the English-numerical word.

Actual results

Ref number will be on the right of the English-numerical word.

Event Timeline

tomert created this task.May 6 2020, 7:37 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 6 2020, 7:37 AM
Dbrant renamed this task from Wikipedia Android app Right-to-Left directionality problem with refs to Right-to-Left directionality problem with refs.May 6 2020, 3:01 PM
Dbrant updated the task description. (Show Details)
LGoto triaged this task as Medium priority.May 6 2020, 3:35 PM
LGoto added a project: Parsoid.

FWIW it looks the same in the original Parsoid HTML, which is probably the place to fix it: https://he.wikipedia.org/api/rest_v1/page/html/%D7%99%D7%A8%D7%95%D7%A9%D7%9C%D7%99%D7%9D

Restricted Application added a project: I18n. · View Herald TranscriptMay 6 2020, 3:58 PM
bearND added a subscriber: bearND.May 6 2020, 5:03 PM

Since this issue comes from Parsoid it appears in mobile-sections, too.

Huji added a subscriber: Huji.May 6 2020, 6:11 PM

The solution is to apply this CSS rule: unicode-bidi: isolate; to the <sup ... class="mw-ref"> tags.

Amire80 added a subscriber: Amire80.May 7 2020, 8:14 AM

The solution is to apply this CSS rule: unicode-bidi: isolate; to the <sup ... class="mw-ref"> tags.

This is probably right. That's what is done on the web version. I don't know how to inspect elements in the app :)

Amire80 moved this task from Untriaged to RTL on the I18n board.May 7 2020, 8:15 AM
tomert updated the task description. (Show Details)May 7 2020, 8:44 AM
ssastry added a subscriber: ssastry.May 7 2020, 1:38 PM

The solution is to apply this CSS rule: unicode-bidi: isolate; to the <sup ... class="mw-ref"> tags.

This is probably right. That's what is done on the web version. I don't know how to inspect elements in the app :)

I imagine this CSS rule is missing in Parsoid's CSS module.

bearND added a comment.EditedMay 7 2020, 2:22 PM

This is probably right. That's what is done on the web version. I don't know how to inspect elements in the app :)

You can look at the mobile-html output (see link in task description) in any browser -- on a desktop browser, too.

Huji added a comment.May 7 2020, 2:54 PM

This is probably right. That's what is done on the web version. I don't know how to inspect elements in the app :)

You can look at the mobile-html output (see link in task description) in any browser -- on a desktop browser, too.

Correct. And in fact, that is how I made sure the CSS solution I proposed actually fixes the issue.

ssastry moved this task from Needs Triage to Bugs & Crashers on the Parsoid board.May 7 2020, 9:40 PM

Once the upstream Parsoid fix is in place we'll want to replicate in mobile-actions. @ssastry would you please ping us when the Parsoid fix is done?

Change 596248 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/core@master] WIP: Fix RTL directionality issue with refs

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

ssastry added a comment.EditedMay 22 2020, 9:43 PM

This turned out to be a bit of a small rabbit hole.

So, anyway, I am going to:
(a) change Cite's CSS (ext.cite.style.css which is Parsoid/VE specific not to be confused with ext.cite.styles.css (note the plural) :-)) isolate and add Amir as a reviewer so he can review it and see that the change from embed to isolate is correct
(b) Get rid of the stray Parsoid CSS styles (for Cite) from mediawiki/core and add anything that is relevant to Cite's ext.cite.style.css
(c) Separately, we'll see if we can rename / merge ext.cite.style.css and ext.cite.styles.css

cscott added a subscriber: cscott.May 22 2020, 9:52 PM

Re c: or at least rename ext.cite.style.css to parsoid.ext.cite.styles.css (which we can do w/o or before merging the styles) to make this less confusing to review!

Change 598125 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/extensions/Cite@master] Sync unicode-bidi property for <sup> tags between Parsoid & core impls

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

Change 596248 abandoned by Subramanya Sastry:
Fix RTL directionality issue with refs

Reason:
In favour of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/ /598125 .. We'll remove the stray Cite styles out of here in a separate patch.

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

ssastry added a comment.EditedMay 22 2020, 10:11 PM

Re c: or at least rename ext.cite.style.css to parsoid.ext.cite.styles.css (which we can do w/o or before merging the styles) to make this less confusing to review!

There is a (deployment) ordering problem with simply renaming it since Parsoid's Cite implementation is in the Parsoid repo and CSS module declaration (ext.cite.style.css) is in there. Since we are in the process of starting to migrate Parsoid's implementations to the respective extension repos, once that is done, it is simpler to do this change (merge / refactor / rename).

Will file a separate phab task for that.

Ah, ok, so the changes proceeded from: span.reference -> span.mw-ref -> sup.mw-ref .. I missed that transition in between.

We can handle the deployment ordering problem, just make sure they are both merged within the same train window.

Change 598125 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Sync unicode-bidi property for <sup> tags between Parsoid & core impls

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

Once the upstream Parsoid fix is in place we'll want to replicate in mobile-actions. @ssastry would you please ping us when the Parsoid fix is done?

This change went out in MW 1.35.0-wmf.35 the week of June 2.

@bearND please verify it looks fixed on your end and resolve / update task appropriately. Thanks!

Change 607595 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Fix display of references for RTL

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

Thanks for the ping. It's still broken because in mobileapps we have our own copy of styles. I've copied the changes you made in the Cite ext fix.

Change 607595 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Fix display of references for RTL

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

@Traffic Is mwmaint1002 still the host to use when using mwscript purgeList.php?
On that host I've tried echo 'https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base' | mwscript purgeList.php and the script was fine with it but I still see old contents when accessing the URL.
OLD/actual:
.mw-ref{[...]unicode-bidi:embed;

$ curl -s "https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base" | grep -o '.mw-ref{[^}]*'
.mw-ref{vertical-align:super;line-height:1;font-size:smaller;unicode-bidi:embed;font-weight:400;font-style:normal

NEW/expected:
.mw-ref{[...]unicode-bidi:-moz-isolate;unicode-bidi:-webkit-isolate;unicode-bidi:isolate

$ curl -s "https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base?a=1" | grep -o '.mw-ref{[^}]*'         
.mw-ref{vertical-align:super;line-height:1;font-size:smaller;font-weight:400;font-style:normal;unicode-bidi:-moz-isolate;unicode-bidi:-webkit-isolate;unicode-bidi:isolate
Restricted Application added a project: Operations. · View Herald TranscriptJun 24 2020, 9:41 PM
bearND removed bearND as the assignee of this task.Jun 24 2020, 10:01 PM
bearND closed this task as Resolved.EditedJun 26 2020, 5:05 PM
bearND claimed this task.

Ok, looks like enough time has passed for the old, cached version to be evicted and the newly deployed CSS to get served. It works now in the mobile CSS (mobile-html, native apps), too.