865079 - [Call log] Remove synchronous reflows

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

We perform a lot of synchronous reflow during call log startup. This is mainly due to width calculations which was originally implemented for bug 818465.

We should be able to get rid of the javascript calculations in favor of CSS tweaks to save some time here.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

This is a WIP. There is a slight UX impact for this bug at it's current state. 

The previous behavior was to truncate the name:

Kevin G... or 1 other (2)


With my patch, the entire line is truncated currently. Something like:

Kevin GrandonOS or 1 oth


I think it should be possible to mimic the previous truncation with pure CSS if desired, and I am looking into this.

Comment 2

[Kevin Grandon :kgrandon]

FYI - It looks like we were spending around 35% of our time inside of this function. Quite a lot!

Comment 3

[Kevin Grandon :kgrandon]

Oops - clearing flags due to bug clone for now. Will renom once a solution is in place.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 741141 [details]
Github pull request pointer

Borja - these javascript calculations are killing us on large lists (~500 entries). When you get a chance could you review this PR?

Outright removing them does have a slight impact on the UX, so we should also probably follow up with UX team.

Comment 5

[Borja Salguero [:borjasalguero]]

Hi Kevin! I would like to remove this method for sure! Im gonna change this review to Etienne and German, because they know this code better than me. Alberto now it's currently working on the performance stuff, so with this change we are going to have even better results. Thanks!

Comment 6

[Borja Salguero [:borjasalguero]]

Comment on attachment 741141 [details]
Github pull request pointer

Moving the review to CallLog peers!

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 741141 [details]
Github pull request pointer

We should wait for German to have a look in case I missed an edge case.
But I'd love to see this land :)

Comment 8

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

Hi guys! Sorry for the delay in jumping in :-(

Sadly the proposed updates are not correct. If you have a look at the current markup in master, you'll see that the .primary-info element is composed of 3 <span>s which contain:
  1. The phone number or contact name, plus
  2. The "and N others" text in case the same phone number has been assigned to distinct contacts (yes, I know ;-999), plus
  3. The number of calls of the same type made from or to that phone number or contact.
The most complex example of the information shown there could be: "Germán Toro del Valle and 3 others (5)" where we have to use the ellipsis after the phone number or typically contact name, this is "Germán Tor... and 3 others (5)".

That is where all the complexity of the the fitPrimaryInfoToSpace function lies :-(

Anyhow, I would be completely in favor of shredding some thinking on it to try to get it via CSS, although I have to say that Ismael already did it and it was not straight forward at all.

I will include some screenshots with the current behavior in master and using this proposal to make myself clearer ;-)

Comment 9

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

Attachment: MASTER screenshot 1

Comment 10

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

Attachment: MASTER screenshot 2

Comment 11

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

Attachment: MASTER screenshot 3

Comment 12

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

Attachment: PR screenshot 1

Comment 13

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

Attachment: PR screenshot 2

Comment 14

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

Attachment: PR screenshot 3

Comment 15

[Kevin Grandon :kgrandon]

German - This was a known issue when we implemented the patch. For one, I think this UI is too cluttered, and we're trying to do too much. I strongly believe we should make some UX sacrifices here to achieve performance.

Can you point me to the right UX person here so we can run it by them? I do believe we can do something like this with CSS, but I would rather simplify the UX first. I don't see why the text "or 2 others" should be more important than the primary contact name.

Comment 16

[Etienne Segonzac (:etienne)]

Ayman did this spec.

Comment 17

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

Yeap, Ayman is your man ;-) Thanks Etienne! Anyhow, he is currently in Portland focused on the SMS/MMS stuff so he may not respond in short time :-( I'll ping him via Skype :-)

Comment 18

[ayman maat :maat]

Hi guys

Well that looks a mess. why has it taken so long to flag? 

To make this work we need to take this back into visual design to bring this content onto three lines so it would read

	Germán Toro
	mobile, 6856420
	11:04 AM

…This is because, to the best of my knowledge (i.e. last time i worked on call log which was some time ago), we have a hard requirement to output the carrier field in the call log. So, if the carrier field has content we output its content where the phone number is. if the carrier field is empty we output the phone number. Therefore dropping the phone number from the interface is not an option i can offer, unless product has a change of requirement.

RFI to Daniel to confirm if the requirement is still true around this.
RFI to Steve to get visual design going

Comment 19

[Daniel Coloma:dcoloma]

(In reply to ayman maat :maat from comment #18)
> Hi guys
> 
> Well that looks a mess. why has it taken so long to flag? 
> 
> To make this work we need to take this back into visual design to bring this
> content onto three lines so it would read
> 
> 	Germán Toro
> 	mobile, 6856420
> 	11:04 AM
> 
> …This is because, to the best of my knowledge (i.e. last time i worked on
> call log which was some time ago), we have a hard requirement to output the
> carrier field in the call log. So, if the carrier field has content we
> output its content where the phone number is. if the carrier field is empty
> we output the phone number. Therefore dropping the phone number from the
> interface is not an option i can offer, unless product has a change of
> requirement.
> 
> RFI to Daniel to confirm if the requirement is still true around this.
> RFI to Steve to get visual design going

But I think the suggestion here is not dropping the phone number, but changing the way ellipsis is done in case a phone number matches multiple contacts. Have you reviewed that?

Comment 20

[ayman maat :maat]

(In reply to Daniel Coloma:dcoloma from comment #19)
> (In reply to ayman maat :maat from comment #18)
> > Hi guys
> > 
> > Well that looks a mess. why has it taken so long to flag? 
> > 
> > To make this work we need to take this back into visual design to bring this
> > content onto three lines so it would read
> > 
> > 	Germán Toro
> > 	mobile, 6856420
> > 	11:04 AM
> > 
> > …This is because, to the best of my knowledge (i.e. last time i worked on
> > call log which was some time ago), we have a hard requirement to output the
> > carrier field in the call log. So, if the carrier field has content we
> > output its content where the phone number is. if the carrier field is empty
> > we output the phone number. Therefore dropping the phone number from the
> > interface is not an option i can offer, unless product has a change of
> > requirement.
> > 
> > RFI to Daniel to confirm if the requirement is still true around this.
> > RFI to Steve to get visual design going
> 
> But I think the suggestion here is not dropping the phone number, but
> changing the way ellipsis is done in case a phone number matches multiple
> contacts. Have you reviewed that?

To be honest with you i would be inclined to clean it up by just populating the name field with the 1st instance of the contact that occurs in the contact list as it is read alphabetically A to Z that has that phone number.

I would like the phone number presentation issue i mentioned in #comment 18 addressing though.. because an overwhelming majority of enteries in the call log will have this truncated presentation ... and it looks really bad.

Comment 21

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

Even with the simplification Ayman suggests, this is (as far as I understand it), no longer showing the "and N others" section, just like in:

Very long... and N others (KK)

we would still have kind of a similar problem with the number of calls section (the one in brackets), this is:

Very long contact nam... (KK)

taking into consideration that the size of this bracket section is also variable in size.

So, in case we want to simplify (or even remove) all the size calculus done now in the Call Log (and lean only on CSS), I would suggest leaving the contact name on the right, I mean:

(kk) Very long contact nam...

I am sure our UX colleagues would be able to come up with an appealing solution leaving the contact on the right or on its very own row :-) 

In fact, I just noticed that Android does something similar :D

A very long contact nam...
666666666 Work
(5) 3 minutes ago

Apart from this, Android seems also not to care in case more than 1 contact have the same phone number. It just displays one of them :)

Comment 22

[ayman maat :maat]

(In reply to gtorodelvalle from comment #21)
> Even with the simplification Ayman suggests, this is (as far as I understand
> it), no longer showing the "and N others" section, just like in:
> 
> Very long... and N others (KK)
> 
> we would still have kind of a similar problem with the number of calls
> section (the one in brackets), this is:
> 
> Very long contact nam... (KK)
> 
> taking into consideration that the size of this bracket section is also
> variable in size.
> 
> So, in case we want to simplify (or even remove) all the size calculus done
> now in the Call Log (and lean only on CSS), I would suggest leaving the
> contact name on the right, I mean:
> 
> (kk) Very long contact nam...
> 
> I am sure our UX colleagues would be able to come up with an appealing
> solution leaving the contact on the right or on its very own row :-) 
> 
> In fact, I just noticed that Android does something similar :D
> 
> A very long contact nam...
> 666666666 Work
> (5) 3 minutes ago
> 
> Apart from this, Android seems also not to care in case more than 1 contact
> have the same phone number. It just displays one of them :)

This goes back to what i was saying in #comment 18. Really this is a visual design issue, and i feel the path to solution is to go for a 3 row layout as we are trying to cram way too much information into two rows... and the result is very poor. 

Just to evolve Germán's suggestion the following layout would work from a UX/IA perspective.

	Germán Toro
	mobile, 6856420
	(12) 11:04 AM

logic behind this is that on the second line the order should be type, phone number as his is the order of recognition for the end user... being that they are more likely to think of and therefore recognize 'Germán's mobile' than 'Germán's phone number'... and remember the phone number is only displayed for two reasons:

1) if there is nothing in the carrier field.. 
2) for disambiguation in the list in instances there is nothing in the carrier field and where Germán has more then one mobile number associated to his name.

...so over to visual design.

Comment 23

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

Perfect! So it seems that in the end is Victoria's magic the one we need :-p Thanks, Ayman!

Comment 24

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

Including Alberto as CC in this email and including the dependency with bug 847406 which is currently being solved by him ;-)

Comment 25

[alberto.pastor]

We were already fixing this in Bug 847406, but as this one is near to land, I will reset that method in the other patch, and rebase with your solution as soon as it lands.

Thanks!

Comment 26

[alberto.pastor]

In fact, is already blocking the meta, so removing the specific blocking.

Comment 27

[steve]

Vicky is already working on the three line option. Our visual design system allows for it to still look good.

Comment 28

[Stephany Wilkes]

Clearing general FFOSUX needinfo since it looks like Ayman and Vicki have this well in hand.

Comment 29

[Victoria Gerchinhoren [:vicky]]

Attachment: New layout for call log with 3 lines rows

Comment 30

[Victoria Gerchinhoren [:vicky]]

I am attaching the new Call Log layout. It now has 3 lines and there's a 5px gap between the photo and the horizontal divider. Which is a bit tide but ideally we should show as much entries as we can in a screen view before scrolling. This is still working, although I don't think having the list that crowded and few entries is the best solution, I worked out the best visual solution available for the requirements made.

Comment 31

[alberto.pastor]

Kevin, are you going to work on this or can I steal it? :)

Comment 32

[Kevin Grandon :kgrandon]

Hi Alberto,

This slipped my mind. I'll assign it to you if you'd like to work on it :) 

If not, feel free to un-assign, or assign back to me. Thanks!

Comment 33

[Victoria Gerchinhoren [:vicky]]

Just to clarify: The red icon and time stamp are for the missed calls that were not checked from the user yet. When the user already checked the call log, so that information is known, the missed calls will have a black icon and time stamp.

Thanks!

Comment 34

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9990

Comment 35

[Victoria Gerchinhoren [:vicky]]

Alberto, can you please post a screenshot?
THanks!

Comment 36

[alberto.pastor]

Attachment: Screenshot new design

Comment 37

[Ismael Gonzalez [:basiclines]]

Comment on attachment 753807 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9990

Everything looks nice! Thx Alberto!

Comment 38

[alberto.pastor]

https://github.com/mozilla-b2g/gaia/commit/d1d250f7e7074841dcec106e41f1ffd1671747f1

Comment 39

[Fabien Cazenave [:kaze]]

Note: the associated l10n strings have been forgotten, see bug 865079.

We must uplift this patch to v1-train (along with 865079?) to fix bug 877046 (leo+).

Comment 40

[David Scravaglieri [:scravag]]

Blocks a blocker

Comment 41

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

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 d1d250f7e7074841dcec106e41f1ffd1671747f1
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 42

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

v1.1.0hd: e6666a2135244f75ff793aae790feeddbeae8832
v1.1.0hd: 0afe7dd285f75ff9a761186aade7103c8a2ccdd2

Comment 43

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

Fabien, please make sure you update the bug with the commit id and set status flags when you update the bug.

$ git branch --contains e6666a2135244f75ff793aae790feeddbeae8832
  v1-train
* v1.1.0hd

Comment 44

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

Hi John, this is weird since I see the updates included in d1d250f7e7074841dcec106e41f1ffd1671747f1 already in v1-train.
For example, comparing https://github.com/mozilla-b2g/gaia/blob/d1d250f7e7074841dcec106e41f1ffd1671747f1/apps/communications/dialer/js/call_log.js and https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/dialer/js/call_log.js we get http://diffchecker.com/rp1pvxw2 which only includes a minor change related to another bug/patch.
On the other hand, comparing https://github.com/mozilla-b2g/gaia/blob/d1d250f7e7074841dcec106e41f1ffd1671747f1/apps/communications/dialer/style/call_log.css and https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/dialer/style/call_log.css we get http://diffchecker.com/wu9yse0x where once again the changes are not related to this bug/patch. Did you manage to finally uplift the commit to v1-train or did I miss anything? Thanks!

Comment 45

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

(In reply to gtorodelvalle from comment #44)
> Hi John, this is weird since I see the updates included in
> d1d250f7e7074841dcec106e41f1ffd1671747f1 already in v1-train.
> For example, comparing
> https://github.com/mozilla-b2g/gaia/blob/
> d1d250f7e7074841dcec106e41f1ffd1671747f1/apps/communications/dialer/js/
> call_log.js and
> https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/dialer/
> js/call_log.js we get http://diffchecker.com/rp1pvxw2 which only includes a
> minor change related to another bug/patch.
> On the other hand, comparing
> https://github.com/mozilla-b2g/gaia/blob/
> d1d250f7e7074841dcec106e41f1ffd1671747f1/apps/communications/dialer/style/
> call_log.css and
> https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/dialer/
> style/call_log.css we get http://diffchecker.com/wu9yse0x where once again
> the changes are not related to this bug/patch. Did you manage to finally
> uplift the commit to v1-train or did I miss anything? Thanks!

Yes, it was uplifted.  If the changes aren't there anymore, they must've been overwritten by a new cherrypick or merge.

$ git log --grep d1d250f7e7074841dcec106e41f1ffd1671747f1 v1-train
commit 0afe7dd285f75ff9a761186aade7103c8a2ccdd2
Author: albertopq <[e-mail]>
Date:   Tue May 28 09:06:32 2013 -0700

    uplift: Bug 865079 - [Call log] Remove synchronous reflows
    (cherry picked from commit d1d250f7e7074841dcec106e41f1ffd1671747f1)
$ git branch --contains d1d250f7e7074841dcec106e41f1ffd1671747f1
  master
$ git branch --contains 0afe7dd285f75ff9a761186aade7103c8a2ccdd2
* v1-train
  v1.1.0hd