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)
Firefox
Settings UI
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)
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.
Updated•6 years ago
|
Keywords: regression
Comment 1•6 years ago
|
||
This is most likely fallout from bug 1452624. Flagging needinfo for Paolo.
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
"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)
Assignee | ||
Comment 5•6 years ago
|
||
(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 ;-)
Comment 6•6 years ago
|
||
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.
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
tracking-firefox61:
--- → +
Assignee | ||
Comment 8•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
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+
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
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)
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
Ahh! Now I get your question. Sorry it took me so long! NI on :flod :)
Flags: needinfo?(francesco.lodolo)
Comment 16•6 years ago
|
||
(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)
Assignee | ||
Comment 17•6 years ago
|
||
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.
Comment 18•6 years ago
|
||
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.
Assignee | ||
Comment 19•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=405f934227fdb9f5cb777e84e2fa990ff1823181
Assignee | ||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b74da1aa868e3151e310d022120be9517e9fea
Comment 21•6 years ago
|
||
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
Comment 22•6 years ago
|
||
Paolo - do you plan to land the intl.css removal separately?
Comment 23•6 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22) > Paolo - do you plan to land the intl.css removal separately? https://hg.mozilla.org/l10n-central/ar/rev/09127d0ec2ec16f07edb809977bfe6066dc4f5f6 https://hg.mozilla.org/l10n-central/he/rev/afd7e27c4f017ea58a1c21524851065d98f2a8ec https://hg.mozilla.org/l10n-central/fa/rev/c386e67c521bb7b03f6720840fb5b7935a29ea5d
Assignee | ||
Comment 24•6 years ago
|
||
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+
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb3fb19eaeac
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Comment 26•6 years ago
|
||
Tested the new Hebrew build, looking good on all cases.
Updated•6 years ago
|
Assignee: nobody → paolo.mozmail
Comment 27•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•