Opened 6 years ago

Closed 6 years ago

#9157 closed task (fixed)

Persian and Arabic should be right aligned on bridges.tpo

Reported by: runa Owned by: isis
Priority: Low Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: RTL, translations, ui
Cc: Matthew.Finkel@…, isis@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Persian and Arabic should be right aligned on bridges.torproject.org.

Child Tickets

Attachments (5)

bug9157_bridges.png (84.9 KB) - added by sysrqb 6 years ago.
bug9157_index.png (89.2 KB) - added by sysrqb 6 years ago.
bug9157_bridges_rtl_dir.png (77.9 KB) - added by sysrqb 6 years ago.
bridges.tpo-farsi-RTL.jpg (189.2 KB) - added by isis 6 years ago.
screenshot of testing of sysrqb's RTL display patch for bridges.tpo in Farsi
9157-english-request.png (95.0 KB) - added by isis 6 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 6 years ago by sysrqb

Cc: Matthew.Finkel@… added

Silly question, but for the main page where is says "Step 1: Get the PTTTB", when translated is the order "Get the PTTBB: Step 1" ?

comment:2 Changed 6 years ago by runa

I believe that is correct.

Changed 6 years ago by sysrqb

Attachment: bug9157_bridges.png added

Changed 6 years ago by sysrqb

Attachment: bug9157_index.png added

comment:3 in reply to:  2 Changed 6 years ago by sysrqb

Status: newneeds_review

Replying to runa:

I believe that is correct.

Thanks


Ok, so I have a patch in bug9157 on https://github.com/sysrqb/bridgedb
It's hacky, but I think until someone else can spend time to do this correctly, this will do. Screenshots of it running locally are attached.

comment:4 Changed 6 years ago by sysrqb

Keywords: important added

comment:5 Changed 6 years ago by isis

Cc: isis@… added
Keywords: RTL translations ui added; important removed
Priority: normalminor
Status: needs_reviewneeds_revision

So, there are a couple things which need a bit of touchup:

1) In HTTPServer.py, in classes WebRoot and WebResource, you do:

    # Grab only the language (first two characters) so we know if we need
    # to right-justify the text
    langs = [ lang[:2] for lang in langs ]
    rtl = False
    if "ar" in langs or "fa" in langs:
        rtl = True

which, for example, if someone were to speak Deutsch and Arabic, they would see German text going right-to-left. See attached screenshot.

2) There is a lot of duplicate code...the above bug is duplicated twice. The following structure is replicated several times:

% if rtl:
    <h4 style="float: right;">
% else:
    <h4>
% endif
    ${_("My bridges don't work! I need help!")}</h4>

which also brings me to...

3) #2 is a bug also. This ticket was ill-specified, and it sucks a bit that you put work into it before someone could point out that right-alignment wasn´t what was needed. Farsi and Arabic (and others) need to be written right-to-left, which isn't the same as align-right, it's more like mirror image. I can read small amounts of Arabic, and the first thing I noticed was that the punctuation is on the wrong side of the sentence. Doing this can be a bit tricky to get right, esp. in our case where we have English text in the middle of RTL languages. The W3C has some good documents on best practices for getting it right, IIRC there's something about using the dir tag in the <meta> sections to set the default, and only when you need to change directions to not-the-default it gets used again. Also, I am not a web developer either, perhaps someone else knows more. In fact, I'm sure one of our friends on IRC coming from an RTL-language speaking country could probably write this code in five minutes, or, at least, better point out when things look weird to them.

4) Farsi and Arabic aren't the only languages which we support which are RTL, off the top of my head, we've already got translations for Hebrew, and Kurdish is also one of the languages we support (if you want a wikipedia blackhole and an interesting history lesson about the mistreated Kurdish ethnic groups in Iran, Turkey and Syria...that just ate my past ten minutes). See the bridgedb* branches in the translation repo for all the languages we get translations for.

5) Line wrapping. This one also isn't your fault, because the line was already that way, but the OCD side of me prefers that PEP8 be followed when touching lines of code (which, yes, means fixing up past contributors mistakes). For example, I am looking at L16 in lib/bridgedb/i18n/bridges.html -- it's like a zillion columns long! In the case of translations, this is actually a tiny bit more than OCD, as someone has to review all the .po files from the translator volunteers before merging (mostly to make sure there aren't any <script> tags or other hijinks which could be used to exploit a browser). If the line is a zillion columns long, there's a higher chance that I might scroll past it and not actually see the end of it, especially if I'm bored from looking at 20+ files of gibberish.

comment:6 in reply to:  5 Changed 6 years ago by isis

Replying to isis:

5) Line wrapping. This one also isn't your fault, because the line was already that way, but the OCD side of me prefers that PEP8 be followed when touching lines of code [...]

There's this script that I use for checking PEP8 conformity; it's used in Twisted and a few other major Python projects as a part of a client-side githook.

comment:7 in reply to:  5 Changed 6 years ago by sysrqb

Replying to isis:

So, there are a couple things which need a bit of touchup:

1) In HTTPServer.py, in classes WebRoot and WebResource, you do:

    # Grab only the language (first two characters) so we know if we need
    # to right-justify the text
    langs = [ lang[:2] for lang in langs ]
    rtl = False
    if "ar" in langs or "fa" in langs:
        rtl = True

which, for example, if someone were to speak Deutsch and Arabic, they would see German text going right-to-left. See attached screenshot.

True, I wanted to limit it to the first lang in the Languages header. The issue I had was that we have no guarantee that we support the first language and we can't (easily) query gettext, as far as I can tell. So, presently I hope everyone prefers languages read/written rtl over all others.

2) There is a lot of duplicate code...the above bug is duplicated twice.

Yeah, this certainly isn't the best implementation. I was/am more concerned with not having a crappy website than code quality, specifically for this.

3) #2 is a bug also. This ticket was ill-specified, and it sucks a bit that you put work into it before someone could point out that right-alignment wasn't what was needed. Farsi and Arabic (and others) need to be written right-to-left, which isn't the same as align-right, it's more like mirror image.

Initially I thought you were wrong, however now it appears I was wrong, indeed. The answer was to add the direction css attribute. Now all is good(er). I expected the translation to be typed rtl, so I thought the only issue was that the text was not right-justified (which this does a hacky job of accomplishing). hrmph. This should be correct now.

I can read small amounts of Arabic, and the first thing I noticed was that the punctuation is on the wrong side of the sentence. Doing this can be a bit tricky to get right, esp. in our case where we have English text in the middle of RTL languages. The W3C has some good documents on best practices for getting it right, IIRC there's something about using the dir tag in the <meta> sections to set the default, and only when you need to change directions to not-the-default it gets used again. Also, I am not a web developer either, perhaps someone else knows more. In fact, I'm sure one of our friends on IRC coming from an RTL-language speaking country could probably write this code in five minutes, or, at least, better point out when things look weird to them.

Probably true.

4) Farsi and Arabic aren't the only languages which we support which are RTL, off the top of my head, we've already got translations for Hebrew, and Kurdish is also one of the languages we support (if you want a wikipedia blackhole and an interesting history lesson about the mistreated Kurdish ethnic groups in Iran, Turkey and Syria...that just ate my past ten minutes). See the bridgedb* branches in the translation repo for all the languages we get translations for.

Right, this was just me taking this ticket very literally. That was very dumb, I admit.

5) Line wrapping. This one also isn't your fault, because the line was already that way, but the OCD side of me prefers that PEP8 be followed when touching lines of code (which, yes, means fixing up past contributors mistakes). For example, I am looking at L16 in lib/bridgedb/i18n/bridges.html -- it's like a zillion columns long! In the case of translations, this is actually a tiny bit more than OCD, as someone has to review all the .po files from the translator volunteers before merging (mostly to make sure there aren't any <script> tags or other hijinks which could be used to exploit a browser). If the line is a zillion columns long, there's a higher chance that I might scroll past it and not actually see the end of it, especially if I'm bored from looking at 20+ files of gibberish.

Yes, that is quite a zillion columns longs (for some number 'zillion' consisting of ~176 characters)! :p

Changed 6 years ago by sysrqb

Attachment: bug9157_bridges_rtl_dir.png added

comment:8 Changed 6 years ago by Sherief

After looking at sysrqb's attachment I can say that the text looks perfectly fine and ready for use.

comment:9 Changed 6 years ago by sysrqb

Status: needs_revisionneeds_review

Ok, slightly-less-hacky branch on bug9157_r2_rebase https://github.com/sysrqb/bridgedb.git

This does not use the dir markup attribute, but that does sound useful, we should actually see if we can use it. Maybe in the next rev.

comment:10 in reply to:  9 ; Changed 6 years ago by sysrqb

Replying to sysrqb:

Ok, slightly-less-hacky branch on bug9157_r2_rebase https://github.com/sysrqb/bridgedb.git

That's actually kinda annoying (at least to me). See commit a31c592eadd72a659f22497f66e77fb90c6c207b

(I also didn't whitespace-check it, don't shoot me!)

comment:11 in reply to:  8 Changed 6 years ago by isis

Replying to Sherief:

After looking at sysrqb's attachment I can say that the text looks perfectly fine and ready for use.

Thanks for checking it for us, Sherief! :)

comment:12 in reply to:  10 Changed 6 years ago by isis

Owner: set to isis
Status: needs_reviewaccepted

Replying to sysrqb:

Replying to sysrqb:

Ok, slightly-less-hacky branch on bug9157_r2_rebase https://github.com/sysrqb/bridgedb.git

That's actually kinda annoying (at least to me). See commit a31c592eadd72a659f22497f66e77fb90c6c207b

(I also didn't whitespace-check it, don't shoot me!)

Don't worry, I don't shoot accomplices. I just incessantly nag them about obsessive compulsive formatting divergences. :)

Overall, this looks really good. One thing, and it's not that important, because overall the documentation is great, but just so that you don't have to dig through the Sphinx API documentation on parameter specifications...the type of a :param: directive is specified like this:

:param str beach: The place with sand where you go to code and surf.

or like this if it has some weird type (like it's a class or something):

:type beach: :class:`sys.env.Beach`
:param beach: The place with sand where you go to code and surf.

:returns: is the same:

:rtype: str
:returns: Some string.

or

:rtype: :class:`sys.env.Beach`
:returns: That place with sand where you go to hack.

Also, I totally don't care for now, since I'm sure Sphinx would choke on a lot worse if I tried to generate docs from BridgeDB's code right now. :P

comment:13 Changed 6 years ago by isis

I cherry-picked the above-mentioned commit a31c592e into my branch 'testing/sysrqb/bug9157_r2_rebase-cherrypick-a31c592e', and it seems to be working great. Screenshot attached for Farsi, and it even handles putting the keybinding tags on the correct side of the links for my I-hate-using-a-mouse extension. :D

Changed 6 years ago by isis

Attachment: bridges.tpo-farsi-RTL.jpg added

screenshot of testing of sysrqb's RTL display patch for bridges.tpo in Farsi

comment:14 Changed 6 years ago by isis

Resolution: fixed
Status: acceptedclosed

Merging into develop for the next deployment (0.1.2), thanks!

comment:15 Changed 6 years ago by isis

For the record, LTR languages (I just tested with French) work too. Hooray! :D

comment:16 Changed 6 years ago by isis

Resolution: fixed
Status: closedreopened

I remember testing a couple days later from my branch testing/sysrqb/bug9157_r2_rebase-cherrypick-a31c592e (which is what was merged for deployment) and noticing that even though several variants of en are sent with top-priority in my headers, that because I accept multiple languages, some of which are RTL, that bridgedb responded with English going right-to-left. Which was cool looking, but if that is the case, we should sort that out before deploying. Someone should test again from the above branch and see if they get the same thing.

comment:17 Changed 6 years ago by isis

Status: reopenedneeds_review

comment:18 Changed 6 years ago by sysrqb

Status: needs_reviewneeds_information

Hm, can you retest and see if you can reproduce it? I tested with English, Arabic, Hebrew (in that order) and it still rendered LTR. Let me know how you break it, so that I can fix it. :)

Changed 6 years ago by isis

Attachment: 9157-english-request.png added

comment:19 in reply to:  18 Changed 6 years ago by isis

Resolution: fixed
Status: needs_informationclosed

Replying to sysrqb:

Hm, can you retest and see if you can reproduce it? I tested with English, Arabic, Hebrew (in that order) and it still rendered LTR. Let me know how you break it, so that I can fix it. :)

I can reproduce the bug if I test your patches on top of the new translations that I'm merging, but not if I test your patches on top of master -- so I suspect it is a problem with the translation files.

What is happening is actually that English isn't displaying at all...instead it returns the first non-English language in the Accept-Language list. Here's a screenshot, running from your patches on top of the newly merged translations, requesting three types of English, then French (the red text is the relevant log lines of the request, it's not actually on the page).


I am inclined to believe that the newly merged translations are messing this up, not your patches. And, obviously, the text direction is fine for French, so I'm closing this as fixed and possibly opening a new ticket for why English, of all things, seems to be missing.

Note: See TracTickets for help on using tickets.