Closed Bug 911643 Opened 11 years ago Closed 11 years ago

[Homescreen] Big icons are overlaped over normal icons

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Homescreen
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Milestone:
1.1 QE6
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
b2g18 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: leo.bugzilla.gaia, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Reproduced_step.mp4
1.99 MB, video/mp4
Details
Result.mp4
1.19 MB, video/mp4
Details
911643-v2.patch
2.49 KB, patch
crdlc
: feedback?
leo.bugzilla.gaia
Details | Diff | Splinter Review 
1. Title : Big icons are overlaped over normal icons
2. Precondition : 
3. Tester's Action : Do the automatic testing using monkey test. 
4. Detailed Symptom (ENG.) : Big icons are overlaped over normal icons
5. Personal email id: promise09th@gmail.com
Attached video Reproduced_step.mp4 — 
Reproduced step.

It is hard to reproduced.
Attached video Result.mp4 — 
Result video
eproduced step :

1. move icon from dock bar to last icon of screen
2. Repeat no 1 until last icon is not shown when user move icon to dock bar.
3. Move icon to home screen page(Not release)
4. Release current icon and select other icon at the same time.

When I try to follow this steps, this issue is reproduced about 1/10

Please check this
Sorry, I missed information.

My test version is already applied Bug 906536 patch
Hi Julien,

 I guess that this the opportunity for other person takes care of this bug because I cannot reproduce it. Although this issue is reproduced about 1/10 according to reporter and furthermore the steps to reproduce are very weird and IMHO I have doubts that real users will do that. Could you take a look?

Thanks
Flags: needinfo?(felash)
Just out of curiosity, I've just seen the last video and ev.me is on the left of the landing page. AFAIK ev.me is on the landing page right now in v1-train. Maybe I don't have enough info and Leo devices have a custom v1-train.

Thanks
I'm renominating because this happens very very rarely and there is an easy workaround (reboot the phone). So imho this should not block the 1.1 release.

Leo, it would help a lot if you could attach a log of when this happens:

  adb logcat | grep Gecko | tee logfile.txt

and attach logfile.txt to this bug.


That said, Cristian, do you think we could make sure that everything is "correct" when the user presses "home" several times ? Eg when he presses "home" and goes to the landing page ? So like re-render everything in that case, if we detect that the use presses "home" a lot.
blocking-b2g: leo+ → leo?
Flags: needinfo?(promise09th)
Flags: needinfo?(leo.bugzilla.gaia)
Flags: needinfo?(felash)
Umm good idea, I try to polish your excellent idea a bit:

"If an user clicks on home button and there is a problem (document.querySelector('body > div.draggable') is defined) we can repaint the grid as you said"
And maybe we should have something to reload completely homescreen. (ie: homescreen would call window.close() and the system would relaunch it automatically). Or this could be done in the Settings app with the new installable/pluggable homescreen feature.

but already we can do what you propose (in another bug :) )
if ("Home button was clicked" && document.querySelector('body > div.draggable') {
  window.close();
}

Very easy theoretically :)
 Becase of security problems, I can't attach full log.
But, I found error log when this issue is reproduced

[JavaScript Error: "TypeError: draggableElem.parentNode is null" {file: "app://homescreen.gaiamobile.org/gaia_build_defer_index.js" line: 76}]

please check this log.
Flags: needinfo?(leo.bugzilla.gaia)
Flags: needinfo?(promise09th)
Cristian, does this log help you or do you want that I look this up ?

(In reply to Cristian Rodriguez (:crdlc) from comment #10)
> if ("Home button was clicked" && document.querySelector('body >
> div.draggable') {
>   window.close();
> }
> 
> Very easy theoretically :)

I would even do : "if ("home button was clicked 3 times in 5 seconds") { window.close() }"
Attached patch 911643.patch for v1-train (obsolete) — — Splinter Review 
I don't have time to take care of this bug although please apply this patch in order to avoid this error seen in traces. I hope it helps with the problem
Attachment #799316 - Flags: feedback?(leo.bugzilla.gaia)
Comment on attachment 799316 [details] [diff] [review]
911643.patch for v1-train

Review of attachment 799316 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/homescreen/js/page.js
@@ +507,4 @@
>        if (draggableElem) {
>          var img = draggableElem.querySelector('img');
>          window.URL.revokeObjectURL(img.src);
> +        document.body.removeChild(draggableElem);

actually, if parentNode is null, it means draggableElem is somehow already removed.

So maybe we try to remove it twice and that is the root cause of the problem ? 

Also, why don't you set this.draggableElem to null ? Maybe that's the root cause ? :)

Your patch should work though :) but it looks like a workaround and I hope we won't run into other problems because we would do some things twice.
I know Julien, but as I said, I cannot take this one because I am busy with the new version of homescreen, feel free to assign to you if you have time :) It is not leo+ and it is extremely difficult to reproduce...
I understand and I agree :)
blocking-b2g: leo? → leo+
Can you please explain the rationale why such a difficult to reproduce bug is blocking the release ? Don't we have more important bugs to work on ?
Because if this issue is reproduced next test(QE or QA), our QE team think this issue should be fixed.

It is hard to reproduce, but this issue is very critical

So, we change flag from leo? to leo+
(In reply to Leo from comment #18)
> Because if this issue is reproduced next test(QE or QA), our QE team think
> this issue should be fixed.
> 
> It is hard to reproduce, but this issue is very critical
> 
> So, we change flag from leo? to leo+

Mozilla doesn't agree here. I tend to agree with Julien on this one - I don't think this is a blocker.
blocking-b2g: leo+ → leo?
Ok, I ask QE team, and I reply soon.
Leo,

Any news on this bug. We will be moving this to minus if we don't hear back by 9/16.
Flags: needinfo?(leo.bugzilla.gaia)
Minusing per comment 21
blocking-b2g: leo? → -
I set this as leo+ because this issue is reproduced during normal usage (not stress test) and become a blocker for shipment.
blocking-b2g: - → leo+
(In reply to Jinyoon from comment #23)
> I set this as leo+ because this issue is reproduced during normal usage (not
> stress test) and become a blocker for shipment.
Could you please explain which build are you using to reproduce this issue?
(In reply to Leo from comment #3)
> eproduced step :
> 
> 1. move icon from dock bar to last icon of screen

> How many icons in dock? How many icons in grid?

> 2. Repeat no 1 until last icon is not shown when user move icon to dock bar.

I don't understand this step

> 3. Move icon to home screen page(Not release)
> 4. Release current icon and select other icon at the same time.
> 
> When I try to follow this steps, this issue is reproduced about 1/10
> 
> Please check this
Attached patch 911643.patch (obsolete) — — Splinter Review 
For v1-train of course
Attachment #799316 - Attachment is obsolete: true
Attachment #799316 - Flags: feedback?(leo.bugzilla.gaia)
Attachment #810567 - Flags: feedback?(leo.bugzilla.gaia)
Could Leo test this patch? Thanks a lot
(In reply to Jinyoon from comment #23)
> I set this as leo+ because this issue is reproduced during normal usage (not
> stress test) and become a blocker for shipment.

That's not a strong argument for blocking. The fact that it took this long to get another reproduction reinforces Julien's argument on probability of occurrence - it's too low to block. We are extra sensitive to taking anything on v1-train at this point, so I don't think this blocks.
blocking-b2g: leo+ → leo?
(In reply to Jason Smith [:jsmith] from comment #28)
> (In reply to Jinyoon from comment #23)
> > I set this as leo+ because this issue is reproduced during normal usage (not
> > stress test) and become a blocker for shipment.
> 
> That's not a strong argument for blocking. The fact that it took this long
> to get another reproduction reinforces Julien's argument on probability of
> occurrence - it's too low to block. We are extra sensitive to taking
> anything on v1-train at this point, so I don't think this blocks.

In talking to the reporter, apparently a QA engineer saw this immediately. Maybe sometimes it takes a few times, and sometimes immediately.

Regardless, we're far past the point of Mozilla determining what blocks for 1.1. We need the patch ready as soon as possible for all manufacturers to take, and each manufacturer can determine whether to apply it to their builds or not.
Smaller STRs:
1) have a full page of icons, take one icon from the dock/hotseat and while the icon is still in the dock paginate the last icon on the homescreen, if the last icon in the dock doesn't move back to the page, then you reached one part of the condition; you don't have to circle the icon.

2) move the icon back in the dock and then to the top of the homescreen, don't let go

3) have another finger ready to press a different icon, let go of the original moving icon and immediately press the second icon [ You can see this at 0:23 of the original video ]
see video for shorter method than circling : 
http://www.youtube.com/watch?v=UhlV9RHj74A&feature=youtube_gdata_player

It has to do with the position of the icon in the dock and the last icon paginating.
With this patch I haven't hit it once following the steps out I outlined as well as trying to hit the home button and also launching an app.  In all cases the icon that is to be moved is on the next page.
Marking leo+ as it's considered a blocker for our partner.  Comment 29 is the right way to look at this.   

Sounds like this patch still needs a review?  Can we get this in someone's queue asap?

Leo, will this go into the current build or in an MR?  Thanks.
blocking-b2g: leo? → leo+
Sorry, I retested again and I was able to hit it even with the patch.

I forgot that I had to have 2 pages full of icons; having more icons and then each icon having to cascade down from the first icon paginating will cause a longer delay.  The patch does not fix the issue.
Summary of "simplified" steps to reproduce
1. have at least 2 full pages of icons
2. go to the first page and position an icon from the dock to push the last icon to the next page while still in the dock,
3. then loop down to the dock and around to have the last icon not coming back to the page. [ see http://www.youtube.com/watch?v=UhlV9RHj74A&feature=youtube_gdata_player ]
4. have a finger ready to press another icon right after release the first icon being dragged.
[ see 0:23 from the original video ]
5. after you let go of the first icon (the dragging icon) immediately press the other icon with your left hand.
(In reply to Julien Wajsberg [:julienw] from comment #9)
> And maybe we should have something to reload completely homescreen. (ie:
> homescreen would call window.close() and the system would relaunch it
> automatically). Or this could be done in the Settings app with the new
> installable/pluggable homescreen feature.

In case you don't know this is already done.(Auto relaunch homescreen once it's closed by itself or crashed.)

> 
> but already we can do what you propose (in another bug :) )
Attached patch 911643-v2.patch — — Splinter Review 
Hi Leo,

   I was trying your STR and during 60-70 seconds. I was dragging the last icon of the dock from there to last position of the grid and viceversa as fast as I could but the bug is not reproducible by myself. With my patch I cannot reproduce  the problem, the 16th icons is still there (without patch that was reproducible).

   Further info (build identifier 20130721095109 and current v1-train)

   BTW and the most important thing is that this patch implements a fallback based on the following idea:

"If an user exits from edit mode and the draggable icon is still scaled (big icon) when it clicks on home button again, the homescreen will be launched again to re-paint the layout correctly.

Thanks a lot
Attachment #810567 - Attachment is obsolete: true
Attachment #810567 - Flags: feedback?(leo.bugzilla.gaia)
Attachment #811016 - Flags: feedback?(leo.bugzilla.gaia)
I have no idea how this bug happens but I found the patch of Bug 885625 (which doesn't appears in v1-train branch) might prevent the following situation:

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #35)
> 3. then loop down to the dock and around to have the last icon not coming
> back to the page.
I am not sure if I did something wrong in patching when using 911643-v2.patch, but I seem to have a white screen flash when I try to hit the home button now.
I believe I see the same thing as lchang; I can't seem to produce the first part of the step with the patch in bug 885625.

in order to add his patch in a new branch:
git remote add crdlc https://github.com/crdlc/gaia.git
git fetch crdlc bug-885625
git cherry-pick 2169791fa95bb4df2a3079a9a3ff23e30c99115c
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #39)
> I am not sure if I did something wrong in patching when using
> 911643-v2.patch, but I seem to have a white screen flash when I try to hit
> the home button now.

If you reboot your device and you go to second page and click on home button, do you have a white screen? It is working for me in build identifier 20130721095109 and current v1-train. I don't have idea if the patch was applied cleanly or it fails in other build. Is it happening to other person?
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #40)
> I believe I see the same thing as lchang; I can't seem to produce the first
> part of the step with the patch in bug 885625.
> 
> in order to add his patch in a new branch:
> git remote add crdlc https://github.com/crdlc/gaia.git
> git fetch crdlc bug-885625
> git cherry-pick 2169791fa95bb4df2a3079a9a3ff23e30c99115c

And for sure, this patch that I implemented months ago should be on v1-train IMHO but was leo- at that moment
(In reply to Cristian Rodriguez (:crdlc) from comment #37)
> Created attachment 811016 [details] [diff] [review]
> 911643-v2.patch
> 
> Hi Leo,
> 
>    I was trying your STR and during 60-70 seconds. I was dragging the last
> icon of the dock from there to last position of the grid and viceversa as
> fast as I could but the bug is not reproducible by myself. With my patch I
> cannot reproduce  the problem, the 16th icons is still there (without patch
> that was reproducible).
> 
>    Further info (build identifier 20130721095109 and current v1-train)
> 
>    BTW and the most important thing is that this patch implements a fallback
> based on the following idea:
> 
> "If an user exits from edit mode and the draggable icon is still scaled (big
> icon) when it clicks on home button again, the homescreen will be launched
> again to re-paint the layout correctly.
> 
> Thanks a lot

We want to know logic of this patch because we explain this patch of logic to our QE Team.
Can you explain?
Flags: needinfo?(leo.bugzilla.gaia) → needinfo?(crdlc)
According to this comment we don't need any patch to fix this problem. Please renominate this one bug 885625

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #40)
> I believe I see the same thing as lchang; I can't seem to produce the first
> part of the step with the patch in bug 885625.
> 
> in order to add his patch in a new branch:
> git remote add crdlc https://github.com/crdlc/gaia.git
> git fetch crdlc bug-885625
> git cherry-pick 2169791fa95bb4df2a3079a9a3ff23e30c99115c
Flags: needinfo?(crdlc)
(In reply to Cristian Rodriguez (:crdlc) from comment #45)
> According to this comment we don't need any patch to fix this problem.
> Please renominate this one bug 885625

So should we dupe to bug 885625 and switch it to leo+ then?
(In reply to Jason Smith [:jsmith] from comment #46)
> (In reply to Cristian Rodriguez (:crdlc) from comment #45)
> > According to this comment we don't need any patch to fix this problem.
> > Please renominate this one bug 885625
> 
> So should we dupe to bug 885625 and switch it to leo+ then?

Actually what might be better here is to see if this reproduces on trunk. If it doesn't, then that will confirm your analysis.
Keywords: qawanted
OK for me perfect
QA Contact: ssuresh
I was not able to reproduce this issue on leo master. Tried around 12 times.

Environmental Variables
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/c630289d6388
Gaia: 02b975e6ce12922928c74276ac7d19432a03f126
Platform Version: 18.0

However I was able to reproduce this issue with only 5 tries on Leo v1.1 ComRil
Keywords: qawanted
Sounds like Cristian is right based on comment 49 - we need bug 885625 on v1-train. Marking as a dependency.
Depends on: 885625
Cristian,

How exactly is the patch in bug 885625 fixing the issue ? Are we convinced that it is the right path forward here ? Can you help understand the risk of adding that patch to 1.1 at this time. Scope of new regressions ? What kind of testing would QA have to perform to ensure nothing else is regressed ?

Based on the above we can leo+ bug 885625 as this bug is already determined to be a blocker per comment# 29 and #33
Flags: needinfo?(crdlc)
This patch [1] has a risk close to null. What kind of testing? Enter in edit mode and drag apps. That code is only performed dragging apps.

Thanks

[1] https://github.com/crdlc/gaia/commit/2169791fa95bb4df2a3079a9a3ff23e30c99115c
Flags: needinfo?(crdlc)
(In reply to Cristian Rodriguez (:crdlc) from comment #52)
> This patch [1] has a risk close to null. What kind of testing? Enter in edit
> mode and drag apps. That code is only performed dragging apps.
> 
> Thanks
> 
> [1]
> https://github.com/crdlc/gaia/commit/2169791fa95bb4df2a3079a9a3ff23e30c99115c

Naoki & one of our contractors know how to reproduce the original bug, so I'm not concerned about not having a clear path to testing this. We know to test this bug.
Fixed per landing in https://bugzilla.mozilla.org/show_bug.cgi?id=885625#c16.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Uplifted 2169791fa95bb4df2a3079a9a3ff23e30c99115c to:
v1.2 already had this commit
status-b2g-v1.2: --- → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: