Closed Bug 1457720 Opened 6 years ago Closed 6 years ago

[RTL] Download folder text overlapping the folder's icon

Categories

(Firefox :: Settings UI, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + verified

People

(Reporter: itiel_yn8, Assigned: Paolo)

References

Details

(Keywords: regression, rtl)

Attachments

(2 files)

Attached image Screenshot
This started a few days ago, but for some reason I can't pinpoint it with mozregression (even after setting intl.uidirection pref to 1 the UI looks okay).

See attached.
This is most likely fallout from bug 1452624. Flagging needinfo for Paolo.
Blocks: 1452624
Flags: needinfo?(paolo.mozmail)
Priority: -- → P1
Keywords: rtl
Is this happening only on final builds? I tested this after changing "intl.uidirection" and the result looks fine to me.
Flags: needinfo?(paolo.mozmail) → needinfo?(itiel_yn8)
(In reply to :Paolo Amadini from comment #2)
> Is this happening only on final builds? I tested this after changing
> "intl.uidirection" and the result looks fine to me.

Makes sense (see comment 0); any mozregression build I run with this pref set to 1 is working just fine.
The only factor I see that should be related to this issue is the locale being RTL, because fresh English Nightly builds work okay, but on the other hand new profiles with RTL locale & RTL turned on have this issue.
"Final builds" = ?
Flags: needinfo?(itiel_yn8)
"Final builds" = the repackaged localized Nightly builds available on the website.

So, using the and the developer tools in Arabic was fun, and I located this "intl.css" declaration that is specific to this build:

https://dxr.mozilla.org/l10n-central/rev/f1dd8aa91a57075e2ef117cfcd498f61849e8d90/ar/toolkit/chrome/global/intl.css#50

Disabling these rules fixes the margin, but the "/" at the beginning of a custom path would appear at the end instead.

Apparently introduced in bug 289934, this solution isn't really scalable. What is the correct way to display the path correctly here, supporting all RTL builds including changing "intl.uidirection"? How can we then have the "#downloadFolder" selector removed from the localized repositories?

I'm also not quite sure what the other IDs refer to, if anything.
Flags: needinfo?(gandalf)
(In reply to :Paolo Amadini from comment #4)
> So, using the and the developer tools in Arabic was fun

The word "browser" was eaten by some Unicode bidirectional mark, I guess ;-)
I'm not sure how paths work in RTL, but generally speaking if it's HTML you can use `<bdi></bdi>` to isolate directionality. If it's an attribute and you can't use HTML elements, you can use FSI/PDI (that's what Fluent uses). http://unicode.org/reports/tr9/#Explicit_Directional_Overrides

You should be able to do `\u2068${path}\u2069`
Flags: needinfo?(gandalf)
(In reply to :Paolo Amadini from comment #2)
> Is this happening only on final builds?
> "Final builds" = the repackaged localized Nightly builds available on the
> website.

Yes.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> I'm not sure how paths work in RTL, but generally speaking if it's HTML you
> can use `<bdi></bdi>` to isolate directionality. If it's an attribute and
> you can't use HTML elements, you can use FSI/PDI (that's what Fluent uses).
> You should be able to do `\u2068${path}\u2069`

Thanks! Since we know when we are displaying a localized folder name versus a custom path, for the latter I did some testing and it seems I should use LRI (\u2066), otherwise if the first path component after the "/" is Arabic the whole paragraph is reversed. I think this matches the behavior of the current localized builds.

I'll post a patch for this, but how do we go about having the "#downloadFolder" selector removed from the localized repositories?
Flags: needinfo?(gandalf)
Comment on attachment 8972355 [details]
Bug 1457720 - Force the left-to-right direction when displaying a custom download path.

https://reviewboard.mozilla.org/r/240980/#review246756
Attachment #8972355 - Flags: review?(gandalf) → review+
This looks good! 

>  but how do we go about having the "#downloadFolder" selector removed from the localized repositories?

I don't think I understand the question. Can you rephrase?
Flags: needinfo?(gandalf) → needinfo?(paolo.mozmail)
There is a "toolkit/chrome/global/intl.css" that is apparently specific to each localized build. Arabic and a few other locales have this rule that causes the bug:

https://dxr.mozilla.org/l10n-central/rev/f1dd8aa91a57075e2ef117cfcd498f61849e8d90/ar/toolkit/chrome/global/intl.css#50
Flags: needinfo?(paolo.mozmail) → needinfo?(gandalf)
And what's the question? :)

Do you want to remove `#downloadFolder` from that css? With the LRI/PDI you should be able to, no?
Flags: needinfo?(gandalf)
Do I need to post a patch for l10n-central? Does it need to be reviewed by individual locale owners? There are three locales affected:

https://dxr.mozilla.org/l10n-central/search?q=%23downloadFolder&redirect=true
Ahh! Now I get your question. Sorry it took me so long! NI on :flod :)
Flags: needinfo?(francesco.lodolo)
(In reply to :Paolo Amadini from comment #14)
> Do I need to post a patch for l10n-central? Does it need to be reviewed by
> individual locale owners? There are three locales affected:
> 
> https://dxr.mozilla.org/l10n-central/search?q=%23downloadFolder&redirect=true

I've CCed the RTL alias, and a few localizers from ar/fa/he.

The request would be to remove the #downloadFolder selector from that rule? I can do it, and reference this bug in the landings, I'm only wondering if there are potential side effects.
Flags: needinfo?(francesco.lodolo)
Thanks! This should work as expected when the other patch here also lands, because we now set the direction in the main code.

In general, the approach of using "intl.css" looks a little fragile and we should ideally replace it with code in mozilla-central. Also, as I noted in comment 4, it's not even clear what the other IDs refer to, if anything.
Let me know when you need the changes landed in l10n repos.

Looking at Hebrew, which is only one with relatively recent changes, all the rest of the code is really ancient
https://hg.mozilla.org/l10n-central/he/log/tip/toolkit/chrome/global/intl.css

It also came out of Persian (and Ehsan) at some point, so he might have an idea.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3fb19eaeac
Force the left-to-right direction when displaying a custom download path. r=gandalf
Paolo - do you plan to land the intl.css removal separately?
Thanks flod!

I'm marking this bug as qe-verify+. To test this, once the patch has landed in Nightly:

1. Download the "ar", "he", and "fa" Nightly builds on either Linux or Mac
2. On each build, open the Preferences and select a custom path for where to save files
    - This should not be the Desktop or the Downloads folder
3. Check that the path is read from left to right with the "/" at the beginning
4. Check that the folder icon is on the right of the text, and does not overlap it
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/cb3fb19eaeac
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Tested the new Hebrew build, looking good on all cases.
Assignee: nobody → paolo.mozmail
Managed to reproduce the issue on Firefox Nightly (2018-04-30) on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64.

The issue cannot be reproduced on the latest Nightly Firefox 61.0a1. (20180503100146 / ar, he, fa) on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64, thus I will close it as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.