Closed Bug 1007709 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: vicky, Assigned: gtorodelvalle)

References

Details

(Whiteboard: [planned-sprint])

Attachments

(7 files, 4 obsolete files)

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

Spec is attached.
Assignee: nobody → gtorodelvalle
Blocks: 951606
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Summary: Call. Create fluid size for contact name in the upper information panel. → [Dialer][Call Screen] Create fluid size for contact name in the upper information panel
Vicky, the upcoming screenshots may burn your eyes... Don't say I did not warn you... :-p
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.
Need-infoing Vicky regarding comment 4 ;)
Flags: needinfo?(vpg)
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
Blocks: 1007747
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
Attachment #8419500 - Flags: ui-review?(vpg) → ui-review+
Flags: needinfo?(vpg)
Comment on attachment 8419501 [details]
callscreen_contact_incoming_min_font_size.png

Font size minumum: OK. Rest of screen not ok.
Attachment #8419501 - Flags: ui-review?(vpg) → ui-review+
Hi Vicky, it seems your ui-review+ removed the need-info I had set :-O

Resetting need-info regarding comments 4 and 6 ;-) Thanks!
Flags: needinfo?(vpg)
(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!
Flags: needinfo?(vpg)
Target Milestone: --- → 2.0 S2 (23may)
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.
Attachment #8419501 - Attachment is obsolete: true
Attachment #8421157 - Flags: ui-review?(vpg)
Attachment #8419501 - Attachment is obsolete: false
Attachment #8419505 - Attachment is obsolete: true
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.
Attachment #8419507 - Attachment is obsolete: true
Attachment #8421158 - Flags: ui-review?(vpg)
Attached file 19171.html (obsolete) —
Attachment #8421184 - Flags: review?(anthony)
Comment on attachment 8421158 [details]
callscreen_callwaiting_contact_connected_2_min_font_size.png

Please, align the name and timestamps baselines.
Attachment #8421158 - Flags: ui-review?(vpg) → ui-review-
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.
Attachment #8421157 - Flags: ui-review?(vpg) → ui-review-
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 :-)
Flags: needinfo?(vpg)
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.
Attachment #8419500 - Flags: ui-review+
Attachment #8419501 - Flags: ui-review+
Attachment #8421157 - Flags: ui-review-
Attachment #8421158 - Flags: ui-review-
Please, consider the attached screenshots as a reference for the peer reviewer and let's focus on the functionality in this bug. Thanks!
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.
Attachment #8421184 - Flags: review?(anthony)
Comment on attachment 8421184 [details]
19171.html

Asking for review once the test issues have been solved ;-)
Attachment #8421184 - Flags: review?(anthony)
Blocks: 1010104
(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.
Flags: needinfo?(vpg)
Attachment #8421184 - Flags: review?(anthony)
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!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
We decided to keep this bug and separate its patch from the one for the visual refresh of the Call Screen (bug 951606) ;-)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Attached file 19918.html (obsolete) —
Attachment #8421184 - Attachment is obsolete: true
Attachment #8433148 - Flags: review?(anthony)
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?
Attachment #8433148 - Flags: review?(anthony) → review?(drs+bugzilla)
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.
Attachment #8433148 - Flags: review?(drs+bugzilla) → review-
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!
Attachment #8433148 - Flags: review- → review?(drs+bugzilla)
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
Attachment #8433148 - Flags: review?(drs+bugzilla) → review-
Attachment #8433148 - Flags: review- → review?(drs+bugzilla)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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.
Attachment #8433148 - Flags: review?(drs+bugzilla) → review-
I think that shared/js/font_size_utils.js does something similar to what you're trying to do here?
(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.
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!
Attachment #8433148 - Flags: review- → review?(drs+bugzilla)
font_size_manager_test.js included and ready for final review ;)
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.
Attachment #8433148 - Flags: review?(drs+bugzilla) → review+
Blocks: 1018283
Merged in master: https://github.com/mozilla-b2g/gaia/commit/919d4a2d84f3c4a2ddc2bca62edf766f375d7ede
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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
Attachment #8433148 - Flags: approval-gaia-v2.0?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The tests were passing... :-O Hopefully not a new case of intermittent tests... :(

Contacting José Manuel to try to revert it.
reverted this now, should be land soon on b2g-i :)
Attached file 20889.html
Creating a new pull request after reverting the patch for the previous one... Let's see the result of the tests.
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!
(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.
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.
Hi Fred, any clue about the test_marketplace_launch.py issue? Please, see https://travis-ci.org/gtorodelvalle/gaia/jobs/28227882 , please. Thanks!
Flags: needinfo?(fwenzel)
(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 on attachment 8433148 [details]
19918.html

Approving, given the intermittent failures are expected. Adding verifyme for QA to test this..
Attachment #8433148 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Keywords: verifyme
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! ;)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
I'm guessing you meant to ping a different Fred?
Flags: needinfo?(fwenzel)
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!
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 ;)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Just confirmed that it works perfectly in 2.0 and the commit can be automatically cherry-picked, so asking for approval ;)
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
Attachment #8444370 - Flags: approval-gaia-v2.0?
Attachment #8433148 - Attachment is obsolete: true
Attachment #8444370 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Whiteboard: [planned-sprint]
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
Status: RESOLVED → VERIFIED
Attached image 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
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: