Closed Bug 1010104 Opened 9 years ago Closed 9 years ago

[Dialer][Call Screen] Baseline of the contact name when applying the fluid font size

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 wontfix, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S1 (1aug)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.1 --- fixed

People

(Reporter: gtorodelvalle, Assigned: gtorodelvalle)

References

Details

(Whiteboard: [planned-sprint c=1][in-sprint=v2.0-S6])

Attachments

(6 files)

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.
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.
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.
Sorry, I meant bug 1007709 instead of bug 1003846 which is the one dealing with the adaptable contact name font size.
Depends on: 1007709, 951606
Needinfo to Vicky to remind her to update this bug when she has reached a decision.
Flags: needinfo?(vpg)
(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.
Flags: needinfo?(vpg)
Need info to Germán, to understand what's the issue here. Germán, do you need further imput from my side here?
Flags: needinfo?(gtorodelvalle)
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 ;-)
Flags: needinfo?(gtorodelvalle)
(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.
Sprint 3 ends Friday. Who is working on this for implementation?
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.
nominate 2.0? to be triaged and well prioritized.
blocking-b2g: --- → 2.0?
Flagging Wilfred for a blocking call or not.
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(wmathanaraj)
triage: not a blocker
blocking-b2g: 2.0? → backlog
Assignee: nobody → gtorodelvalle
Attached file 21347.html
Attachment #8452170 - Flags: review?(anthony)
Attachment #8452170 - Flags: review?(anthony) → review?(drs+bugzilla)
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.
Attachment #8452170 - Flags: review?(drs+bugzilla) → review-
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!
Flags: needinfo?(drs+bugzilla)
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.
Flags: needinfo?(drs+bugzilla)
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S6 (18july)
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.
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? :)
Flags: needinfo?(drs+bugzilla)
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.
Attachment #8452170 - Flags: feedback+
Flags: needinfo?(drs+bugzilla)
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!
Flags: needinfo?(drs+bugzilla)
(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.
(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.
Flags: needinfo?(drs+bugzilla)
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.
Attached file 21892.html
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 ;)
Attachment #8458249 - Flags: review?(drs+bugzilla)
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 :(
It is not your patch. The reflow test tool is broken at the moment, we think it is because of bug 1000428.
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.
Attachment #8458249 - Flags: review?(drs+bugzilla) → review-
Whiteboard: [planned-sprint][in-sprint=v2.0-S6]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
:D No problem, of course ;) Working on your comments. Thanks!
Whiteboard: [planned-sprint][in-sprint=v2.0-S6] → [planned-sprint c=1][in-sprint=v2.0-S6]
(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! ;)
Attachment #8458249 - Flags: review- → review?(drs+bugzilla)
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.
Attachment #8458249 - Flags: review?(drs+bugzilla) → review-
Comment on attachment 8458249 [details]
21892.html

We can IRC-review it this afternoon ;)
Attachment #8458249 - Flags: review- → review?(drs+bugzilla)
Comment on attachment 8458249 [details]
21892.html

Almost there.
Attachment #8458249 - Flags: review?(drs+bugzilla) → review-
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!
Attachment #8458249 - Flags: review- → review+
Waiting for the tests to be checked and I'll merge the patch after including the latest comments by Doug ;) Thanks!
Landed in master: https://github.com/mozilla-b2g/gaia/commit/4f79d00a118b1899a31f31279841420bda192b7e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
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!
Flags: needinfo?(anthony)
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.
Flags: needinfo?(anthony)
Depends on: 1079440
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.