RTL support for history page

VERIFIED FIXED

Status

()

Firefox for Android
Awesomescreen
P1
normal
VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: Lauren, Assigned: Lauren)

Tracking

(Blocks: 1 bug)

Trunk
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

This bug blocks on 702845; all the padding on the history page tab widget should be in the reverse direction when in RTL locale
(Assignee)

Updated

4 years ago
Blocks: 702845
Depends on: 924418
(Assignee)

Updated

4 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: Firefox 27 → Trunk
(Assignee)

Comment 1

4 years ago
Created attachment 827854 [details] [diff] [review]
historyPage_rtl.patch
Attachment #827854 - Flags: feedback?

Updated

4 years ago
Assignee: nobody → kolauren
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

4 years ago
Attachment #827854 - Flags: feedback? → review?(sriram)
Comment on attachment 827854 [details] [diff] [review]
historyPage_rtl.patch

Throwing review ot Lucas while Sriram is PTO.

Question: Does paddingStart work on older versions of Android or do we need both paddingStart and paddingLeft ?
Attachment #827854 - Flags: review?(sriram) → review?(lucasr.at.mozilla)
Comment on attachment 827854 [details] [diff] [review]
historyPage_rtl.patch

Review of attachment 827854 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Giving r- to get some answers. Looks fine *if* it doesn't break anything on pre-17 Android.

::: mobile/android/base/resources/values-large-land-v11/styles.xml
@@ +46,5 @@
>  
>      <style name="Widget.Home.HistoryTabWidget">
>          <item name="android:showDividers">beginning|middle|end</item>
>          <item name="android:dividerPadding">0dp</item>
> +        <item name="android:paddingStart">100dp</item>

Isn't this going to break on pre-17 Android? Or is this made backwards-compatible at build time?
Attachment #827854 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 4

4 years ago
Created attachment 8341787 [details] [diff] [review]
bug-935388-fix
(Assignee)

Comment 5

4 years ago
Woops, that's right - I've uploaded a new patch keeping the paddingLeft attribute.
(Assignee)

Updated

4 years ago
Attachment #8341787 - Flags: review?(lucasr.at.mozilla)

Updated

4 years ago
Attachment #8341787 - Attachment is patch: true
Comment on attachment 8341787 [details] [diff] [review]
bug-935388-fix

Review of attachment 8341787 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, assuming the paddingStart attribute is safely ignored on pre-17 Android devices.
Attachment #8341787 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 7

4 years ago
Yep! It actually is. They actually recommend that you keep both paddingStart and paddingLeft attributes to support pre and post 17 devices: http://android-developers.blogspot.ca/2013/03/native-rtl-support-in-android-42.html
No longer depends on: 924418
Priority: -- → P1

Updated

9 months ago
QA Contact: ioana.chiorean

Comment 8

6 months ago
Looking pretty good on the latest Nightly build, however there are 2 minor issues.
The first is where long hisotry item's titles/URLs are being faded out from the wrong side (or not faded at all if the text is hebrew).
Scenarios to consider:
1. Long URLs should ALWAYS fade away from the right side, no matter if the build is RTL or LTR
2. Long history item's title should be faded away according to it's text directionality:
   a. If text is RTL, fade out from the left (currently there's no fading at all)
   b. If text is LTR, fade out from the right
   * The same should possibly be applied to LTR builds as well

The second is that the strings indicating the time the history items were created are still LTR ("Today", "Last 7 days" etc).

I may file different bugs on these two.
Hi ItielMaN , 

Thank you for your support! could you please file another 2 bugs mentioned in comment8, and share with us the bugID!

Thank you very much !
Flags: needinfo?(itiel_yn8)

Comment 10

6 months ago
Created attachment 8816052 [details]
History-RTL-Fixed.png

Updated

6 months ago
Depends on: 1321633
See Also: → bug 1321633

Updated

6 months ago
Depends on: 1321635
See Also: → bug 1321635

Comment 11

6 months ago
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #9)
> Hi ItielMaN , 
> 
> Thank you for your support! could you please file another 2 bugs mentioned
> in comment8, and share with us the bugID!
> 
> Thank you very much !

Done.
Flags: needinfo?(itiel_yn8)
Hi all,

Tested on latest Nightly (Arabic) and the results are here:
Phone:
- LG G4 (Android 5.1) - https://i.imgur.com/yKu00bZ.png
Tablet:
- Asus ZenPad 8 (Android 6.0.1) - https://i.imgur.com/Pg0KzGQ.png

We will test again after bug 1321635 and bug 1321633 are fixed.

Comment 13

4 months ago
This is verified fixed with the fixed from bug 1321635 and bug 1321633 too. 
Nightly AR 53.0a1 2017-01-17
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED

Comment 14

4 months ago
This was implemented and works correctly - further bugs that might arise will be linked for tracking.
Status: RESOLVED → VERIFIED
Blocks: 1319302
You need to log in before you can comment on or make changes to this bug.