Closed Bug 1319302 Opened 7 years ago Closed 7 years ago

[Fennec] Turn RTL support on and fix a few oustanding issues for Android >= 4.2

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(relnote-firefox 53+, firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
relnote-firefox --- 53+
firefox53 --- fixed

People

(Reporter: maliu, Assigned: maliu)

References

Details

(Keywords: feature, rtl)

Attachments

(1 file)

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

- RTL will probably be needed in the future for L10n; would like to be able to accommodate for it now.

2 layouts (for tablets and phones) * no. of RTL languages + one default set for tablets and phones
Comment on attachment 8813002 [details]
Bug 1319302 - RTL support for Firefox for Android,

https://reviewboard.mozilla.org/r/94520/#review95226

LGTM.

Maybe it would be helpful to send a mail to the mobile-firefox-dev mailing list to let other developers know what they should be mindful of to support RTL in the future and do not break anything you just "fixed" (Best practices?).

After this lands in Nightly let's ask ItielMaN in bug 702845 to verify which linked bugs are now fixed (on Android >= 4.4) and what is still needed.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabPanelBackButton.java:18
(Diff revision 5)
> +import android.support.v4.graphics.drawable.DrawableCompat;
> +import android.support.v4.view.MarginLayoutParamsCompat;
> +import android.support.v4.view.ViewCompat;
> +import android.support.v4.view.ViewGroupCompat;
>  import android.util.AttributeSet;
> +import android.util.Log;

nit: Unused import

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PhoneTabsButton.java:36
(Diff revision 5)
> +        int layoutDirection = ViewCompat.getLayoutDirection(this);
> +
> +        Point[] nodes = getDirectionalNodes(width, height, layoutDirection);
> +        TabCurve.Direction directionalCurve = getDirectionalCurve(layoutDirection);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PhoneTabsButton.java:63
(Diff revision 5)
> +            nodes = new Point[]{
> +                    new Point(width, 0)
> +                    , new Point(width, height)
> +                    , new Point(0, height)
> +                    , new Point(0, 0)
> +            };
> +        } else {
> +            nodes = new Point[]{

nit: style, space before {

::: mobile/android/base/java/org/mozilla/gecko/widget/FadedSingleColorTextView.java:42
(Diff revision 5)
>                                            mTextGradient.getColor() != color ||
>                                            mTextGradient.getWidth() != width);
>  
>          final boolean needsEllipsis = needsEllipsis();
>          if (needsEllipsis && needsNewGradient) {
> -            mTextGradient = new FadedTextGradient(width, fadeWidth, color);
> +            boolean isRTL = ViewCompat.getLayoutDirection(this) == ViewCompat.LAYOUT_DIRECTION_RTL;

nit: final

::: mobile/android/base/java/org/mozilla/gecko/widget/ResizablePathDrawable.java:15
(Diff revision 5)
>  import android.graphics.Color;
>  import android.graphics.Paint;
>  import android.graphics.Path;
>  import android.graphics.drawable.ShapeDrawable;
>  import android.graphics.drawable.shapes.Shape;
> +import android.support.v4.graphics.drawable.DrawableCompat;

Unused import

::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedImageButton.java:20
(Diff revision 5)
>  import android.content.Context;
>  import android.content.res.ColorStateList;
>  import android.content.res.TypedArray;
>  import android.graphics.drawable.ColorDrawable;
>  import android.graphics.drawable.Drawable;
> +import android.support.v4.graphics.drawable.DrawableCompat;

Unused import
Attachment #8813002 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> After this lands in Nightly let's ask ItielMaN in bug 702845 to verify which
> linked bugs are now fixed (on Android >= 4.4) and what is still needed.

No problem. Just let me know when the changes will land in Nightly.
I can also test in Android 4.1.x, my family has some old devices. Tell me if that will be needed.
(In reply to ItielMaN from comment #7)
> (In reply to Sebastian Kaspari (:sebastian) from comment #6)
> > After this lands in Nightly let's ask ItielMaN in bug 702845 to verify which
> > linked bugs are now fixed (on Android >= 4.4) and what is still needed.
> 
> No problem. Just let me know when the changes will land in Nightly.
> I can also test in Android 4.1.x, my family has some old devices. Tell me if
> that will be needed.

We are relying on some Android platform features that have been introduced in Android 4.2 - 4.4. So with the changes here we should be in a much better shape on Android >= 4.4. With usage numbers of earlier Android versions going down it is unlikely that we will invest a lot of time to support older versions.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/120c84ca5362
RTL support for Firefox for Android, r=sebastian
Keywords: checkin-needed
(In reply to Max Liu [:maliu] from comment #0)
> +++ This bug was initially created as a clone of Bug #702845 +++

Your bug summary and patch commit summary does not contain enough information, to a point that GMail simply confused and combine the two threads. Please make sure to change the bug summary and make your intention clear next time!
Summary: RTL support for Firefox for Android → [Fennec] Turn RTL support on and fix a few oustanding issues for Android >= 4.2
ni L10N team Delphine such that L10N team can be on the same page of the current Fennec RTL status.
Flags: needinfo?(lebedel.delphine)
https://hg.mozilla.org/mozilla-central/rev/120c84ca5362
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Thanks everyone for the effort. I just downloaded the latest Nightly[1] for Persian locale to test the changes and update the RTL spec, but I cannot see any difference in the UI. Do I need to toggle any option or something?

[1] http://ftp.mozilla.org/pub/mobile/nightly/latest-mozilla-central-android-api-15-l10n/fennec-53.0a1.fa.android-arm.apk
Hi Max, could you check and feedback on Comment#16 ?
Flags: needinfo?(max)
Hi Arash,
For now, we support RTL by android framework native support. To see the difference, a. Android device system language must be RTL; b. App have to build with RTL locale. In your case, the apk is RTL ready. 
Would you help to confirm if your device language is also set to RTL(ex. Persian) please?
Flags: needinfo?(max) → needinfo?(mousavi.arash)
Thanks Max, one more question: Once we leverage the Android native RTL support, which Priority 1 parts of the following spec have been done ?

https://bug702845.bmoattachments.org/attachment.cgi?id=8787324

Thanks
Flags: needinfo?(max)
Thanks Max. I can confirm that with changing the whole phone's/tablet's language to Persian, most issues specified on RTL specs are fixed, except these issues happening only on tablet screens:

1) positions of tabs are still from left to right (first issue on page 3)
2) back/forward icon directions (first issue on page 3)
3) The lock icon/button (for SSL pages) overlaps forward button and makes it unusable (new issue not in specs)

The interface for phones seems pretty perfect. Isn't it possible to force the direction when choosing an RTL language?
Flags: needinfo?(mousavi.arash)
My question in Comment#19 is answered by Arash in Comment#20, but I will keep the ni for Max since still need his comment on Comment#20
(In reply to Arash Mousavi [:Arash-M] from comment #20)
> Thanks Max. I can confirm that with changing the whole phone's/tablet's
> language to Persian, most issues specified on RTL specs are fixed, except
> these issues happening only on tablet screens:
> 
> 1) positions of tabs are still from left to right (first issue on page 3)
Yes, this part is implemented by custom view, no native RTL support. So is WIP

> 2) back/forward icon directions (first issue on page 3)
> 3) The lock icon/button (for SSL pages) overlaps forward button and makes it
> unusable (new issue not in specs)
I noticed these two symptom during development, but I'm pretty sure they should both be fixed before patch landed.
I also cannot reproduce the symptom on current nightly build.
Would you help to provide more detailed information on Bug 928688 please?

> 
> The interface for phones seems pretty perfect. Isn't it possible to force
> the direction when choosing an RTL language?
This part is also under investigation. I'll file another bug to keep track on this.
Flags: needinfo?(max)
(clearing ni off me since this is on our radar)
Flags: needinfo?(lebedel.delphine)
Release Note Request (optional, but appreciated)
[Why is this notable]: Support RTL on Fennec firefox53
[Affects Firefox for Android]:
[Suggested wording]:RTL Support on Fennec firefox53
[Links (documentation, blog post, etc)]:
https://wiki.mozilla.org/Mobile/Firefox_for_Android/RTL
relnote-firefox: --- → ?
Depends on: 1332258
Priority: P5 → P1
Thanks, I updated the release note for 53 beta with a link to the wiki page.
Depends on: 1351439
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.