Closed Bug 1135660 Opened 7 years ago Closed 7 years ago

No transition when delete-app dialog appears on homescreen

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S7 (6mar)
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: cwiiis, Assigned: cwiiis)

Details

(Keywords: polish, Whiteboard: [systemsfe])

Attachments

(2 files)

Unlike most other dialogs on the system, the delete-app dialog in the homescreen is missing a transition.
Comment on attachment 8570413 [details] [review]
[gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master

Whoops, fixed this a couple of days ago and forgot to actually do the pull request...
Attachment #8570413 - Flags: review?(kgrandon)
Comment on attachment 8570413 [details] [review]
[gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master

I like it when it works, but it seems to not work for me the very first time you open the dialog. After that it seems the transition works. I think it's pretty important that it works on the first time since it's not a common action, and the home screen can be oom killed, etc. Chris - does it work for you on the very first dialog opening?

I'm also seeing this in the logcat, but I'm not sure if it was a prior error of if we're introducing this here: W/Homescreen( 2370): [JavaScript Error: "TypeError: this.element is null" {file: "app://verticalhome.gaiamobile.org/shared/js/homescreens/confirm_dialog_helper.js" line: 100}]
Flags: needinfo?(chrislord.net)
Attachment #8570413 - Flags: review?(kgrandon) → review-
(In reply to Kevin Grandon :kgrandon from comment #3)
> Comment on attachment 8570413 [details] [review]
> [gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master
> 
> I like it when it works, but it seems to not work for me the very first time
> you open the dialog. After that it seems the transition works. I think it's
> pretty important that it works on the first time since it's not a common
> action, and the home screen can be oom killed, etc. Chris - does it work for
> you on the very first dialog opening?

hmm, I thought I fixed this... I guess not :/

> I'm also seeing this in the logcat, but I'm not sure if it was a prior error
> of if we're introducing this here: W/Homescreen( 2370): [JavaScript Error:
> "TypeError: this.element is null" {file:
> "app://verticalhome.gaiamobile.org/shared/js/homescreens/
> confirm_dialog_helper.js" line: 100}]

Not sure, but I'll look on Monday.
Comment on attachment 8570413 [details] [review]
[gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master

Both issues fixed. What's odd is that the setTimeout really did require about 100ms. No time didn't work, 20ms (which usually works) didn't work, 50ms didn't work, requestAnimationFrame didn't work... But 100ms was fine. I could hypothesise, but to be honest, I just don't know...
Flags: needinfo?(chrislord.net)
Attachment #8570413 - Flags: review- → review?(kgrandon)
(In reply to Chris Lord [:cwiiis] from comment #5)
> Both issues fixed. What's odd is that the setTimeout really did require
> about 100ms.

Hmm, sounds fishy, I will try to take a look at this soon. I wonder if it's waiting for layer creation or something? 100ms is a bit long =/
(In reply to Kevin Grandon :kgrandon from comment #6)
> (In reply to Chris Lord [:cwiiis] from comment #5)
> > Both issues fixed. What's odd is that the setTimeout really did require
> > about 100ms.
> 
> Hmm, sounds fishy, I will try to take a look at this soon. I wonder if it's
> waiting for layer creation or something? 100ms is a bit long =/

mmm, I wonder if it could be replaced with a nested requestAnimationFrame? If that worked, do you think it'd be preferable? (I'm not really sure either way tbh...) I imagine the nested rAf might be more reliable, but probably also trigger one or two more composites? But probably not worth trying to save single composites in the lead up to an animation anyway...
I would love to find some alternative to the 100ms timeout, but not sure what the best option is yet. I wonder if we trigger a synchronous reflow with something like document.body.clientTop would change anything?
(In reply to Kevin Grandon :kgrandon from comment #8)
> I would love to find some alternative to the 100ms timeout, but not sure
> what the best option is yet. I wonder if we trigger a synchronous reflow
> with something like document.body.clientTop would change anything?

I couldn't get any combination of style flushes, including coupling them with lower-timed setTimeouts to work. On the other hand, a nested requestAnimationFrame does work (so raf(() => { raf(() => { ... }); });) - Perhaps it's worth going with that, with a comment that it's a gross work-around, but that a better work-around couldn't be found (for now)?
(In reply to Chris Lord [:cwiiis] from comment #9)
> I couldn't get any combination of style flushes, including coupling them
> with lower-timed setTimeouts to work. On the other hand, a nested
> requestAnimationFrame does work (so raf(() => { raf(() => { ... }); });) -

It's gross, but less gross than the timeout I think so let's go with that!
(In reply to Kevin Grandon :kgrandon from comment #10)
> (In reply to Chris Lord [:cwiiis] from comment #9)
> > I couldn't get any combination of style flushes, including coupling them
> > with lower-timed setTimeouts to work. On the other hand, a nested
> > requestAnimationFrame does work (so raf(() => { raf(() => { ... }); });) -
> 
> It's gross, but less gross than the timeout I think so let's go with that!

Done, and change pushed. I'll see if someone from layout can help me at some point in diagnosing what the heck is going on here, and perhaps a nicer work-around. Perhaps animations would work out better here, despite their horribly verbose mark-up...

This is only a temporary solution anyway, I hope that we'll move the half-dozen (not an exaggeration) different dialogs to all use the same gaia component in the v3 cycle.
Comment on attachment 8570413 [details] [review]
[gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master

Great job. Please make sure gaia-try passes before trying to land. Thanks!
Attachment #8570413 - Flags: review?(kgrandon) → review+
Tests are green.
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#041Xp2msRiuIt9iT6qC1lg

The pull request failed to pass integration tests. It could not be landed, please try again.
goddamnit, tests didn't retrigger, for no discernable reason, so I guess tests aren't actually green...? Though there are no significant changes since the all-green 7 hours ago... I'll sort this out tomorrow.
(In reply to Chris Lord [:cwiiis] from comment #15)
> goddamnit, tests didn't retrigger, for no discernable reason, so I guess
> tests aren't actually green...? Though there are no significant changes
> since the all-green 7 hours ago... I'll sort this out tomorrow.

Actually I think the tests might be ok, it might be some infra issue with taskcluster. I'll do some investigation, and in the meantime we might want to land manually.
Let me just try doing this again to see if anything bad happens.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Ok, seems this landed after all. This is possibly infrastructure related (something wrong with autolander/taskcluster), or potentially related to github availability. Not sure what's going on here, but will keep my eye on this and try to extract some useful information from the logs.
Comment on attachment 8570413 [details] [review]
[gaia] Cwiiis:bug1135660-homescreen-no-delete-transition > mozilla-b2g:master

Thanks for taking care of this Kevin!

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Some (more) dialogs appear with no transition, where as most do. Makes the phone feel broken/unpolished/janky
[Testing completed]: Tested manually and use of the dialog is covered by automated tests (I think)
[Risk to taking this patch] (and alternatives if risky): Low risk
[String changes made]: None
Attachment #8570413 - Flags: approval-gaia-v2.2?
Attachment #8570413 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This bug has been verified as "pass" on latest nightly build of Flame v2.2&Master.

STR:
1. Long tap any icon on Homescreen.
2. Tap 'X' icon, then the delete-app dialog appears, and then you can tap the 'Cancel' button.

Actual results: Showing the transition when delete-app dialog appears/disappears on homescreen.
See attachment: verified_Flame_v2.2.3gp
Reproduce rate: 0/10


Device: Flame v2.2 (Verified) 
Build ID               20150714162501
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a5db6d9850f6
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150714.200955
Firmware Date          Tue Jul 14 20:10:07 EDT 2015
Bootloader             L1TC000118D0

Device: Flame master (Verified)
Build ID               20150714160203
Gaia Revision          803d04e3829fd4fe9261211aa0ddca6b79d4e328
Gaia Date              2015-07-14 17:54:44
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d5025c151d17
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150714.193145
Firmware Date          Tue Jul 14 19:31:57 EDT 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.