Closed Bug 1116803 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S5 (6feb)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: Jamie_, Assigned: nefzaoui)

References

Details

Attachments

(8 files, 3 obsolete files)

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.
Flags: needinfo?(lebedel.delphine)
Flags: needinfo?(nhirata.bugzilla)
This bug was reported by a FirefoxOS Contributor who completed a One and Done task:
https://oneanddone.mozilla.org/en-US/tasks/90/
QA Whiteboard: [rtl-impact]
(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!
Assignee: nobody → nefzaoui
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(lebedel.delphine)
Blocks: contacts-rtl
Component: Simulator → Gaia::Contacts
Attachment #8547756 - Flags: ui-review?(swilkes)
Attachment #8547756 - Flags: review?(jmcf)
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
Attachment #8547756 - Flags: review?(jmcf) → review?(francisco)
Hi Ahmed, could you rebase your PR?
Flags: needinfo?(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.
Flags: needinfo?(nefzaoui)
Summary: [rtl] [contacts] when adding new contact the add field and remove field buttons are not coresponding to select → [rtl] [contacts] main two contacts app UIs: Contacts List and Add new contact page lack RTL support
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.
Attachment #8547756 - Flags: review?(francisco)
Attached image 2015-01-13-16-32-22.png
List view with FB information, with icon and company missaligned
Attached image 2015-01-13-16-27-51.png
Search view with cancel icon and fb information missaligned.
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!
Ahmed, can you please attach a screenshot? I can't flash the patch in Mexico with my almost nonexistent Internet access. Sorry!
Flags: needinfo?(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!
Flags: needinfo?(nefzaoui) → needinfo?(swilkes)
Attachment #8547756 - Flags: review?(francisco)
We will uplift this in 2.2 once fixed on master/central.
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!
Flags: needinfo?(nefzaoui)
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.
Flags: needinfo?(swilkes) → needinfo?(cawang)
Attachment #8547756 - Flags: ui-review?(swilkes) → ui-review+
Attachment #8547756 - Flags: ui-review?(cawang)
Flags: needinfo?(cawang)
Attachment #8547756 - Flags: ui-review?(cawang) → ui-review-
Attached image 2015-01-21-18-01-30.png
Hi, 

The add buttons cover the some texts. Please check the attached photo. Thanks!
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.
Attachment #8547756 - Flags: review?(francisco) → review+
RTL triage: P2 -- will make a best effort to get this into the 2.2 release.
Priority: -- → P2
I strongly suggest this becomes a P1. TBH I don't see how a bug that fixes a whole module (contacts) is a P2 :)
Flags: needinfo?(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!
Flags: needinfo?(cawang)
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.
Attachment #8547756 - Flags: review+ → review?(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?
Attachment #8547756 - Flags: review?(francisco) → review+
Duplicate of this bug: 1124585
Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Carrie can you recheck?

Thanks!
Attachment #8547756 - Flags: ui-review- → ui-review?
RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)
feature-b2g: --- → 2.2+
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!
Flags: needinfo?(cawang)
Attachment #8547756 - Flags: ui-review? → ui-review-
Hi Ahmed,

could you try to fix last comment from Carrie?

Thanks a lot!
Flags: needinfo?(nefzaoui)
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!
Flags: needinfo?(cawang)
Rebased and updated my patch again.
Though as Stephany said, it is right aligned already :)
Thanks
Flags: needinfo?(nefzaoui)
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!
Attachment #8547756 - Flags: ui-review- → ui-review?(cawang)
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
(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!! :)
Flags: needinfo?(nhirata.bugzilla)
Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/27328
Flags: needinfo?(cawang)
Attachment #8547756 - Flags: ui-review?(cawang) → ui-review-
(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!
Flags: needinfo?(cawang)
Attached image 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.
Flags: needinfo?(nhirata.bugzilla)
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
(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!
ni Fang for visual spec (RTL/ LTR).
Flags: needinfo?(cawang) → needinfo?(fshih)
Attached image 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!
Flags: needinfo?(fshih)
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!
Attachment #8547756 - Flags: ui-review- → ui-review?(cawang)
Comment on attachment 8547756 [details] [review]
[PullReq] anefzaoui:bug-1116803 to mozilla-b2g:master

Perfect! Well done! Thanks!
Attachment #8547756 - Flags: ui-review?(cawang) → ui-review+
(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!
Keywords: checkin-needed
Flags: needinfo?(jmcf)
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Oh, must have been fixed then!
Sorry for the noise.
Flags: needinfo?(jmcf)
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
Attachment #8547756 - Flags: approval-gaia-v2.2?
Duplicate of this bug: 1126208
Duplicate of this bug: 1126388
Attachment #8547756 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Duplicate of this bug: 1126883
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
Duplicate of this bug: 1126384
Duplicate of this bug: 1127360
Duplicate of this bug: 1127325
Duplicate of this bug: 1126914
Duplicate of this bug: 1127298
Duplicate of this bug: 1104709
Duplicate of this bug: 1107517
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [rtl-impact] → [rtl-impact],[MGSEI-Triage+]
Attached image Verify_RTL_1.png (obsolete) —
Attached image Veriy_RTL_2.png (obsolete) —
Attached image Verify_RTL_3.png (obsolete) —
Attachment #8560285 - Attachment is obsolete: true
Attachment #8560284 - Attachment is obsolete: true
Attachment #8560283 - Attachment is obsolete: true
Status: VERIFIED → RESOLVED
Closed: 5 years ago5 years ago
(In reply to Coler from comment #59)
> This issue has been verified successfully on Flame 2.2/3.0
Ignore this comment. Thanks.
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
QA Whiteboard: [rtl-impact],[MGSEI-Triage+] → [QAnalyst-Triage?][rtl-impact][MGSEI-Triage+][failed-verification]
Flags: needinfo?(pbylenga)
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?][rtl-impact][MGSEI-Triage+][failed-verification] → [QAnalyst-Triage?][rtl-impact][MGSEI-Triage+]
Depends on: 1127360
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?][rtl-impact][MGSEI-Triage+] → [QAnalyst-Triage+][rtl-impact][MGSEI-Triage+]
Flags: needinfo?(pbylenga)
Duplicate of this bug: 1126883
You need to log in before you can comment on or make changes to this bug.