Germán Toro del Valle (:gtorodelvalle) Assignee
	Description • 10 years ago
kanban-ready

Once bug 1003846 and bug 951606, the font size applied to the name of the contact in the Call Screen will adapt to the available space. This provokes the baseline of that text to move upwards depending on the final font size (please see attached screenshots). We should decide the right appearance to implement regarding this baseline.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 1 • 10 years ago

This is an example of the maximum font size applicable to the contact name when it is short enough to be fitted in the available space.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 2 • 10 years ago

This is an example of the minimum font size applicable to the contact name when it is big enough and it does not fit in the available space.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 3 • 10 years ago

Sorry, I meant bug 1007709 instead of bug 1003846 which is the one dealing with the adaptable contact name font size.

	Anthony Ricaud (:rik)
	Comment 4 • 10 years ago

Needinfo to Vicky to remind her to update this bug when she has reached a decision.

	Victoria Gerchinhoren [:vicky]
	Comment 5 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #4)
> Needinfo to Vicky to remind her to update this bug when she has reached a
> decision.

Hi Anthony. What decision are you expecting me to make? The baseline should remain the same when the size changes, this bug is to work on that.

Thanks.

	Victoria Gerchinhoren [:vicky]
	Comment 6 • 10 years ago

Need info to Germán, to understand what's the issue here. Germán, do you need further imput from my side here?

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 7 • 10 years ago

Hi Vicky, although I will contact you via chat :-) my doubt here is regarding the final appearance. If we keep the baseline no matter the font size, the distance from the upper part of the letters to the top of the header area will increase as the font size diminishes... Is this the desired behaviour? Maybe we should think of a visual spec based on rems regarding the desired final behaviour. As mentioned, I'll contact you via chat to discuss about it and we'll include the decision here ;-)

	Victoria Gerchinhoren [:vicky]
	Comment 8 • 10 years ago

(In reply to Germán Toro del Valle from comment #7)
> Hi Vicky, although I will contact you via chat :-) my doubt here is
> regarding the final appearance. If we keep the baseline no matter the font
> size, the distance from the upper part of the letters to the top of the
> header area will increase as the font size diminishes... Is this the desired
> behaviour? Maybe we should think of a visual spec based on rems regarding
> the desired final behaviour. As mentioned, I'll contact you via chat to
> discuss about it and we'll include the decision here ;-)

Please, keep the baseline as I told you in the first place. I know what will happen with that distance, of course, I tried it.

	Stephany Wilkes
	Comment 9 • 10 years ago

Sprint 3 ends Friday. Who is working on this for implementation?

	Anthony Ricaud (:rik)
	Comment 10 • 10 years ago

We didn't take this in our sprint but it's ok because it's totally the kind of bug that can be fixed during the Feature Complete period.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 11 • 10 years ago

nominate 2.0? to be triaged and well prioritized.

	Stephany Wilkes
	Comment 12 • 10 years ago

Flagging Wilfred for a blocking call or not.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 13 • 10 years ago

triage: not a blocker

	Doug Sherk (:drs) (inactive)
	Comment 15 • 10 years ago

Comment on attachment 8452170 [details]
21347.html

I don't think that this amount of extra code is necessary. When we set font-size, we can also override the height and line-height props and set them to the same number. If we're worried about the header having a minimum size, we can set the min-height prop of the header. I've tested this in the inspector and it works quite well.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 16 • 10 years ago

Hi Doug, I do not know if I follow :S Have you tested your solution throughout the life cycle of a call, I mean: connecting/dialing, ongoing, ended, etc., and covering all the possible scenarios? The adaptToSpace() function is called many times during a call :(

In fact, the code which solves the issue is https://github.com/gtorodelvalle/gaia/blob/callscreen-bug-1010104-baseline/shared/js/dialer/font_size_manager.js#L198 There is where I set the line-height when the new font size is calculated. The rest of the code is to assure that adaptToSpace() is properly called regarding the concrete scenario. To this regard, there is a new scenario (and a typo I just found :O) to distinguish between the ongoing call waiting scenario and the second incoming call one which although using the same font-size do use distinct header sizes.

May I have a take a glimpse about the code you are proposing, please? Thanks!

	Doug Sherk (:drs) (inactive)
	Comment 17 • 10 years ago

Thanks for trying this Germán. You're right that this doesn't work exactly, but I looked into it some more and with a few more properties it does work quite well. Here's exactly what I did:

.number (given a font-size):
+ line-height: font-size (you should set this in JS once you try this in the inspector)
+ height: font-size (you should set this in JS once you try this in the inspector)
+ display: table-cell
+ vertical-align: bottom (or baseline, you can mess around with this)

.numberWrapper:
+ display: table
+ min-height: 58px (mess around with this number)
+ width: 100%

Let me know if you have any more questions or trouble with this.

	Doug Sherk (:drs) (inactive)
	Comment 18 • 10 years ago

Germán told me on IRC that this didn't quite work and the baseline was slightly off. I tested this and it definitely works:

.number (given a font-size):
+ line-height: font-size (you should set this in JS once you try this in the inspector)
+ height: font-size (you should set this in JS once you try this in the inspector)
+ display: table-cell
+ vertical-align: bottom (or baseline, you can mess around with this)

.numberWrapper:
+ display: table
+ height: 58px (mess around with this number)
+ width: 100%

.duration:
+ position: absolute

The baseline might be 1px off with this, but I can't really tell. I did a side-by-side comparison 10 times and I'm still not sure. It it's even an issue and I'm not just imagining it, it might just be an issue with the font.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 19 • 10 years ago

Hi Doug! I took your new suggestion but although pretty close to the desired behaviour, I think it is not good enough because if I am able to see that is not good news :)

In this screenshot I include the appearance of the call screen using the maximum font and using the minimum font using my approach (in the middle and ui-review+ by Vicky :p) and yours at the end. I know Vicky enough to say that we won't be OK with it :D

Regarding my approach, let me share with you that it is based on input such as http://www.smashingmagazine.com/2012/12/17/css-baseline-the-good-the-bad-and-the-ugly/ Taking into consideration the spaghetti code we currently have for  call screen page I really doubt we are going to find a simpler solution :( But, do you want to give it a new try? :)

	Doug Sherk (:drs) (inactive)
	Comment 20 • 10 years ago

Comment on attachment 8452170 [details]
21347.html

Ok, I'm convinced. I've filed bug 1039594 as a followup because I'm not happy about having to do this, and it seems like the kind of thing that could be centralized better. I left some comments on the PR.

	Stephany Wilkes
	Comment 21 • 10 years ago

Please let me know if we expect this to land in time for 2.0. Just going through the last open bugs for the visual refresh so we can close it out. Thanks!

	Maria Angeles Oteo (:oteo)
	Comment 22 • 10 years ago

(In reply to Stephany Wilkes from comment #21)
> Please let me know if we expect this to land in time for 2.0. Just going
> through the last open bugs for the visual refresh so we can close it out.
> Thanks!

Hi Stephany, we are trying to solve it and ask for approval to 2.0 but the revision is taking longer. In the Comms triage we all agreed we want it for 2.0 (very nice to have). Let's see if German can land it today and ask for the approval, otherwise we will have to leave it for 2.1, what's really a pity.

	Doug Sherk (:drs) (inactive)
	Comment 23 • 10 years ago

(In reply to Stephany Wilkes from comment #21)
> Please let me know if we expect this to land in time for 2.0. Just going
> through the last open bugs for the visual refresh so we can close it out.
> Thanks!

(In reply to Maria Angeles Oteo (:oteo) from comment #22)
> (In reply to Stephany Wilkes from comment #21)
> > Please let me know if we expect this to land in time for 2.0. Just going
> > through the last open bugs for the visual refresh so we can close it out.
> > Thanks!
> 
> Hi Stephany, we are trying to solve it and ask for approval to 2.0 but the
> revision is taking longer. In the Comms triage we all agreed we want it for
> 2.0 (very nice to have). Let's see if German can land it today and ask for
> the approval, otherwise we will have to leave it for 2.1, what's really a
> pity.

We discussed this internally within the dialer team and we don't think that this is a) noticeable enough, b) trivial enough, or c) timely enough, to get uplifted to 2.0, regardless of whether or not we finish it before the FC date. So for now, I'm going to say that we're not going to land this on 2.0.

	Doug Sherk (:drs) (inactive)
	Comment 24 • 10 years ago

The approach for calling getScenario() needs some rethinking. In particular, I think it's awkward to carry around the node.textContent variable everywhere. Also, it's unreliable. If you're already in a call with a 'Withheld number', and another one comes in, this logic will think that they are one call. The attached patch suggests an alternate way to do this which accounts for both problems.

This also needs tests, which it has none of right now.

Germán asked for review again on IRC so I'll just verbally say review- again since the flag is already set.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 25 • 10 years ago

Hi Doug, I prepared a new pull request implementing what I understood as your approach :) , this is, no getScenario() function and getting the scenario each time FontSizeManager.adaptToSpace() is called. I won't set the previous pull request as obsolete until we agree that the one implemented in this one is the one you are more in favor of ;)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 26 • 10 years ago

BTW, Doug, there is a test failing regarding the reflow count I am not getting locally :O I do not know if it is related to my patch or it would also reproduce in master since as I said I am not getting it using my patch or in master when run locally :(

	Anthony Ricaud (:rik)
	Comment 27 • 10 years ago

It is not your patch. The reflow test tool is broken at the moment, we think it is because of bug 1000428.

	Doug Sherk (:drs) (inactive)
	Comment 28 • 10 years ago

Comment on attachment 8458249 [details]
21892.html

Thanks. To me, the CALL_WAITING/SECOND_INCOMING_CALL ternary looks a lot cleaner than what we had before. When I glance at it, I know exactly what's going on and what the possible code paths are. When I was talking about pulling this logic out of getScenario(), it was because there was so much logic in getScenario() which I didn't think was necessary for cases where we can only have CALL_WAITING/SECOND_INCOMING_CALL. Using getScenario() here would have confused onlookers who are unfamiliar with the code into thinking that this could be any of the 5 scenarios. As I said before, I also find it a bit clunky to carry around parameters for getScenario().

On the other hand, getting rid of getScenario() entirely just throws us in the opposite direction of not having enough abstraction. There's obvious value in abstracting this logic as it's used so heavily elsewhere. I understand your objections now, and I would understand if you were frustrated, as I don't think I was clear enough about my thoughts here and we were thinking different things. I'll be more specific in the future so that we avoid misunderstandings like this.

I left some comments on the PR. We're also going to need some new unit tests for this functionality. Here are the ones I've identified:

calls_handler.js / calls_handler_test.js
* A second incoming call with a "Withheld number" should have the correct parameters (incl. scenario=SECOND_INCOMING_CALL) passed into adaptToSpace(). This should test both of: 1) the ongoing (first) call is a real number/contact name, 2) the ongoing call is also "Withheld number".
* A single incoming call, with no other ongoing calls, should have the correct parameters (incl. scenario=CALL_WAITING) passed into adaptToSpace().

handled_call.js / handled_call_test.js
* A second incoming call has the correct parameters (incl. scenario=SECOND_INCOMING_CALL) passed into adaptToSpace().

If you'd like, after the next iteration is done, we can do another IRC review. I find that we work better that way as it's easier to clarify and give examples.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 29 • 10 years ago

:D No problem, of course ;) Working on your comments. Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 30 • 10 years ago

(In reply to Doug Sherk (:drs) from comment #28)
> Comment on attachment 8458249 [details]
> 21892.html
> 
> Thanks. To me, the CALL_WAITING/SECOND_INCOMING_CALL ternary looks a lot
> cleaner than what we had before. When I glance at it, I know exactly what's
> going on and what the possible code paths are. When I was talking about
> pulling this logic out of getScenario(), it was because there was so much
> logic in getScenario() which I didn't think was necessary for cases where we
> can only have CALL_WAITING/SECOND_INCOMING_CALL. Using getScenario() here
> would have confused onlookers who are unfamiliar with the code into thinking
> that this could be any of the 5 scenarios. As I said before, I also find it
> a bit clunky to carry around parameters for getScenario().
> 
> On the other hand, getting rid of getScenario() entirely just throws us in
> the opposite direction of not having enough abstraction. There's obvious
> value in abstracting this logic as it's used so heavily elsewhere. I
> understand your objections now, and I would understand if you were
> frustrated, as I don't think I was clear enough about my thoughts here and
> we were thinking different things. I'll be more specific in the future so
> that we avoid misunderstandings like this.
> 
> I left some comments on the PR. We're also going to need some new unit tests
> for this functionality. Here are the ones I've identified:
> 
> calls_handler.js / calls_handler_test.js
> * A second incoming call with a "Withheld number" should have the correct
> parameters (incl. scenario=SECOND_INCOMING_CALL) passed into adaptToSpace().
> This should test both of: 1) the ongoing (first) call is a real
> number/contact name, 2) the ongoing call is also "Withheld number".
> * A single incoming call, with no other ongoing calls, should have the
> correct parameters (incl. scenario=CALL_WAITING) passed into adaptToSpace().
> 

Included. BTW, the single incoming call scenario is tested in handled_call_test.js.

> handled_call.js / handled_call_test.js
> * A second incoming call has the correct parameters (incl.
> scenario=SECOND_INCOMING_CALL) passed into adaptToSpace().
> 

The second incoming call scenario is tested in handled_calls_test.js. handled_call_test.js only covers single handled calls scenarios.

> If you'd like, after the next iteration is done, we can do another IRC
> review. I find that we work better that way as it's easier to clarify and
> give examples.

Sure! ;)

	Doug Sherk (:drs) (inactive)
	Comment 31 • 10 years ago

Comment on attachment 8458249 [details]
21892.html

We're getting a lot closer. Unfortunately, there are some indentation problems with the code you added back, but structurally this is now fairly sound. I added some comments to the PR.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 32 • 10 years ago

Comment on attachment 8458249 [details]
21892.html

We can IRC-review it this afternoon ;)

	Doug Sherk (:drs) (inactive)
	Comment 33 • 10 years ago

Comment on attachment 8458249 [details]
21892.html

Almost there.

	Doug Sherk (:drs) (inactive)
	Comment 34 • 10 years ago

Comment on attachment 8458249 [details]
21892.html

(new version of the PR)

One last indentation mistake I mentioned on the PR and then I think we're good to go. Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 35 • 10 years ago

Waiting for the tests to be checked and I'll merge the patch after including the latest comments by Doug ;) Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 36 • 10 years ago

Landed in master: https://github.com/mozilla-b2g/gaia/commit/4f79d00a118b1899a31f31279841420bda192b7e

	Anthony Ricaud (:rik)
	Comment 37 • 10 years ago

This patch is lacking new tests for the FontSizeManager bahviours. And because I'm refactoring for bug 967440, it makes it more painful than necessary.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 38 • 10 years ago

Hi Anthony. Which tests are you missing to the already added in the patch (attachment 8458249 [details] ) or the already included in https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/dialer/font_size_manager_test.js ? Just to file a bug to include them if appropriate ;) Thanks!

	Anthony Ricaud (:rik)
	Comment 39 • 10 years ago

The line-height changes to keep the same baseline. But I'll write those as part of my refactoring in bug 967440. It would be too much work to write them for the current code and adapt them for my refactoring. I'll ask you feedback on it once I have a patch.