Closed Bug 949470 Opened 6 years ago Closed 6 years ago
[Contacts] Broken image links when entering the search mode
TO REPRODUCE: 1. Open the contacts app and include (or scroll to) some contacts with photos as shown in attachment 1 [details] [diff] [review]. Having only 1 contact in view is enough. 2. Enter the "search mode" by clicking on the search bar. OBSERVED: 3. The contacts' photo links are broken as shown in attachment 2 [details] [diff] [review]. EXPECTED: 3. The contacts' photos are shown.
As far as my analysis, the problem is due to: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L832 in conjunction with the code which initializes the ImageLoader when entering the "search mode", and more concretely the one which checks if list item where the image is included has already been visited, this is: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/image_loader.js#L122 A possible and straightforward solution to the problem would be to include the following line: img.parentNode.parentNode.dataset.visited = false; after https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L832 but, apart from the ugly code :-) , since this may cause other regressions (mainly regarding performance) and there is probably a more efficient solution I would like to ask Ben about this issue to know his opinion about it :-) Thanks!
BTW, I found this issue, well in fact it was José Manuel :-p , using Gecko-f88ec24 and latest Gaia... Noemí is checking if it also reproduces in 1.3 right now :-)
(In reply to gtorodelvalle from comment #4) > BTW, I found this issue, well in fact it was José Manuel :-p , using > Gecko-f88ec24 and latest Gaia... Noemí is checking if it also reproduces in > 1.3 right now :-) Hi, It also occurs with today's (12/12) build in v1.3 branch so nominating to v1.3?: gaia: cbd2921 gecko: c8ebb7f BuildId: 20131212024136 Platform version: 28.0a2
blocking-b2g: --- → 1.3?
Can you try this untested patch? I think the issue is that when we clone the DOM node from the main list to the search list they end up sharing the same URL. Then when one the URL is revoked a stale reference is left in the other list. This patch tries to produce a separate URL in the search list. (Side issue, do we ever actually revoke the URLs in the nodes in the search list?? Sounds like a possible memory issue there.)
Flags: needinfo?(bkelly) → needinfo?(gtorodelvalle)
Hi Ben, the proposed patch solves the issue ;-) On the other hand and regarding the revoking of URLs in the nodes when in search mode and according to the observed behaviour, I would say we do revoke URLs :-) Would you send a PR with the proposed patch or would you like me to do it? I would ask José Manuel for revision to be completely sure about the issue. Thanks!
Flags: needinfo?(gtorodelvalle) → needinfo?(bkelly)
(In reply to gtorodelvalle from comment #7) > Would you send a PR with the proposed patch or would you like me to do it? I > would ask José Manuel for revision to be completely sure about the issue. > Thanks! I would be grateful if you could write the patch. I'm a bit buried investigating perf regressions at the moment. Thanks!
Assignee: nobody → gtorodelvalle
PR sent on behalf of Ben according to the patch proposed by him ;-)
Attachment #8349357 - Flags: review?(jmcf)
triage: 1.3+ regression
blocking-b2g: 1.3? → 1.3+
Comment on attachment 8349357 [details] 14806.html works ok, pleae check Travis before landing thanks
Attachment #8349357 - Flags: review?(jmcf) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Uplifted adba622b1072a94426ba618fb3e407e4393461df to: v1.3: d4f7cb3bb4bec8f28f1efedae99d638c1d003998
You need to log in before you can comment on or make changes to this bug.