1116803 - [rtl] [contacts] main two contacts app UIs: Contacts List and Add new contact page lack RTL support

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P2)

Description

[Jamie Charlton (:Jamie_)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141215134203

Steps to reproduce:

1.) open contacts
2.) select to add new contact
3.) edit available fields


Actual results:

when set up for rtl the actuall places to press do not corespond with the obects designated for press they are partialy set to the left of the buttons.


Expected results:

the area should accuratly be clickable on the object to add/remove field.

Comment 1

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

This bug was reported by a FirefoxOS Contributor who completed a One and Done task:
https://oneanddone.mozilla.org/en-US/tasks/90/

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to charja13 from comment #0)
> User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:36.0) Gecko/20100101
> Firefox/36.0
> Build ID: 20141215134203
> 
> Steps to reproduce:
> 
> 1.) open contacts
> 2.) select to add new contact
> 3.) edit available fields
> 
> 
> Actual results:
> 
> when set up for rtl the actuall places to press do not corespond with the
> obects designated for press they are partialy set to the left of the buttons.
> 
> 
> Expected results:
> 
> the area should accuratly be clickable on the object to add/remove field.

Hey there,
Jumping in to say thank you for filing the bug :)
Even though I'm still not totally understanding the issue but I might have a clue, so I'll be working on it.
So far, the whole Add Contact UI is shifted slightly to the right and the plus buttons are misplaced.
Thanks!

Comment 3

[GitHub Autolander Bot]

Attachment: [PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Comment 4

[Jose Manuel Cantera]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

handing over to Francisco as I've never coded RTL stuff and he's more familiar with

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Hi Ahmed, could you rebase your PR?

Comment 6

[Ahmed Nefzaoui [:Nefzaoui]]

Done, extended code to things I missed before rebasing, also added support for RTL in main interface, since it's fairly small code and no other bug filed for it.
UI review is still needed.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Incredible job, thanks for removing lot's of code that we added unnecessarily before knowing about -moz-padding-start/end

CSS wide looks good to me, just found 2 minor errors for those views:

- When we have a FB contact, the FB icon is not aligned with the name, also the company name is just next to the icon, losing the space.

- Search view is part of the list view too, it clones the items, so if you enter the search view you'll see that the 'cancel' icon is not right, also the fb icon and company are missaligned.

We could do the search view in a different PR, but correcting the FB icon and company will be great in this PR.

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: 2015-01-13-16-32-22.png

List view with FB information, with icon and company missaligned

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: 2015-01-13-16-27-51.png

Search view with cancel icon and fb information missaligned.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Was checking the list in select mode (go to export contacts) or single pick up mode (go to set ice, or try to get a contact from an activity like sms) and it's looking pretty nice.

Great work, this is pretty close to land!

Comment 11

[Stephany Wilkes]

Ahmed, can you please attach a screenshot? I can't flash the patch in Mexico with my almost nonexistent Internet access. Sorry!

Comment 12

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Done, addressed the issues and fixed couple of others that I realized I forgot to address at first.

Stephany, how does it look now? Should I add screenshots or the PR would do it?

Thanks!

Comment 13

[bhavana bajaj [:bajaj]]

We will uplift this in 2.2 once fixed on master/central.

Comment 14

[Stephany Wilkes]

Ahmed, please provide screenshots wherever you can. I'm having a battery issue with my phone and it keeps dying, so until I can fix it I am having a hard time reviewing patches. Thank you!

Comment 15

[Stephany Wilkes]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

OK! Finally got this patch (naturally, since I just commented that I could not). Flagging Carrie as an FYI.

Comment 16

[Carrie Wang [New account: [e-mail]]]

Attachment: 2015-01-21-18-01-30.png

Hi, 

The add buttons cover the some texts. Please check the attached photo. Thanks!

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Excellent job, lgtm, please take a look to Carrie's comment. I've tried the add contact form, and looks good to me though.

Comment 18

[Dylan Oliver [:doliver]]

RTL triage: P2 -- will make a best effort to get this into the 2.2 release.

Comment 19

[Ahmed Nefzaoui [:Nefzaoui]]

I strongly suggest this becomes a P1. TBH I don't see how a bug that fixes a whole module (contacts) is a P2 :)

Comment 20

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Carrie Wang [:carrie] from comment #16)
> Created attachment 8552336 [details]
> 2015-01-21-18-01-30.png
> 
> Hi, 
> 
> The add buttons cover the some texts. Please check the attached photo.
> Thanks!

Hey Carrie,
I just rebased the PR, though it works perfectly fine on my end. Could you please try it again on master?
Thanks!

Comment 21

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

And kindly requesting another r? since I made couple other fixes i wasn't paying attention to.

Comment 22

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Looking pretty good.

Just relaunched an integration test that was failing, but seems it's not related to this PR.

Carrie, would you kindly review the UX for this patch?

Comment 24

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Carrie can you recheck?

Thanks!

Comment 25

[Dylan Oliver [:doliver]]

RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)

Comment 26

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Hi, 

It looks good but the add and delete buttons are not right-aligned. :(
Thanks!

Comment 27

[Francisco Jordano [:arcturus] [:francisco]]

Hi Ahmed,

could you try to fix last comment from Carrie?

Thanks a lot!

Comment 28

[Stephany Wilkes]

Hi Carrie - It looks as if the add and delete buttons are right aligned (from your screenshot) but that there are some overlap problems with text. Is that the issue? Just checking, since Ahmed and I see them on the right. Thanks!

Comment 29

[Ahmed Nefzaoui [:Nefzaoui]]

Rebased and updated my patch again.
Though as Stephany said, it is right aligned already :)
Thanks

Comment 30

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Reasking Carrie for ui review.

We can do any minor problem in follow ups, but having this landed soon will help us to start working over a good base.

Thanks!

Comment 31

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

There's still some issues with the contacts

* [RTL] clear button for a new contact is on the wrong side for first name, last name company
* [RTL] street name, comment for a new contact starts on the wrong side
* [RTL] Current drop down is set for city? in a new contact

Comment 32

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #31)
> There's still some issues with the contacts
> 
> * [RTL] clear button for a new contact is on the wrong side for first name,
> last name company
> * [RTL] street name, comment for a new contact starts on the wrong side
> * [RTL] Current drop down is set for city? in a new contact

Hey Naoki,
I'm not quite sure if i can reproduce or understand any of your concerns, could you please attach a screenshot with all the bugs pointed at?
Thanks so much!! :)

Comment 33

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/27328

Comment 34

[Carrie Wang [New account: [e-mail]]]

Attachment: 2015-01-30-18-02-07.png

Comment 35

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Carrie Wang [:carrie] from comment #34)
> Created attachment 8557026 [details]
> 2015-01-30-18-02-07.png

Carrie,
I don't see any issue with this screenshot.
If you mean the "add" and "delete" buttons, they are respectively in their correct position since in the original LTR UI they're on the left edge of the screen, behind the select element and text.
Thus, this means they would still be behind those elements in RTL too, i.e. the "behind" position is the right, not left in RTL. In other words, what's on the left edge of the screen in LTR becomes on the right edge in RTL.
Thus, their position is correct :)
Please, let me know what do you think.
Thanks!

Comment 36

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: 2015-01-30-12-52-03.png

Oh ya, Ahmed... you're working on master, while I was looking at 2.2

Doing the retesting on master:
1) I don't see the issue that Carrie is seeing.  I think this is because the + icon is on the wrong side?

2) I still do see the issue of the clear text field being on the wrong side for at least the first 3 fields.

In order to see the issue, you have to click into the fields.  See the first field in the screenshot.

Comment 37

[Stephany Wilkes]

There is something really odd going on here. Ahmed, Carrie and I see the same thing, but Naoki does not. Something must be going on with flashing, no?
https://bug1116803.bugzilla.mozilla.org/attachment.cgi?id=8557026

Comment 38

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #36)
> Created attachment 8557197 [details]
> 2015-01-30-12-52-03.png
> 
> Oh ya, Ahmed... you're working on master, while I was looking at 2.2
> 
> Doing the retesting on master:
> 1) I don't see the issue that Carrie is seeing.  I think this is because the
> + icon is on the wrong side?
> 
> 2) I still do see the issue of the clear text field being on the wrong side
> for at least the first 3 fields.
> 
> In order to see the issue, you have to click into the fields.  See the first
> field in the screenshot.

Naoki I'm sorry to be a pain but could you please get the latest version of my patch and re apply it?
I'm pretty sure I did address everything that seemed broken there already, so it might be a matter of patch's version?

Thanks!

Comment 39

[Carrie Wang [New account: [e-mail]]]

ni Fang for visual spec (RTL/ LTR).

Comment 40

[Fang Shih [:fang]]

Attachment: Contact_RTL.001.jpg

Attached the screenshots of the difference between current RTL build and a correct version of it. The red button should move right aligned with white button. Thanks!

Comment 41

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

(In reply to Fang Shih [:grasspizza] from comment #40)
> Created attachment 8557768 [details]
> Contact_RTL.001.jpg
> 
> Attached the screenshots of the difference between current RTL build and a
> correct version of it. The red button should move right aligned with white
> button. Thanks!

Thanks Fang!
Fixed.

Carrie, please review?
Thanks!

Comment 42

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Perfect! Well done! Thanks!

Comment 43

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Carrie Wang [:carrie] from comment #42)
> Comment on attachment 8547756 [details] [review]
> [PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master
> 
> Perfect! Well done! Thanks!

Thanks :)

(In reply to Jose Manuel Cantera from comment #4)
> Comment on attachment 8547756 [details] [review]
> [PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master
> 
> handing over to Francisco as I've never coded RTL stuff and he's more
> familiar with

Hey Jose could you please throw in a r+ for the patch since Autolander won't land it without a granted review from the first assigned reviewer to the patch ?
Thanks!

Comment 44

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c800d5d86c4d93b608f70165a59a0160398c0d89

Comment 45

[Ahmed Nefzaoui [:Nefzaoui]]

Oh, must have been fixed then!
Sorry for the noise.

Comment 46

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): lack of RTL in the OS
[User impact] if declined: broken Contacts app
[Testing completed]: Yes, device: flame, and merge-able.
[Risk to taking this patch] (and alternatives if risky): no risks as it only affects RTL-related code.
[String changes made]: No string changed

Comment 49

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/ac3179f4a49f5fde05025ad7d73986160ad7e518

Comment 51

[Jamie Charlton (:Jamie_)]

I still am able to repo this on both the nightly xpi and the nightly on the flame also

WebIDE
appid   	{3c2e2abc-06d4-11e1-ac3b-374f68613e61}
apptype  	b2g
vendor  	Mozilla
name    	B2G
version 	2.2.0.0-prerelease
appbuildid	20150204002509
platformbuildid	20150204002509
platformversion	37.0a2
geckobuildid	20150204002509
geckoversion	37.0a2
changeset	8669c26fd4a5
locale  	en-US
os      	B2G
hardware	
processor	x86_64
compiler	gcc3

Flame
appid   	{3c2e2abc-06d4-11e1-ac3b-374f68613e61}
apptype  	b2g
vendor  	Mozilla
name    	B2G
version 	2.2.0.0-prerelease
appbuildid	20150204002509
platformbuildid	20150204002509
platformversion	37.0a2
geckobuildid	20150204002509
geckoversion	37.0a2
changeset	8669c26fd4a5
locale  	en-US
os      	B2G
hardware	qcom
processor	arm
compiler	eabi
brandName	
channel  	nightly-b2g37

Comment 59

[Coler]

This issue has been verified successfully on Flame 2.2/3.0
Reproduce rate:0/5
Attachments:Verify_RTL_1.PNG,Verify_RTL_2.PNG,Verify_RTL_3.PNG

Flame 2.2 build:

Build ID               20150205002503
Gaia Revision          c2047a46e29696238e9b4c9caaba47736421449a
Gaia Date              2015-02-04 20:34:04
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/adfba0a07e9b
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150205.045043
Firmware Date          Thu Feb  5 04:50:54 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:

Build ID               20150205010209
Gaia Revision          2b83a6d5d1185a438b5bbd287497ac2743b501db
Gaia Date              2015-02-04 21:30:59
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/34a66aaaca81
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150205.044014
Firmware Date          Thu Feb  5 04:40:24 EST 2015
Bootloader             L1TC000118D0

Comment 60

[Coler]

Attachment: Verify_RTL_1.png

Comment 61

[Coler]

Attachment: Veriy_RTL_2.png

Comment 62

[Coler]

Attachment: Verify_RTL_3.png

Comment 63

[Coler]

(In reply to Coler from comment #59)
> This issue has been verified successfully on Flame 2.2/3.0
Ignore this comment. Thanks.

Comment 64

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This bug is failing verification on Flame 3.0 and Flame 2.2

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150206010204
Gaia: 94af4b42d2ace6c9f38f31de77240604fac68af1
Gecko: 7c5f187b65bf
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150206002505
Gaia: a52999ce7f783177deb17e267bf003a53e6fde06
Gecko: 01446d5231ef
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Things that are fixed:
- The "+" icon to add new subject lines is not overlapping any text anymore
- The "x" icon to remove subject lines is alligned properly with the "+" icons
- The "x" icon to delete text in a textbox is located on the left side of the textbox, and does not overlap any text
- All names in the list of contacts are properly right aligned

The issue that is blocking the verification of this bug is that the Facebook Icon is not properly aligned on the right and causes it to be put very close to the "company" section with no space between. A screenshot of this issue is attached

Comment 65

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Facebook Icon Alignment Failure

Comment 66

[Johan Lorenzo [:jlorenzo]]

Thank for this verification Derek. Let's track this Facebook icon issue in bug 1127360 which was duplicate against this one. I'll reopen it.

Switching 2.2 and 3.0 to verified per comment 64 (minus Facebook of course).