Opened 5 years ago

Closed 5 years ago

#15086 closed defect (fixed)

tor circuit display has text positioning/alignment problems for RTL languages

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-circuit-display, tbb-usability, TorBrowserTeam201503R, GeorgKoppen201503R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Reported by Sherief:
https://lists.torproject.org/pipermail/tor-qa/attachments/20150225/35ab6fb4/attachment-0003.png

Child Tickets

Attachments (6)

torCircuitRTL_Farsi.png (44.0 KB) - added by arthuredelstein 5 years ago.
circuitDisplayWith4Relays.png (41.7 KB) - added by arthuredelstein 5 years ago.
tor_browser_45a4_ar.png (23.3 KB) - added by gk 5 years ago.
circuitDIsplay_english.png (18.3 KB) - added by arthuredelstein 5 years ago.
circuitDisplay_arabic.png (19.2 KB) - added by arthuredelstein 5 years ago.
circuitDisplay_farsi.png (16.2 KB) - added by arthuredelstein 5 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by arthuredelstein

Attachment: torCircuitRTL_Farsi.png added

comment:1 Changed 5 years ago by arthuredelstein

Keywords: TorBrowserTeam201503R added
Status: newneeds_review

My patch for this ticket is here:
https://github.com/arthuredelstein/torbutton/commit/c1a3acb3716b7c5a678bb81c8576ab37302fab70

Here's the circuit display from the Farsi Tor Browser running the patch:

Changed 5 years ago by arthuredelstein

comment:2 in reply to:  1 Changed 5 years ago by arthuredelstein

Replying to arthuredelstein:

My patch for this ticket is here:
https://github.com/arthuredelstein/torbutton/commit/c1a3acb3716b7c5a678bb81c8576ab37302fab70

This patch has the added benefit that it is able to accommodate circuits with more relays. For example, here's a circuit display with Tor Browser connected to a tor instance patched to use 4 relays:


comment:3 Changed 5 years ago by gk

Keywords: GeorgKoppen201503R added

comment:4 Changed 5 years ago by gk

Status: needs_reviewneeds_revision

Looks good. I'd like to get two nits fixed:

1)

// Updates the Tor circuit display SVG, showing the current domain

It is no SVG anymore as far as I can see.

2) I agree that this bug fixes #13074 as well. If you have that fix in the same commit (which is fine for me) please give a hint in the commit message as well. This makes it a lot easier for people doing the release to get the changelog right (they can then just look for bug numbers with |git log|)

I tested a while the ar version and it behaves weird. (See attachment) Not sure if that is something we can/should fix, though. I guess there is an RTL/LTR mixup causing this due to yet untranslated strings?

Changed 5 years ago by gk

Attachment: tor_browser_45a4_ar.png added

Changed 5 years ago by arthuredelstein

Attachment: circuitDIsplay_english.png added

Changed 5 years ago by arthuredelstein

Attachment: circuitDisplay_arabic.png added

Changed 5 years ago by arthuredelstein

Attachment: circuitDisplay_farsi.png added

comment:5 Changed 5 years ago by arthuredelstein

Replying to gk:

Looks good. I'd like to get two nits fixed:

1)

// Updates the Tor circuit display SVG, showing the current domain

It is no SVG anymore as far as I can see.

Fixed.

2) I agree that this bug fixes #13074 as well. If you have that fix in the same commit (which is fine for me) please give a hint in the commit message as well. This makes it a lot easier for people doing the release to get the changelog right (they can then just look for bug numbers with |git log|)

Good idea. I've added a mention of #13704.

I tested a while the ar version and it behaves weird. (See attachment) Not sure if that is something we can/should fix, though. I guess there is an RTL/LTR mixup causing this due to yet untranslated strings?

Yes, it's partly due to untranslated strings. The circuit diagram should turn around if everything is translated. You're right that the parentheses are misbehaving, though, so I've added a fix for those.

Here is the new patch:
https://github.com/arthuredelstein/torbutton/commit/ac63db8d95fce5f743c04a8c4f5cca7b852504e8

Here is the corrected circuit display in English, Arabic, and Farsi:


comment:6 Changed 5 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:7 in reply to:  5 ; Changed 5 years ago by gk

Replying to arthuredelstein:

Replying to gk:

I tested a while the ar version and it behaves weird. (See attachment) Not sure if that is something we can/should fix, though. I guess there is an RTL/LTR mixup causing this due to yet untranslated strings?

Yes, it's partly due to untranslated strings. The circuit diagram should turn around if everything is translated. You're right that the parentheses are misbehaving, though, so I've added a fix for those.

Here is the new patch:
https://github.com/arthuredelstein/torbutton/commit/ac63db8d95fce5f743c04a8c4f5cca7b852504e8

Neat. Could you add a comment explaining what is going on and what we are doing about it?

comment:8 in reply to:  7 Changed 5 years ago by arthuredelstein

Replying to gk:

Neat. Could you add a comment explaining what is going on and what we are doing about it?

Good idea. I added comments to the lines in question, and rebased on top of master:
https://github.com/arthuredelstein/torbutton/commit/e8050135513b16792f97de4c127fe1f412f4bdff

comment:9 Changed 5 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Merged as commit 6d0c5e64ce8e104e3f9bfe648356a4f40fdde404. I slightly updated your commit message to make it better readable on some terminals (see: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html for my reasoning).

Note: See TracTickets for help on using tickets.