894270 - [Buri][Call hold]The call hold screen display wrong.

Status: RESOLVED FIXED

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

Description

[sync-1]

AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.152
 Firefox os  v1.1
 Mozilla build ID:20130702230206
 
 Created an attachment (id=459501)
 pic
 
 DEFECT DESCRIPTION:
 [Call hold]The call hold screen display wrong.
  REPRODUCING PROCEDURES:
 1.Enable call waiting
 2.establish a call with A and B.
 3.C make a call to A,A hold the call,swap the call between B and C.
 4.the picture is not swap--KO1
 5.there is no call number and time--KO2
 
  EXPECTED BEHAVIOUR:
 KO1:The picture of B and C should be swap
 KO2:There should be call number and time
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 3/3
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Alex Keybl [:akeybl]]

Joe, what state is call holding supposed to be for 1.1? Is this intended design? Stephany suggested you would know.

Comment 2

[Wayne Chang [:wchang]]

Triage partners are not concerned about this being put to v1.2. Joe please minus if you confirm this is not spec'ed for 1.1

Comment 3

[Amelie Kong]

Attachment: pic.PNG

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

With the screenshot from comment 3, this bug is easier to understand. 

Base on what I am able to find from v1.1 UX visuals. Current implementation was not done per spec'ed. (Stephany can confirm on the UX design, hopefully i didn't look into the wrong folder for v1.1)
https://mozilla.app.box.com/files/0/f/991297523/1/f_7980848270
https://mozilla.app.box.com/files/0/f/991297523/1/f_7980848110
https://mozilla.app.box.com/files/0/f/991297523/1/f_7980847716
https://mozilla.app.box.com/files/0/f/991297523/1/f_7980847292

Comms team Product/EPM (MOZ+TEF) discussed this and given how much time we have left for v1.1, we'd like this to be considered for v1.2. Actually this should be considered together as part of the conf-call feature in v1.2.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

At this point we're not taking more blockers unless they're partner-championed or critical security/regression.

Comment 6

[Stephany Wilkes]

Removing the needinfo? flag since we've noted that the feature was not designed to spec and has since been moved to 1.2.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

batch update: waiting for conference call feature to be completed, then QA to verify these

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it links to koi-comms bug

Comment 9

[Germán Toro del Valle (:gtorodelvalle)]

Including the dependency on the conference call user story so that this bug is rechecked once the conference call one is resolved.

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

WIP
https://github.com/rexboy7/gaia/commit/290c9bfe00f68a528abeb32df6b2b22f070a805c

Since we switch calls by just holding active one, the call resumed doesn't go through "resuming" states before switching to "connected" state. "resuming" state is only raised when we call resume() explicitly.

Therefore I'm considering remove "resuming" case in handler since most of the process is the same as what "connected" do.

I'm working on CSS now with visual shown below:
https://bug887680.bugzilla.mozilla.org/attachment.cgi?id=795988

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: WIP Screenshot

And some WIP screenshot. Not sure if I need to get feedback from UX (Especially I'm not sure when to show/not to show incoming/outcoming/pause small icons). If I can't find and spec later I'll ni? someone.

Comment 12

[Stephany Wilkes]

Hi there, 

Please set another ni? for UX if this spec doesn't address the issue (it should):
https://mozilla.box.com/s/tipch2ziarstxcbnjvt1

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: wip screenshot

These are screenshots on the following proposed patch.

Notice that the font size of contacts name in multi-call screen is still bigger than proposed visual spec. It's because |HandledCall.formatPhoneNumber| set font-size directly on them rather than just using predefined css rule. I'm not sure if we have any specified consideration on this so I just keep it for now.

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: patch

The most significant change on Javascript part is that I removed "resuming" state, see comment 10. Most of the other works are css changes.

Hello Etienne, would you help review this patch too?

Comment 15

[Mingming ZHAO]

Hi, how do you deal with this situation: contact B has photo, but C has not, when I swap these two calls, I think when B is connected, C is held, it should display B's photo, when C is connected, B is held, it should display the wallpaper.

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Mingming ZHAO from comment #15)
> Hi, how do you deal with this situation: contact B has photo, but C has not,
> when I swap these two calls, I think when B is connected, C is held, it
> should display B's photo, when C is connected, B is held, it should display
> the wallpaper.

Makes sense. Sorry for didn't consider that case.
Let me figure out how to add it to patch.

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 817695 [details]
patch

Forwarding this CSS-heavy review to Rik :)

Comment 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

Updated per comment 15 and comment 16, handling cases of switching between contacts with/without photos. (as one additional commit)

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

After rebasing to latest master I found the patch doesn't show the CDMA switch button correctly.
Anthony I'll send review to you again once I fix this issue. Sorry for bothering.

Comment 20

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

Done, just a few css to keep CDMA case.
I have no real device to test so I just tested by hard-code something. They should be OK I think. We may call Gabriele if necessary.

Anthony may you help?

Comment 21

[Anthony Ricaud (:rik)]

Comment on attachment 817695 [details]
patch

I haven't reviewed the CSS nor tested the CDMA case. I'm just giving my partial feedback now so I'm not blocking you.

Overall, it's good. I have a few stylistics comments, one edge case that needs fixing and some tests modifications.

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

Ok, Just updated per suggestions. Thanks for helping.
Anthony may you take a look again?

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 817695 [details]
patch

Sorry for the slow review. I forgot to think about a test when we initially connect to a call. (There is one, but only test with a photo)

Comment 24

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

Thanks again. I also forgot to consider that case :/
for first call it doesn't matter but we need it for second incoming call.

Small changes due to last comments.

Comment 25

[Anthony Ricaud (:rik)]

Comment on attachment 817695 [details]
patch

I'm so sorry, I just noticed now that I have an error when I run call_screen_test.js alone:
  ReferenceError: MockNavigatorSettings is not defined
  at (anonymous) (http://communications.gaiamobile.org:8080/dialer/test/unit/call_screen_test.js:471)
I have no idea where this error comes from.

I also have a nit for the style of the new test.

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Sorry for didn't notice that error..
(I ran both call_screen and handled_call together so that it didn't appear.. weird)
Using Require() rather than requireApp() fixes it.
For the new test I have something to confirm before modifying the patch, see Github.
Thanks for the detailed review!

Comment 27

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 817695 [details]
patch

Updated per comments on Github. Thanks for the help!

Comment 28

[Anthony Ricaud (:rik)]

Comment on attachment 817695 [details]
patch

Excellent! Don't forget to squash and remove the extra debug code before landing.

Comment 30

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thanks for your help Rik!

Landed in master:
https://github.com/rexboy7/gaia/commit/ad79e2a2b3076c74d7ebbca402723e2a3d7254cb

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted ad79e2a2b3076c74d7ebbca402723e2a3d7254cb to:
v1.2: e4b10fdb0c9bd38c25a3fe16c75fd0c18068f5cb