Closed Bug 1614767 Opened 2 years ago Closed 2 years ago

With intl.l10n.pseudo = bidi, icons are not mirrored

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla75
Tracking Status
firefox75 --- verified

People

(Reporter: itiel_yn8, Assigned: zbraniecki)

References

Details

Attachments

(3 files)

Attached image intl.l10n.pseudo = bidi

+++ This bug was initially created as a clone of Bug #1597822 +++

STR:

  1. Create a intl.l10n.pseudo pref with bidi as its contents.

AR:
The UI is flipped to look like RTL, but icons that are normally mirrored for RTL, aren't.

ER:
Icons should look as if intl.uidirection = 1.

This was supposedly fixed in bug 1597822 (and according to attachment 9110117 [details] everything looks fine with the patch) but this is still an issue.
At first I thought maybe this regressed since it was fixed on November 26th so I ran mozregression but I got to November 25th and I still see this being an issue.

Flags: needinfo?(gandalf)
Attached image intl.uidirection = 1
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)

Itiel - thank you for reporting it!

I now have a patch that may address it. Can you test the build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4c96e316731813a008ce636e9df2d34483e3f4 ?

Basically, when selecting document's directionality we used to look into ChromRegistry and check the "package". But we really didn't care about any other package than chrome, resource and about. And for those, we still really didn't expect their locale to be different than AppLocale.

But we used ChromeRegistry::IsLocaleRTL(package) test which doesn't check for intl.l10n.bidi since intl.l10n.bidi is not just directionality override, it's a locale override.

So with this patch, I'm switching us to check LocaleService::IsAppLocaleRTL for all packages we care about.

In result, with this patch I see the library icon, the fwd/back icons and the reload icon flipped. That seems to be an improvement.

One thing I noticed is that both with intl.l10n.bidi and with intl.uidirection we don't react to the switch updating the hamburger menu arrow icon, you need to open a new window after the switch to see the result.
That's likely worth filling as a new bug against the hambuger menu.

All in all, I'd expect with this patch the behavior of intl.uidirection and intl.l10n.pseudo to be aligned.
Please, verify that.

Flags: needinfo?(itiel_yn8)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #3)

Itiel - thank you for reporting it!

I now have a patch that may address it. Can you test the build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4c96e316731813a008ce636e9df2d34483e3f4 ?

All in all, I'd expect with this patch the behavior of intl.uidirection and intl.l10n.pseudo to be aligned.
Please, verify that.

Tested different bits of the Firefox UI and all seem well with this build, for both prefs :)

One thing I noticed is that both with intl.l10n.bidi and with intl.uidirection we don't react to the switch updating the hamburger menu arrow icon, you need to open a new window after the switch to see the result.
That's likely worth filling as a new bug against the hambuger menu.

Hmm? I do know that there are several places where updating the prefs requires to open a new window / restart Firefox for them to function properly, but I'm not sure what you mean?
All arrow icons in the hamburger menu look okay to me immediately after updating the prefs...

I see some non-upside-down text when intl.l10n.bidi = bidi. I'm guessing all of these will become upside-down after the migration of Firefox to fluent will be completed?

Flags: needinfo?(itiel_yn8) → needinfo?(gandalf)

Hmm? I do know that there are several places where updating the prefs requires to open a new window / restart Firefox for them to function properly, but I'm not sure what you mean?

If you look at the screenshots you attached in this bug, in the hamburger menu by "Sign in to Firefox" the arrow ">" is pointing to the left in one picture, and to the right in the other.
I verified that this is not a difference between intl.uidirection and intl.l10n.pseudo but rather the fact that you need to open a new window after flipping either to trigger the arrow to point in the new direction.

Ok, thank you! I'll ask for review of this patch!

Flags: needinfo?(gandalf)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #5)

Hmm? I do know that there are several places where updating the prefs requires to open a new window / restart Firefox for them to function properly, but I'm not sure what you mean?

If you look at the screenshots you attached in this bug, in the hamburger menu by "Sign in to Firefox" the arrow ">" is pointing to the left in one picture, and to the right in the other.
I verified that this is not a difference between intl.uidirection and intl.l10n.pseudo but rather the fact that you need to open a new window after flipping either to trigger the arrow to point in the new direction.

I'm confused, the screenshots I attached earlier were taken before testing with your patch and then the arrows are indeed pointing in the wrong direction, but now after testing with your patch (with either prefs) they are pointing to the correct direction without having to open a new window first. Are you not seeing the same?

Also, kindly answer the last question from comment 4 :-)
(I need to know if the recommendation in the following MDC doc for testers to test RTL using the intl.l10n.pseudo pref is misleading atm or not, and edit it accordingly:
https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/RTL_Guidelines#Testing )

Flags: needinfo?(gandalf)

I'm confused, the screenshots I attached earlier were taken before testing with your patch and then the arrows are indeed pointing in the wrong direction, but now after testing with your patch (with either prefs) they are pointing to the correct direction without having to open a new window first. Are you not seeing the same?

No, With my patch you still will get the "wrong" direction of those arrows with both intl.uidirection and intl.l10n.pseudo if you switch the preference and check the menu in a window previously opened.
That is not related to this patch and is not fixed by it.

I see some non-upside-down text when intl.l10n.bidi = bidi. I'm guessing all of these will become upside-down after the migration of Firefox to fluent will be completed?

Yeah, only Fluent is pseudolocalized.

Flags: needinfo?(gandalf)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #7)

if you switch the preference and check the menu in a window previously opened.

Ah, that's what I was missing. Thanks.

Priority: -- → P3
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87347a74981d
Use AppLocale to set document direction for chrome, about and resource protcols. r=jfkthame

Ugh... this tests actually relies on the idea that we can override locale in chrome registry - https://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/rtltest/righttoleft.manifest#3

:jfkthame - would you be ok with me removing it? I don't think it makes sense anymore to try to salvage it as we are actively moving away from ChromeRegistry overall?

Flags: needinfo?(gandalf) → needinfo?(jfkthame)

Yeah, I guess that's fine.

Flags: needinfo?(jfkthame)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/127a116c7f53
Use AppLocale to set document direction for chrome, about and resource protcols. r=jfkthame
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

All seems to work great now. Thanks!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.