RTL support for the source code license page

VERIFIED FIXED in 5.0.7

Status

VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({rtl})

unspecified
5.0.7

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
RTL support for the source code license page.
Attachment #382270 - Flags: review?(clouserw)
Comment on attachment 382270 [details] [diff] [review]
Patch (v1)

I am not sure your assumptions are right. The header language is marked as en-US if fallback to en-US is in place. Is that unnecessary? The expected behavior is that an en-US name is displayed correctly regarding its locale, while if a name is present in Farsi (and others), it is marked as such as well and displayed correctly.

What you may not assume, however, is that the source license locale matches the one of the add-on name. I would be surprised if we had, for example, a French version of the GPL to go along with a French add-on name.
Attachment #382270 - Flags: review?(clouserw) → review-
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)
> (From update of attachment 382270 [details] [diff] [review])
> I am not sure your assumptions are right. The header language is marked as
> en-US if fallback to en-US is in place. Is that unnecessary? The expected
> behavior is that an en-US name is displayed correctly regarding its locale,
> while if a name is present in Farsi (and others), it is marked as such as well
> and displayed correctly.

The header contains a text (versions_license_header_source) which will be translated to Persian for example.  That text might contain an English name which will be LTR, and the LTR text inside an RTL block will flow correctly.  But the way things are right now, the block becomes LTR itself, which causes neither of the parts to flow correctly.

Perhaps we should consider wrapping the add-on name in a span with the locale HTML added to it?

> What you may not assume, however, is that the source license locale matches the
> one of the add-on name. I would be surprised if we had, for example, a French
> version of the GPL to go along with a French add-on name.

Fair enough.  Will license texts ever be translated, if not, then we should mark this block as LTR and left-aligned explicitly all the time.
(Assignee)

Comment 3

10 years ago
Posted patch Patch (v2) (obsolete) — Splinter Review
Something like this perhaps?
Attachment #382270 - Attachment is obsolete: true
Attachment #382274 - Flags: review?(fwenzel)
Attachment #382274 - Flags: review?(fwenzel) → review+
Comment on attachment 382274 [details] [diff] [review]
Patch (v2)

Ah, you have a good point there. I forgot that the string itself is localized so we can indeed not mark it as ltr.

I'll attach a screen shot in a second, please take a look if that is readable for you, and if the switch from ltr to rtl in the middle makes sense?

AMO 5.0.6 is code-frozen though, so please do not check it in before the development on 5.0.7 has started, unless clouserw considers this a release blocker.
Posted image Screenshot
Here's a screenshot of your fix. (Please note that I just chose the license and add-on randomly, I am not making claims about ABP's source license here).
Wil: I'm putting this into 5.0.7, please pull it back and commit it/have it committed if this blocks the release.
Target Milestone: 5.0.6 → 5.0.7
(Assignee)

Comment 7

10 years ago
Posted patch Patch (v2.1)Splinter Review
This look mostly good except that the span jumps up a bit.  This patch fixes that as well.  Thanks for creating the screenshot.
Attachment #382274 - Attachment is obsolete: true
Attachment #382319 - Flags: review?(fwenzel)
Attachment #382319 - Flags: review?(fwenzel) → review+
Comment on attachment 382319 [details] [diff] [review]
Patch (v2.1)

Yes, that works, absolutely.
(Assignee)

Comment 9

10 years ago
Adding checkin-needed because I'm sure that I'm going to miss this...
Keywords: checkin-needed
Whiteboard: should be checked in after 5.0.6 code freeze
r27799
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: should be checked in after 5.0.6 code freeze
Verified FIXED; preview matches Fred's screenshot in comment 5.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.