984399 - Contacts list is not properly updated when importing a new FB Friend

Status: VERIFIED FIXED

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

Description

[Jose Manuel Cantera]

STR:

Have a FB Account with more than 14 friends
Import all of them excepting the one which is on the top (name starts with A or the following)
Go to update FB Friends. Import the first friend (the one not imported before)
Then go to the Contacts List. The imported contact will appear without the photo and without the FB markers and information.

Comment 1

[Jason Smith [:jsmith]]

QA Wanted to check behavior on 1.3.

Comment 2

[Cristian Rodriguez (:crdlc)]

Hi,

   Basically I think that the regression was introduced here

https://github.com/mozilla-b2g/gaia/commit/3b22e091af653fd867c58ee1ce74fc293e262faa

   You can forget FB if you want, it happens adding a new contact with photo as well

STR:

   1) You have x contacts on the list in group 'A'
   2) Create a new one with photo that should be added to this group and it should appear on the viewport

Expected 

   The image should be displayed

Result:

   The image is not displayed before scrolling

I guess that if a contact is updated or created and it will be on the viewport, we should reload the image loader library. What do you think Ben?

Thanks a lot

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Cristian, can you try moving this:

    if (imgLoader) {
      needImgLoaderReload = true;
    }

Earlier in |addToList()|?  Basically, before the calls to |addToGroup()|.  Perhaps the mutation events are synchronous, so |onscreen()| fires before we have set |needImgLoaderReload = true|.

Comment 4

[Cristian Rodriguez (:crdlc)]

I can take a look without doubt, thx

Comment 5

[Cristian Rodriguez (:crdlc)]

This method [1] is not invoked when a contact is added, at least it is that I am seeing..

[1] var onscreen = function(row) {

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez (:crdlc) from comment #5)
> This method [1] is not invoked when a contact is added, at least it is that
> I am seeing..
> 
> [1] var onscreen = function(row) {

That's very surprising.  The visibility_monitor should be getting a mutation event indicating the new contact was inserted into the list.

Is this contact inserted via the |addToList()| function?  And the contact is in the viewable range of the list?

Comment 7

[Peter Bylenga [:PBylenga]]

    This issue does not reproduce on latest Buri v1.3 with the STRs found in Comment 0.  The updated contact receives a photo and facebook link icon.

v1.3 Environmental Variables:
    Device: Buri v1.3 MOZ
    BuildID: 20140317004001
    Gaia: 0ab8a9cbcef5f23cec904a3d7f7675e44de29951
    Gecko: f824e9d91a2d
    Version: 28.0
    Firmware Version: v1.2-device.cfg

Comment 8

[Cristian Rodriguez (:crdlc)]

(In reply to Ben Kelly [:bkelly] from comment #6)
> (In reply to Cristian Rodriguez (:crdlc) from comment #5)
> > This method [1] is not invoked when a contact is added, at least it is that
> > I am seeing..
> > 
> > [1] var onscreen = function(row) {
> 
> That's very surprising.  The visibility_monitor should be getting a mutation
> event indicating the new contact was inserted into the list.
> 
> Is this contact inserted via the |addToList()| function?  And the contact is
> in the viewable range of the list?

I think so...

case 'create':
  contactsList.refresh(event.contactID, checkPendingChanges,....

refresh -> refreshContact -> remove(contact.id); addToList(contact, enriched);

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.4+ regression

Comment 10

[David Scravaglieri [:scravag]]

Jose, could you take this one ?

Comment 11

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

Just to clarify, the issue was when we try to add a new item as first or last element in the scrollable container. In that case, the mutation observer was woken up when the new node was inserted in the DOM but it had defined legacy state.firstChild and state.lastChild variables at that moment and it needs re-compute them in order to check if the new one is the first or last in the list

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8393409 [details]
Patch v1

Since this is a change to tag_visibility_monitor.js in shared/js, I think we need review from David.  Also, lets get feedback from Evan as the tag_visibility_monitor author.

Overall this looks ok to me.  My one thought was, can limit our calls to |recomputeFirstAndLastOnscreen()| to only one per invocation of |mutationHandler()|?

I think the computation can be expensive, trigger sync reflows, etc.  Also, the state of the DOM should be fixed even if we have not looked at all the mutations yet.  So the first and last elements should not change while we are iterating within |mutationHandler()|.

Comment 13

[Cristian Rodriguez (:crdlc)]

I agree with you, we could move 

mutation.addedNodes.length && recomputeFirstAndLastOnscreen(); 

to:

https://github.com/crdlc/gaia/blob/bug-984399/shared/js/tag_visibility_monitor.js#L190

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Also, really nice job tracking this down!

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez (:crdlc) from comment #13)
> I agree with you, we could move 
> 
> mutation.addedNodes.length && recomputeFirstAndLastOnscreen(); 
> 
> to:
> 
> https://github.com/crdlc/gaia/blob/bug-984399/shared/js/
> tag_visibility_monitor.js#L190

Hmm, so not check for |child.nodeType === ELEMENT_NODE && child.tagName === tag| before doing the recompute?  I guess that's fair as the addition of other nodes may cause things to shift around on the screen.

I guess I was more just thinking of wrapping your existing call to recompute in an |if (!recomputed) { recomputed=true; recompute(); }|.

Comment 16

[Cristian Rodriguez (:crdlc)]

Addressed your comment and added the if statement, thx mate

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

set target milestone to 1.4 S4 for now

Comment 18

[Cristian Rodriguez (:crdlc)]

Hi David, when could you take a look at this one? Thanks

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

David, if you feel comfortable doing so, you could delegate the review to me.  I think I'm the only other person who has looked at tag_visibility_monitor.js besides you and Evan.

Cristian, looking at it again, I think it might be nice to extend the unit test to catch this problem.  So something that fails without your fix and succeeds with your fix.  If I recall correctly the tests are pretty well factored and it should not be too difficult.  They are under gallery.

Comment 20

[Cristian Rodriguez (:crdlc)]

Done Ben

Comment 21

[David Flanagan [:djf]]

Comment on attachment 8393409 [details]
Patch v1

Delegating the review to Ben. I wrote the original visibility monitor, but have never really been familiar with this version of it.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8393409 [details]
Patch v1

The new test looks good and travis is green.  r=me if you have tested this on device.

Thanks!

Comment 23

[Cristian Rodriguez (:crdlc)]

I tested it but I thought that reviewers do it as well :)

Comment 24

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/42541cd4751fb37bb6c866fc404c53328af55712

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Cristian Rodriguez (:crdlc) from comment #23)
> I tested it but I thought that reviewers do it as well :)

I normally do, but my devices currently all have badly hacked up gecko... so it would take me an hour or so to test then get back to my current state.  Since we have a new unit test specifically for this I was willing to rely on your device testing.  Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/011e5ab15a27f75de908c69d40177b3a5f44b344

Comment 27

[Loli (:lolimartinezcr)]

Today's (4/2) master build and working:
Device: Hamachi
BuildId: 20140402071651
Gaia: adde0f6
Gecko: 011a30c
Platform version: 31.0a1

Today's (4/2) 1.4 build and working:
Device: Hamachi
BuildId: 20140402094724
Gaia: b8e6e61
Gecko: 6bfa4cb
Platform version: 30.0a2