1131490 - [RTL][Dialer]The number of calls handled is shown at the right of call number.

Status: VERIFIED FIXED

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

Description

[Sue]

Attachment: screenshot.png

[1.Description]:
[RTL][Dialer]The number of calls handled in call log is not shown at the left of call number, it should be shown like " 10010(2) ".
See attachment:screenshot.png

[2.Testing Steps]: 
Prerequisite: Set system language as Arabic and insert a SIM card.
1. Launch Phone app.
2. Make a call.(suck as call to "10010").
3. Hang up the call and call 10010 again.
4. Hang up the call and observe the call log.

[3.Expected Result]: 
4. The number of calls handled should be shown at the left of call number. (such as " (2)10010" )

[4.Actual Result]: 
4. The number of calls handled is shown at the right of call number. (Shown as " 10010(2) " )

[5.Reproduction build]: 
Flame 2.2 build:
Build ID               20150209002504
Gaia Revision          e827781324cbde91d2434b388f5dead3303a85ee
Gaia Date              2015-02-06 20:54:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0552759956d3
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150209.040038
Firmware Date          Mon Feb  9 04:00:51 EST 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Sue]

Hi Nefzaoui,
Could you help to confirm if this is in fact a bug? Thanks!

Comment 2

[Dylan Oliver [:doliver]]

triage: P2

Confirmed that the desired format is "(2) [phone]"

Comment 3

[Dylan Oliver [:doliver]]

Re-triage: removing minor bugs from 2.2 scope. We'll still take a fix for this if 2.2 approval is granted before Feb 23.

Comment 4

[Stephany Wilkes]

Setting ni? to Kaze instead of Ahmed to spread the bug load a bit more evenly.

Comment 5

[Stephany Wilkes]

This is definitely a bug but is not a blocker. Kaze, if you can't take this, let me know and I'll raise this to the Comms team.

Comment 6

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

Stephany should we make it a P3 then?

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] autra:bug-1131490 > mozilla-b2g:master

Comment 8

[Augustin Trancart [:autra]]

Comment on attachment 8567202 [details] [review]
[gaia] autra:bug-1131490 > mozilla-b2g:master

Hi Gabriel, 

Could you review this please?

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

I'm a bit puzzled by your change, this is effectively reversing the change from bug 1102344 which would likely reintroduce that bug.

Comment 10

[Augustin Trancart [:autra]]

Hi Gabriel,

I'm puzzled as well, but I think this is due to bug #1102340 having landed at that same moment. When span.primary-info-main and span.retry-count enclose a bdi, the order is wrong (number is to the left and call count to the right in RTL). When only span.primary-info-main enclose a bdi, thinks are displayed in the right order (without reproducing bug #1102344). But thinking about it, this looks like a dangerous corner-case to be based on. 

So, I think that to make things properly, both of them should have a dir="ltr", as they will always be ltr. But in this case, the js should reverse the order of insertion for rtl to have the call count displayed left. Does it looks like an acceptable way of doing things to you? I'll submit another pull request with this.

Comment 11

[Augustin Trancart [:autra]]

Actually I do reproduce the bug without the bidi.

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] autra:bug-1131490-2 > mozilla-b2g:master

Comment 13

[Augustin Trancart [:autra]]

Comment on attachment 8568026 [details] [review]
[gaia] autra:bug-1131490-2 > mozilla-b2g:master

Ok I just took 5 min to think a bit about it, I try to do things logically and "semantically" and here is another PR:
- I keep bdi around numbers, but I also add bdi around names. After all, names also can be bdi! You can have name in both english and arab in RTL context.
- I removed bdi around call count. I don't see why it should have bdi.

And it actually works! According to my tests, both RTL and LTR languages with numbers or contact info. 

The bug with bdi is because updateContactInfo was not adding the bdi around the contact name (so it was removed). 

That's the only way I managed to make it works properly. No combinations of dir ltr, rtl or auto have worked for me. But I don't say I have tested everything, so feel free to suggest anything else! Thanks.

Comment 14

[Stephany Wilkes]

Let's leave this as a P2. It's still bigger and more important than P3s. Thanks - great progress here.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8568026 [details] [review]
[gaia] autra:bug-1131490-2 > mozilla-b2g:master

(In reply to Augustin Trancart [:autra] from comment #13)
> - I keep bdi around numbers, but I also add bdi around names. After all,
> names also can be bdi! You can have name in both english and arab in RTL
> context.
> - I removed bdi around call count. I don't see why it should have bdi.

Both sound right, the latter was added to prevent the parentheses from going astray but with everything wrapped into bdi I think it's not needed anymore.

> And it actually works! According to my tests, both RTL and LTR languages
> with numbers or contact info. 
> 
> The bug with bdi is because updateContactInfo was not adding the bdi around
> the contact name (so it was removed).

Yes, I think that was the core of the problem. I've tested it on my phone and switching between English and Arabic using the reference workload for the call log gives consistent, correct results (from what I can tell). Thanks!

Comment 16

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/28367

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 17

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Autolander from comment #16)
> Autolander could not land the pull request due to not having collaborator
> rights. This is possibly due to a tree closure. Please check the tree status
> and request checkin again once the tree is open.

Gaia is closed right now, we'll have to try again once it gets reopened.

Comment 18

[Augustin Trancart [:autra]]

NI myself to retry later

Comment 19

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#5C7EplCORjeTsN7pUfK-Lw

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 20

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#j4j_wnhkRi-RQNo_OxfgjA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 21

[Augustin Trancart [:autra]]

Comment on attachment 8568026 [details] [review]
[gaia] autra:bug-1131490-2 > mozilla-b2g:master

Hi Gabriel,

I'm flipping the r flag again, because I let some jshint go through and I've just corrected them. Thanks!

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8568026 [details] [review]
[gaia] autra:bug-1131490-2 > mozilla-b2g:master

Always blame the reviewer for things like this ;-) I checked the tests but forgot to run the linter.

Comment 23

[Augustin Trancart [:autra]]

I recently changed my machine and forgot to install the linter to run on commit, so I also blame myself :-)

But if you insist: shame on you!

Thanks for the review anyway ;-)

Comment 24

[GitHub Autolander Bot]

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

Comment 25

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly 3.0 build.  Leaving a verifyme tag for whenever this gets uplfited on 2.2.
	
Actual results:  The number of calls is on the left instead of the right.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150226010233
Gaia: 7894b929f1b0394f3c997f72a6482bc7813e758d
Gecko: dd6353d61993
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 26

[SandKing]

Attachment: Fail log: logcat_1115.txt

This issue has been verified fial on latest Flame 2.2,
See log: "logcat_1115.txt".
See picture:"2015-02-27-11-15-02.png"

Flame 2.2 bulid:
Build ID               20150226162510
Gaia Revision          eb6a5ac9081d3962198e0f4520b0743d716d7a27
Gaia Date              2015-02-26 17:25:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1be266320634
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150226.195624
Firmware Date          Thu Feb 26 19:56:35 EST 2015
Bootloader             L1TC000118D0

Comment 27

[SandKing]

Attachment: Fail on Flame2.2 picture: 2015-02-27-11-15-02.png

Comment 28

[Augustin Trancart [:autra]]

It has not been uplifted on 2.2, if I'm not mistaken

Comment 29

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Augustin Trancart [:autra] from comment #28)
> It has not been uplifted on 2.2, if I'm not mistaken

No, it hasn't. Let me ask for approval.

Comment 30

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8568026 [details] [review]
[gaia] autra:bug-1131490-2 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: As per title, the number of calls is shown in the wrong place in RTL mode
[Testing completed]: Tested on a device and on the emulator using the Arabic locale
[Risk to taking this patch] (and alternatives if risky): The usual risks involved in RTL patches, possible UI regressions when dealing with corner cases, in this case however the change seems small enough that it should carry limited risk
[String changes made]: None

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Sue]

Attachment: 2015-03-05-15-59-34.png

This issue has been verified successfully on Flame 2.2
The number of calls handled shown at the left of call number. (such as " (5)10010")
See attachment:2015-03-05-15-59-34.png
Rate:0/5

Flame 2.2 build:
Build ID               20150304002529
Gaia Revision          8b4b3e4b7e7c308764f71542437fd60625ac6b75
Gaia Date              2015-03-03 15:01:17
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2cb52b7cda5a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150304.042036
Firmware Date          Wed Mar  4 04:20:47 EST 2015
Bootloader             L1TC000118D0

Comment 33

[Sue]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15428/