1007709 - [Dialer][Call Screen] Create fluid size for contact name in the upper information panel

Status: VERIFIED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Spec: Call activecall fluidtext

Create a dynamic size for contact's full name in Incomming call and Active Call so to avoid cutting long names.

Spec is attached.

Comment 1

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

Vicky, the upcoming screenshots may burn your eyes... Don't say I did not warn you... :-p

Comment 2

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

Attachment: callscreen_contact_outgoing_max_font_size.png

Comment 3

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

Attachment: callscreen_contact_incoming_min_font_size.png

Comment 4

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

Attachment: callscreen_callwaiting_contact_incoming_min_font_size.png

BTW, to cover all the possible cases you may also want to prepare a spec (or just tell me the values) for the maximum and minimum font sizes for the second incoming call (aka. call waiting). You know, the footer which appears at the bottom in this screenshot.

Comment 5

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

Need-infoing Vicky regarding comment 4 ;)

Comment 6

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

Attachment: callscreen_callwaiting_contact_connected_2_min_font_size.png

Last but not least, it would be also great if you could provide me with the maximum and minimum font sizes for the call waiting case (please see the attached screenshot). Reusing the same values used in the case of the 1 to 1 call (this is 3.4 rem and 2.3rem) would ease things a lot :-p

Comment 7

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8419501 [details]
callscreen_contact_incoming_min_font_size.png

Font size minumum: OK. Rest of screen not ok.

Comment 8

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

Hi Vicky, it seems your ui-review+ removed the need-info I had set :-O

Resetting need-info regarding comments 4 and 6 ;-) Thanks!

Comment 9

[Victoria Gerchinhoren [:vicky]]

(In reply to Germán Toro del Valle from comment #4)
> Created attachment 8419505 [details]
> callscreen_callwaiting_contact_incoming_min_font_size.png
> 
> BTW, to cover all the possible cases you may also want to prepare a spec (or
> just tell me the values) for the maximum and minimum font sizes for the
> second incoming call (aka. call waiting). You know, the footer which appears
> at the bottom in this screenshot.

Please use the same minimum value as the snigle screen one, since it is a matter of legibility and visual hierarchy between the name and the carrier/type of phone number.

Thanks!

Comment 11

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

Attachment: callscreen_callwaiting_contact_incoming_min_font_size.png

I used the same minimum font size as requested.

Please, do not focus on concrete details of the appearance since they will be solved in the Call Screen bug, this is bug 951606.

Comment 12

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

Attachment: callscreen_callwaiting_contact_connected_2_min_font_size.png

I used the same minimum font size as requested.

Please, do not focus on concrete details of the appearance since they will be solved in the Call Screen bug, this is bug 951606.

Comment 13

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

Attachment: 19171.html

Comment 14

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8421158 [details]
callscreen_callwaiting_contact_connected_2_min_font_size.png

Please, align the name and timestamps baselines.

Comment 15

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8421157 [details]
callscreen_callwaiting_contact_incoming_min_font_size.png

Font size is ok, but please, set the maximum width of the text area 2 rem before the icon.

Comment 16

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

As commented in comment 12, those kind of adjustments will be made in bug 951606 which deals with the visual refresh and it is the one I am 100% focusing on now. Would you rethink the previous ui-review? I'll ask for ui-reviews for this and the rest of the screens in bug 951606 :-)

Comment 17

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

As agreed with Victoria, I have asked for ui-review in the Call Screen visual refresh bug to deal with the concrete final appearance of this functionality (see https://bugzilla.mozilla.org/show_bug.cgi?id=951606#c108) so we can focus in the functionality per se in this bug and not the appearance. Thanks!

So deactivating the requested ui-reviews in this bug.

Comment 18

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

Please, consider the attached screenshots as a reference for the peer reviewer and let's focus on the functionality in this bug. Thanks!

Comment 19

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

Comment on attachment 8421184 [details]
19171.html

Solving the issues with the test cases. I'll ask for a new review as soon as they are solved.

Comment 20

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

Comment on attachment 8421184 [details]
19171.html

Asking for review once the test issues have been solved ;-)

Comment 21

[Victoria Gerchinhoren [:vicky]]

(In reply to Germán Toro del Valle from comment #16)
> As commented in comment 12, those kind of adjustments will be made in bug
> 951606 which deals with the visual refresh and it is the one I am 100%
> focusing on now. Would you rethink the previous ui-review? I'll ask for
> ui-reviews for this and the rest of the screens in bug 951606 :-)

Sure.

Comment 22

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

Hi Anthony, I setting this bug as a duplicate of bug 951606 and moving its patch to that's bug patch so I can start asking Vicky for ui-reviews of the VR where this fluid font size behaviour is needed so we do not block the VR of the Call Screen any longer ;-) Thanks!

Comment 23

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

We decided to keep this bug and separate its patch from the one for the visual refresh of the Call Screen (bug 951606) ;-)

Comment 24

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

Attachment: 19918.html

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 8433148 [details]
19918.html

Pretty big patch so we might want to team up, but mind taking a first crack at it Doug?

Comment 26

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8433148 [details]
19918.html

This is actually not bad overall, but it's sorely lacking tests. In particular, off the top of my head:

* FontSizeManager in general (the mock should also uses this.sinon.spy instead of storing whether or not functions have been called on it)
* KeypadManager should have checks that it's calling into FSM with the correct parameters
* CallsHandler
* CallScreen should have a test for its getScenario() function

I also filed a followup (bug 1021540) because I don't believe that the ellipses calculations that we're doing are necessary. Since this is a refactor I'd rather not overcomplicate it for now.

Comment 27

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

Comment on attachment 8433148 [details]
19918.html

Hi Doug! Thank you very much for your comments. I have updated the pull request covering them. Re-review? Thanks!

Comment 28

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8433148 [details]
19918.html

You only added one extra test, and we need many more. These all still apply:

(In reply to Doug Sherk (:drs) from comment #26)
> Comment on attachment 8433148 [details]
> 19918.html
> 
> * FontSizeManager in general (the mock should also uses this.sinon.spy
> instead of storing whether or not functions have been called on it)
> * KeypadManager should have checks that it's calling into FSM with the
> correct parameters
> * CallsHandler
> * CallScreen should have a test for its getScenario() function

Comment 29

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8433148 [details]
19918.html

German, I think there might have been some miscommunication on my part. So I'll be more clear about what I was thinking in terms of what tests to add.

* FontSizeManager needs to have an associated MockFontSizeManager, and test file font_size_manager_test.js, which will contain the following tests:
  - adaptToSize should be tested in each scenario
  - adaptToSize should be tested with forceMaxFontSize
  - adaptToSize should be tested with both ellipsis sides

* HandledCall needs to have tests for calling into adaptToSize and passed parameters
  - in formatPhoneNumber()

* ConferenceGroupHandler needs to have tests for calling into adaptToSize and passed parameters
  - in end()

* CallsHandler needs to have tests for calling into adaptToSize and passed parameters
  - in handleCallWaiting()
  - and again inside the cb for Contacts.findByNumber()

* KeypadManager needs to have tests for calling into adaptToSize and passed parameters
  - in _updatePhoneNumberView()

* CallScreen needs a check for calling updateCallsDisplay
  - in resizeHandler()

I left some additional comments on the PR.

Comment 30

[Axel Hecht [:Pike]]

I think that shared/js/font_size_utils.js does something similar to what you're trying to do here?

Comment 31

[Doug Sherk (:drs) (inactive)]

(In reply to Axel Hecht [:Pike] from comment #30)
> I think that shared/js/font_size_utils.js does something similar to what
> you're trying to do here?

It seems like it does. But this is mostly a refactor, so I don't think it's reasonable to switch gears and use the shared code now. I think we can do the port to FontSizeUtils in bug 1021540.

Comment 33

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

Hi Doug,

(In reply to Doug Sherk (:drs) from comment #29)
> Comment on attachment 8433148 [details]
> 19918.html
> 
> German, I think there might have been some miscommunication on my part. So
> I'll be more clear about what I was thinking in terms of what tests to add.
> 
> * FontSizeManager needs to have an associated MockFontSizeManager, and test
> file font_size_manager_test.js, which will contain the following tests:
>   - adaptToSize should be tested in each scenario
>   - adaptToSize should be tested with forceMaxFontSize
>   - adaptToSize should be tested with both ellipsis sides
> 

After some tests, I noticed that it is not easy (possible?) to unit test the FontSizeManager object due to the use it does of the |getBoundingClientRect()| function and so on. The code is probably not running in the right "environment" to get this data :( The final font size highly depends on this to set the final font size.

On the other hand, I considered the option of including some Marionette integration test but the problem there is that there is no way to test the Call Screen using Marionette as far as my understanding :-(

Any clue here is more than appreciated? Anyhow, I think we do have a good code coverage now taking into consideration the tests included in 

> * HandledCall needs to have tests for calling into adaptToSize and passed
> parameters
>   - in formatPhoneNumber()
>

Do you refer to this (already included in the current pull request): https://github.com/mozilla-b2g/gaia/pull/19918/files#diff-08f2a55579da54c0d78bfff6b34abaa3R667
 
> * ConferenceGroupHandler needs to have tests for calling into adaptToSize
> and passed parameters
>   - in end()
> 

I included the following test: https://github.com/gtorodelvalle/gaia/blob/dialer-callscreen-bug-1007709-fluid-font-size/apps/callscreen/test/unit/conference_group_handler_test.js#L284

> * CallsHandler needs to have tests for calling into adaptToSize and passed
> parameters
>   - in handleCallWaiting()
>   - and again inside the cb for Contacts.findByNumber()
> 

I included several test to check that FontSizeManager.adaptToSpace() is called with the proper arguments.

> * KeypadManager needs to have tests for calling into adaptToSize and passed
> parameters
>   - in _updatePhoneNumberView()
> 

I included a test in keypad_test.js

> * CallScreen needs a check for calling updateCallsDisplay
>   - in resizeHandler()
> 

Included ;-)

I included some tests to cover 

> I left some additional comments on the PR.

I'll ask for a new review to try to land this patch ASAP :-) Thanks!

Comment 34

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

font_size_manager_test.js included and ready for final review ;)

Comment 35

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8433148 [details]
19918.html

LGTM! \o/

This was a really long patch and review. Thanks for staying with me on it. I tested it and it seems to work.

We also got an over-IRC review+ from Etienne on the removal of apps/system/test/unit/mock_keypad.js.

Comment 36

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

Merged in master: https://github.com/mozilla-b2g/gaia/commit/919d4a2d84f3c4a2ddc2bca62edf766f375d7ede

Comment 37

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

Comment on attachment 8433148 [details]
19918.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature requesting the phone number or contact name to fill all the available space and adapt its font size accordingly.
[User impact] if declined: Bad user experience and appearance.
[Testing completed]: Tests passing and heavily tested on device.
[Risk to taking this patch] (and alternatives if risky): Although it is not a simple patch, it has been heavily tested.
[String changes made]: None

Comment 38

[Carsten Book [:Tomcat]]

can this be reverted, seems this is causing problems like https://tbpl.mozilla.org/php/getParsedLog.php?id=42255283&tree=B2g-Inbound but somehow it seems i can't revert this

Comment 39

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

The tests were passing... :-O Hopefully not a new case of intermittent tests... :(

Contacting José Manuel to try to revert it.

Comment 40

[Carsten Book [:Tomcat]]

reverted this now, should be land soon on b2g-i :)

Comment 41

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

Attachment: 20889.html

Creating a new pull request after reverting the patch for the previous one... Let's see the result of the tests.

Comment 42

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

Hi guys, Travis is back with the results and it seems that there is a test failing but it has nothing to do with the patch which solves this bug :S (it is related to the Marketplace app)

https://travis-ci.org/mozilla-b2g/gaia/builds/28214353

Any ideas about how we should proceed? Thanks!

Comment 43

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #42)
> Hi guys, Travis is back with the results and it seems that there is a test
> failing but it has nothing to do with the patch which solves this bug :S (it
> is related to the Marketplace app)
> 
> https://travis-ci.org/mozilla-b2g/gaia/builds/28214353
> 
> Any ideas about how we should proceed? Thanks!

This is a fairly common intermittent. If you want to be extra sure, you can rerun the test, but I think this is safe.

I looked at your last automatic Gaia-Try push here:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=c4ae24f6475c

It looks like there were some intermittent failures in tests for SIMSlot, but I didn't see any FSM intermittents in the full log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42271732

Just to be safe, I requested 3 additional runs of the unit tests on TBPL, but if they pass, I would just go ahead and merge.

Comment 44

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

Hi, yeah I sent a couple of pull request to my local (updated) master to run 10 times the unit tests and they always pass. There is a problem with some marketplace and homescreen tests will "sometimes" fail though. Please, have a look at:
  1. https://github.com/gtorodelvalle/gaia/pull/33 running 10 times the tests-in-firefox. In this case, the tests always pass. Results at https://travis-ci.org/gtorodelvalle/gaia/builds/28227648 . Sometimes a marketplace and homescreen tests fail.
  2. https://github.com/gtorodelvalle/gaia/pull/34 running 10 times the gaia_ui_tests (unit, functional and accessibility tests). Results at https://travis-ci.org/gtorodelvalle/gaia/builds/28227880

So I guess it is pretty safe to land this patch. Anyhow, the marketplace issue with test_marketplace_launch.py should be solved.

Comment 45

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

Hi Fred, any clue about the test_marketplace_launch.py issue? Please, see https://travis-ci.org/gtorodelvalle/gaia/jobs/28227882 , please. Thanks!

Comment 46

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #45)
> Hi Fred, any clue about the test_marketplace_launch.py issue? Please, see
> https://travis-ci.org/gtorodelvalle/gaia/jobs/28227882 , please. Thanks!

If you look at the current builds, it looks like this is a fairly common intermittent that other commits are getting too. I think you're safe to land this.

Comment 47

[bhavana bajaj [:bajaj]]

Comment on attachment 8433148 [details]
19918.html

Approving, given the intermittent failures are expected. Adding verifyme for QA to test this..

Comment 48

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

With support by Francisco I launched several TBPL runs yesterday and they passed. We also included same traces to try to narrow the issue down. I just updated de pull request and if we get the all green by Travis I will merge it with Francisco's OK. Thank you everyone! ;)

Comment 49

[Fred Wenzel [:wenzel]]

I'm guessing you meant to ping a different Fred?

Comment 50

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

Ups, sorry Fred, I had a look at https://wiki.mozilla.org/Modules/All and took your name instead of Marketplace app owner Wil Clouser ;9 Sorry about that!

Comment 51

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

Merged again in master: https://github.com/mozilla-b2g/gaia/commit/0043a44bd2fc23d1e71f4c83113da27892351c01

Checking if everything is also fine in 2.0 to ask for the approval ;)

Comment 52

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

Just confirmed that it works perfectly in 2.0 and the commit can be automatically cherry-picked, so asking for approval ;)

Comment 53

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

Comment on attachment 8444370 [details]
20889.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature requesting the phone number or contact name to fill all the available space and adapt its font size accordingly.
[User impact] if declined: Bad user experience and appearance.
[Testing completed]: Tests passing and heavily tested on device.
[Risk to taking this patch] (and alternatives if risky): Although it is not a simple patch, it has been heavily tested.
[String changes made]: None

Comment 54

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/e8d3c4b5f40d721dd47d725625acd316c1860183

Comment 56

[Loli (:lolimartinezcr)]

Pending  https://bugzilla.mozilla.org/show_bug.cgi?id=1031257

Comment 58

[Eric Chang [:ericcc] [:echang]]

Verified okay with these 2 builds.
Flame - aurora - v2.0 
Gaia      e935f4ff190b76c70d9b2af8856c542a6e4a7546
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
BuildID   20140707160206
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

Flame - master - v2.1 
Gaia      1dc9e53393ae4680a174dffa44a958ec564ebbe8
Gecko     https://hg.mozilla.org/mozilla-central/rev/dfef245594b6
BuildID   20140707160202
Version   33.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

Comment 59

[Paladin]

Attachment: verify_image.png

This issue has been verified successfully on Flame 2.0,2.1

See attachment: Verify_image.png
Reproducing rate: 0/5
Flame2.0  build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Flame2.1 build:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0