996515 - [NFC] Screen shot does not have move back animation when video is failed to share

Status: VERIFIED FIXED

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

Description

[Alison Shiue]

Using most up-to-date pvt build (2014/4/14) to test
Two phones(Device A, Device B) with NFC

STR:
1. Enable NFC in settings
2. Device A open video
3. Touch two phones together
4. Swipe screen shot on device A
5. Remove device A from device B before start to transfer file

Expected result:
Device A should have screen shot move back animation before screen back to video

Actual result:
Just back to video without move back animation

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Greg, would you take this?

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

Yes, but not now. It may take some time to survey the reason, because I suspect that something broken under the Gaia, so the event didn't send normally.
I've discussed this with QA.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to Greg, we should already have the UI code ready. We need to verfiy shrinking_ui.js have indeed received the "shrinking-rejected" event in this case.

Siddartha, could you check the NFC manager code on this? thanks.

Comment 4

[psiddh]

Does this issue get reproduced after multiple attempts? Because, I tried to reproduce the issue with two NFC phones (latest FxOS builds), I was able to reproduce a issue (Not sure if its the same one reported) only after attempting multiple times. The shrinking UI does not move back after 5 -6 attempts on Video app. 

Upon further analyzing by adding logs, 

1) nfc_manager does send 'shrinking-stop' @ http://lxr.mozilla.org/gaia/source/apps/system/js/nfc_manager.js#388

2) Shrinking UI's handle event gets called - http://lxr.mozilla.org/gaia/source/apps/system/js/shrinking_ui.js#125

3) Shrinking UI attempts to stop the tilted UI 
http://lxr.mozilla.org/gaia/source/apps/system/js/shrinking_ui.js#242

4) However 'this._state()' returns 'false' @ Ln http://lxr.mozilla.org/gaia/source/apps/system/js/shrinking_ui.js#244 and the control returns from the stop() function. As a consequence of this, 'shrinking effect' is not restored (tilted back) and therefore this issue.

*Again, I am not sure if this is the same bug that is reported, if not this should be another bug to be raised and fixed.

Also please note that there is also a race condition issue around shrinking UI (in Nfc stack). This is being addressed in the bug : https://bugzilla.mozilla.org/show_bug.cgi?id=959059

Comment 5

[Ken Chang[:kenkai]]

Greg, any comment?

Comment 6

[Ken Chang[:kenkai]]

This bug blocks NFC 2.0 release.

Comment 7

[Garner Lee]

In Bug 961687, The send does not happen unless the "confirm" slide animation is complete, but I was only looking at sendNDEF(), not sendFile(blob). However, I think it's very similar, and it may be a relevant. Please retest when it lands.

Comment 8

[Ken Chang[:kenkai]]

(In reply to Garner Lee from comment #7)
> In Bug 961687, The send does not happen unless the "confirm" slide animation
> is complete, but I was only looking at sendNDEF(), not sendFile(blob).
> However, I think it's very similar, and it may be a relevant. Please retest
> when it lands.
Hi, please retest it. Thanks.

Comment 9

[Alison Shiue]

Attachment: share_URL_fail.mp4

I verified on 
Gaia      4f352142a54f7f7ae2c460aad9049eda4b0edb14
Gecko     https://hg.mozilla.org/mozilla-central/rev/9ac3e2dd0898
BuildID   20140508160203
Version   32.0a1

Please refer this video that still no move back animation when share URL fail.

Comment 10

[Garner Lee]

Hi Greg,

I think the reproduction steps changed between the bug open, and the switch to URL sharing changed, but it's the same bug.

Right now, the animation doesn't chain, because stop does a hard stop on all animations (_cleanEffects). Can you take a look? If it stops, it has to let the tilt animation to be added, which means shrinking-start has to transition from an intermediate "stopping" state, instead of flipping back instantly.

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

I remember that I mentioned once...Shrinking UI would do the rejection animation if NFC manager or other components send an event called 'shrinking-rejected', it would perform the animation to revert the session back. This is because the Shrinking UI of course should not judge if the sharing is successful or not. However, I grep the whole System app, no one would send the event. So this may be the first thing we should solved, unless the event would already be fired but it would be handled incorrectly.

Comment 12

[Kamil Leszczuk [:kamituel]]

Greg, I've looked into how NfcManager handles NFC events and you're right, it doesn't dispatch the 'shrinking-rejected' event. 

Me and Krzysiek have discussed it, and we believe it should be called as a result of 'nfc-manager-tech-lost'. Now, 'shrinking-stop' is dispatched instead [1].

However, even if we replace it with 'shrinking-rejected', it wouldn't behave as expected, right? I mean the screenshot would fly back in, but it'll still be tilted, right? So we'd need to either send 'shrinking-stop' shortly after that, or have 'shrinking-rejected' call .stop() once animation finishes.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L412

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

I think it should be modified the handler of 'shrinking-rejected' in shrinking_ui.js, to make it tilt back:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/shrinking_ui.js#L294

This was missed at the first implementation.

Comment 14

[[e-mail]]

Attachment: pull request redirect to github

* call 'shrinking-rejected' in 'nfc-manager-tech-lost'
* have 'shrinking-rejected' call .stop() once animation finishes.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8424651 [details] [review]
pull request redirect to github

Test for nfc_manager and shrinking_ui is wanted.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8424651 [details] [review]
pull request redirect to github

With tests it looks good.

Comment 17

[Kamil Leszczuk [:kamituel]]

Damn, I forgot to assign bug to myself ;)

Comment 18

[[e-mail]]

Comment on attachment 8424651 [details] [review]
pull request redirect to github

test added and travis passed, please kindly review it again

Comment 19

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/5a0c617fcb004e1066a2f0326ab86dc204fc11d2

thanks

Comment 20

[Alison Shiue]

Verified on
Gaia      c462d9183d294a2d8ecc472f593ea8cfa15bc9de
Gecko     https://hg.mozilla.org/mozilla-central/rev/9d8d16695f6a
BuildID   20140520160203
Version   32.0a1