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

VERIFIED FIXED in 2.2 S7 (6mar)

Status

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: lulu.tian, Assigned: autra)

Tracking

unspecified
2.2 S7 (6mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Posted image 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
Reporter

Comment 1

4 years ago
Hi Nefzaoui,
Could you help to confirm if this is in fact a bug? Thanks!
QA Whiteboard: [rtl-impact]
Flags: needinfo?(nefzaoui)
triage: P2

Confirmed that the desired format is "(2) +861234567"
feature-b2g: --- → 2.2+
Priority: -- → P2
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.
feature-b2g: 2.2+ → ---

Comment 4

4 years ago
Setting ni? to Kaze instead of Ahmed to spread the bug load a bit more evenly.
Flags: needinfo?(nefzaoui) → needinfo?(fabien)

Comment 5

4 years ago
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.
Stephany should we make it a P3 then?
Flags: needinfo?(swilkes)
Assignee

Updated

4 years ago
Assignee: nobody → augustin.trancart
Assignee

Comment 8

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

Hi Gabriel, 

Could you review this please?
Flags: needinfo?(fabien)
Attachment #8567202 - Flags: review?(gsvelto)
I'm a bit puzzled by your change, this is effectively reversing the change from bug 1102344 which would likely reintroduce that bug.
Assignee

Comment 10

4 years ago
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.
Flags: needinfo?(gsvelto)
Assignee

Comment 11

4 years ago
Actually I do reproduce the bug without the bidi.
Assignee

Comment 13

4 years ago
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.
Attachment #8568026 - Flags: review?(gsvelto)

Comment 14

4 years ago
Let's leave this as a P2. It's still bigger and more important than P3s. Thanks - great progress here.
Flags: needinfo?(swilkes)
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!
Flags: needinfo?(gsvelto)
Attachment #8568026 - Flags: review?(gsvelto) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
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.
(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.
Assignee

Comment 18

4 years ago
NI myself to retry later
Flags: needinfo?(augustin.trancart)
Assignee

Updated

4 years ago
Flags: needinfo?(augustin.trancart)
Keywords: checkin-needed
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.
Assignee

Updated

4 years ago
Keywords: checkin-needed
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.
Assignee

Updated

4 years ago
Attachment #8567202 - Attachment is obsolete: true
Attachment #8567202 - Flags: review?(gsvelto)
Assignee

Comment 21

4 years ago
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!
Attachment #8568026 - Flags: review+ → review?(gsvelto)
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.
Attachment #8568026 - Flags: review?(gsvelto) → review+
Assignee

Comment 23

4 years ago
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 ;-)
Assignee

Updated

4 years ago
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [rtl-impact] → [Qanalyst-triage?][rtl-impact]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [Qanalyst-triage?][rtl-impact] → [Qanalyst-triage+][rtl-impact]
Flags: needinfo?(ktucker)

Comment 26

4 years ago
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

Updated

4 years ago
QA Whiteboard: [Qanalyst-triage+][rtl-impact] → [Qanalyst-triage+][rtl-impact][MGSEI-triage+]
Keywords: verifyme
Assignee

Comment 28

4 years ago
It has not been uplifted on 2.2, if I'm not mistaken
(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 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
Attachment #8568026 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8568026 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Reporter

Comment 32

4 years ago
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
Reporter

Updated

4 years ago
Reporter

Comment 33

4 years ago
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15428/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.