497069 - RTL support for the source code license page

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

RTL support for the source code license page.

Comment 1

[Fred Wenzel [:wenzel]]

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.

Comment 2

[(no longer active)]

(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.

Comment 3

[(no longer active)]

Attachment: Patch (v2)

Something like this perhaps?

Comment 4

[Fred Wenzel [:wenzel]]

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.

Comment 5

[Fred Wenzel [:wenzel]]

Attachment: 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).

Comment 6

[Fred Wenzel [:wenzel]]

Wil: I'm putting this into 5.0.7, please pull it back and commit it/have it committed if this blocks the release.

Comment 7

[(no longer active)]

Attachment: Patch (v2.1)

This look mostly good except that the span jumps up a bit.  This patch fixes that as well.  Thanks for creating the screenshot.

Comment 8

[Fred Wenzel [:wenzel]]

Comment on attachment 382319 [details] [diff] [review]
Patch (v2.1)

Yes, that works, absolutely.

Comment 9

[(no longer active)]

Adding checkin-needed because I'm sure that I'm going to miss this...

Comment 10

[Wil Clouser [:clouserw]]

r27799

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; preview matches Fred's screenshot in comment 5.