834117 - Failure when we move an icon quickly over panels

Status: VERIFIED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

Attachment: Clo*c*king device.

STR:
 - Move the "Clock" icon to a newly created right-most panel.
 - Move the "Music" icon to a newly created right-most panel (after the one with the clock).
 - Move the "Clock" to any where else.

Expected:
 - Leave a blank panel, while moving icons.
 - Remove the panel after finishing the edition.

Seen:
 - … look at the screenshot …

Comment 1

[Kai-Chih Hu [:khu]]

Can't reproduce this issue.

Comment 2

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

can you provide a video? or try on latest build?
please renom if reproduced

Comment 3

[Nicolas B. Pierron [:nbp]]

I was able to reproduce it on the last test-driver update.  So I renominate this bug.

Now, the problem is that I am no longer able to reproduce it to take a video, since I had to restart my phone to get rid of the extra icon and that it does not boot anymore …

Comment 4

[Julien Wajsberg [:julienw]]

I reproduce this.

The last step is not necessary, you merely need to long press the clock icon to enter edit mode.

Comment 5

[Julien Wajsberg [:julienw]]

The error shown in logcat is :

E/GeckoConsole(  378): [JavaScript Error: "TypeError: children[i] is undefined" {file: "app://homescreen.gaiamobile.org/js/page.js" line: 605}]

IMHO it could even be TEF+ but I let drivers choose.
Taking.

Comment 6

[Julien Wajsberg [:julienw]]

Actually the title is not precise enough: I've seen that this can happen in other cases too. However the STR provided by Nicolas makes it easily reproducible.

Comment 7

[Cristian Rodriguez (:crdlc)]

Hi Julien, do you want that I take a look? thanks

the STR are perfect!

Comment 8

[Cristian Rodriguez (:crdlc)]

here 

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.js#L583

if (originIcon && targetIcon && this.olist.children.length > 1) {

and works fine

Comment 9

[Julien Wajsberg [:julienw]]

Cristian, as I said earlier, it happens also in other situation; for example, i've seen it with length == 6 :)

I believe the caclulation inside "animate" is wrong, I'm currently trying to understand this and I'll most probably have a patch later today (or maybe tomorrow if I'm brave enough to write unit tests) :) I'll ask you for review !

Comment 10

[Cristian Rodriguez (:crdlc)]

OK, in that case you are right. I haven't seen it in other situations. Perfect, I can review it, thanks a lot

Comment 11

[Julien Wajsberg [:julienw]]

Ok, the correct STR is:
* don't be on the last panel, but any other panel with icons is fine
* long press to go in edit mode
* drag and drop an icon to the right, quickly go over the next panel and land it to the panel after
* go back to the previous panel and long press on one icon

Expected:
* as usual, we want to be able to move this icon

Actual:
* it stays at the same place in a bigger size, even when you switch panels, and the Homescreen doesn't answer to event anymore.

This is serious because we actually go quickly over panels if we don't move the finger after moving to one panel.

This doesn't happen when we go back however.

Comment 12

[Jason Smith [:jsmith]]

Did you unintentionally renom?

Comment 13

[Julien Wajsberg [:julienw]]

of course not, sorry about that..

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Apply the patch with |git am|.
See also the PR in https://github.com/mozilla-b2g/gaia/pull/7847.

We keep the list of icons that we animated so that we can remove the animation
exactly for the same list.

Also slightly refactored dragdrop's `overIconGrid` function. We can do more but
I didn't want to do more in a TEF+ bug.

Also fixed a bug occurring when the user tried to move an icon below the dock.
---
 apps/homescreen/js/dragdrop.js |   56 +++++++++++++++++++++-------------------
 apps/homescreen/js/page.js     |   33 +++++++++++++----------
 2 files changed, 48 insertions(+), 41 deletions(-)

Didn't add any test because I couldn't make it work with the animationend event.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Fixed most remarks, thanks !

Apply the patch with |git am|
PR: https://github.com/mozilla-b2g/gaia/pull/7847

We keep the list of icons that we animated so that we can remove the animation
exactly for the same list.

Also refactored dragdrop's `overIconGrid` function. We probably can do more but
I didn't want to do more in a TEF+ bug. I tried to fail fast with boolean values
when before we were sometimes doing expensive checks before checking these bool.

Also fixed a bug occurring when the user tried to move an icon below the dock.

Last but not least, we now don't try to animate when there is only one icon
(that is the icon being dragged) in a page.
---
 apps/homescreen/js/dragdrop.js |   94 ++++++++++++++++++----------------------
 apps/homescreen/js/page.js     |   55 ++++++++++++++++-------
 2 files changed, 83 insertions(+), 66 deletions(-)

Comment 16

[Cristian Rodriguez (:crdlc)]

thanks a lot, reviewing

Comment 17

[Cristian Rodriguez (:crdlc)]

Comment on attachment 707639 [details] [diff] [review]
patch v2

All works fine, great work! thanks a lot

Comment 18

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/7055b7908825a4c248b053c0526b748a10554b1d

Comment 19

[Mila Davydova]

This issue does not reproduce on Unagi build 20130214070203 with Dec 5th Kernel
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49

No errors happen when moving an icon quickly over panels.