Closed
Bug 984399
Opened 11 years ago
Closed 11 years ago
Contacts list is not properly updated when importing a new FB Friend
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: jmcf, Assigned: crdlc)
Details
(Keywords: regression)
Attachments
(1 file)
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.
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.4?
Reporter | ||
Updated•11 years ago
|
QA Contact: lolimartinezcr
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bkelly)
Comment 3•11 years ago
|
||
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|.
Flags: needinfo?(bkelly) → needinfo?(crdlc)
Assignee | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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
Keywords: qawanted
Assignee | ||
Comment 8•11 years ago
|
||
(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);
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 11•11 years ago
|
||
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
Attachment #8393409 -
Flags: review?(bkelly)
Flags: needinfo?(jmcf)
Updated•11 years ago
|
Assignee: nobody → crdlc
status-b2g-v1.4:
--- → affected
Comment 12•11 years ago
|
||
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()|.
Attachment #8393409 -
Flags: review?(dflanagan)
Attachment #8393409 -
Flags: review?(bkelly)
Attachment #8393409 -
Flags: feedback?(eshapiro)
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
Also, really nice job tracking this down!
Comment 15•11 years ago
|
||
(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(); }|.
Assignee | ||
Comment 16•11 years ago
|
||
Addressed your comment and added the if statement, thx mate
Updated•11 years ago
|
Attachment #8393409 -
Flags: feedback+
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 17•11 years ago
|
||
set target milestone to 1.4 S4 for now
Target Milestone: --- → 1.4 S4 (28mar)
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 18•11 years ago
|
||
Hi David, when could you take a look at this one? Thanks
Flags: needinfo?(dflanagan)
Comment 19•11 years ago
|
||
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.
Assignee | ||
Comment 20•11 years ago
|
||
Done Ben
Comment 21•11 years ago
|
||
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.
Attachment #8393409 -
Flags: review?(dflanagan) → review?(bkelly)
Flags: needinfo?(dflanagan)
Comment 22•11 years ago
|
||
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!
Attachment #8393409 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 23•11 years ago
|
||
I tested it but I thought that reviewers do it as well :)
Assignee | ||
Comment 24•11 years ago
|
||
Merged in master: https://github.com/mozilla-b2g/gaia/commit/42541cd4751fb37bb6c866fc404c53328af55712
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 25•11 years ago
|
||
(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•11 years ago
|
||
v1.4: https://github.com/mozilla-b2g/gaia/commit/011e5ab15a27f75de908c69d40177b3a5f44b344
status-b2g-v2.0:
--- → fixed
Comment 27•11 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Attachment #8393409 -
Flags: feedback?(eshapiro)
You need to log in
before you can comment on or make changes to this bug.
Description
•