mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.47k stars 1.28k forks source link

Find and report RTL problems #5497

Closed liuche closed 4 years ago

liuche commented 4 years ago

Why/User Benefit/User Problem

As a user, I want to use the browser in my preferred RTL language and have a good experience.

What/Requirements

App doesn't look broken (In order to test, you can set the Android System language to an RTL language, and even though the strings won't change, the app will switch to RTL direction.)

Acceptance Criteria (how do I know when I’m done?)

This issue is done when we have issues filed for the RTL issues. Since these screenshots are all listed here, the fixes can be against this issue as well. We are NOT doing adding RTL languages, that's #5496

┆Issue is synchronized with this Jira Task

liuche commented 4 years ago

@Delphine here's the bug to track RTL investigation for Reza :)

5y commented 4 years ago

image

liuche commented 4 years ago

I think these all seem like reasonable items that we should fix! I'm going to throw an eng:ready label on this.

mcarare commented 4 years ago

@liuche For b) - the icon is mirrored correctly, because the icon is aligned to end in normal mode (LTR). It is reversed from the save to collection text where it is aligned to start. So IMO the RTL version is correct.

mcarare commented 4 years ago

@5y Can you please retest "e) Find on the page button is out of the page." I cannot reproduce this. TY!

brampitoyo commented 4 years ago

I also agreed with @liuche that all of these issues look reasonable, without requiring any more UX feedback since what we need to do is already very clear.

mcarare commented 4 years ago

@brampitoyo see my comment about c).

5y commented 4 years ago

@mcarare Considering the C, The icon is fine. the textbox should be aligned.

mcarare commented 4 years ago

@5y Thank you for your input! 👍 Actually, I wanted to ask about b). I already fixed c) And if e) Find on the page button is out of the page. is still reproducible

5y commented 4 years ago

@mcarare You are welcome. d- Solved. Regarding the "b", If we change the strings with RTL language strings such as Persian, It is more user-friendly for the users to have icon on the right side of the text.

cadeyrn commented 4 years ago

I found another problem in the URL bar at the bottom. The conflict between the icon and the URL bar text.

This is fixed since #6016.

brampitoyo commented 4 years ago

@mcarare wrote:

see my comment about c).

The fix you proposed on #6152 looks great to me. Is there a specific feedback that you’re looking for?

@5y wrote:

Regarding the "b", If we change the strings with RTL language strings such as Persian, It is more user-friendly for the users to have icon on the right side of the text.

I agree. The “No collection” icon should be placed on the right side.

mcarare commented 4 years ago

@brampitoyo So, you want to keep the position in RTL same as LTR (to the right of text)? IMO mirroring some layouts partially it's not the greatest idea. There are no other layouts in this screen that we don't mirror.

brampitoyo commented 4 years ago

@mcarare Sorry. I think I’m just a little unclear as to the fix that you’re suggesting.

Would you mind posting a mockup of your proposed fix? I don’t have enough experience with RTL layout to know that mirroring is sometimes correct, and sometimes incorrect.

What would your suggestion be?

mcarare commented 4 years ago

@brampitoyo My suggestion is leave it as it is, because IMO it is correct.

LTR:RTL:

5y commented 4 years ago

@mcarare Yes, We can leave it as it is. However, Align the icons on the right side of the RTL language is more userfriendly, Because we usually align them on the right side of the words, phrases ...

liuche commented 4 years ago

Hi @mcarare if this is in progress, could you move it to the In Progress column in the kanban? That way it's easy to see what work is available to work on. (And move it back + unassign if you stop working on it).

5y commented 4 years ago

@liuche @mcarare Please let me know if I can be of further assistance on this.

mcarare commented 4 years ago

@liuche It's not in progress, I've already fixed some issues, and waiting for UX feedback on the others. IMO this issue is ready for QA.

brampitoyo commented 4 years ago

@mcarare @5y Hi Mihai and Reza,

Icons inside start page sections are arranged in a special way:

Compared to icons in menus, dialogs and other pages:

I agree that the start page icon arrangement is different from the rest, but it was also designed intentionally.

For consistency, we should follow this special arrangement in RTL languages, as @mcarare suggested above on https://github.com/mozilla-mobile/fenix/issues/5497#issuecomment-549716100

Therefore, if the LTR layout is:

Then the RTL layout should be:

And the copy underneath that says “Collect the things that matter to you” should be right-aligned.

5y commented 4 years ago

@brampitoyo Basically all of the icons in the Menu's should be aligned to the right, It means we have icons first and then the text. The order from Right-To-Left is : [RTL-aligned text ] [icon].

Considering " Save to Collection", the RTL version that you mentioned is correct:

image

mcarare commented 4 years ago

For QA: Please also check on multiple devices if this is reproducible:

e) Find on the page button is out of the page. < is reproducible

Thank you!

sv-ohorvath commented 4 years ago

Verified all as fixed on Nightly 12/2. Devices: Samsung Galaxy Tab 3 (Android 8.0), Mi 4i (Android 5.0.2), Samsung Galaxy S8 (Android 9)