Alison Shiue Reporter
	Description • 11 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 1 • 11 years ago

Greg, would you take this?

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 2 • 11 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 3 • 11 years ago

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.

	psiddh
	Comment 4 • 11 years ago

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

	Ken Chang[:kenkai]
	Comment 5 • 11 years ago

Greg, any comment?

	Ken Chang[:kenkai]
	Comment 6 • 11 years ago

This bug blocks NFC 2.0 release.

	Garner Lee
	Comment 7 • 11 years ago

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.

	Ken Chang[:kenkai]
	Comment 8 • 11 years ago

(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.

	Alison Shiue Reporter
	Comment 9 • 11 years ago

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.

	Garner Lee
	Comment 10 • 11 years ago

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.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 11 • 11 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 12 • 11 years ago

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

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 13 • 11 years ago

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.

	gasolin@mozilla.com Assignee
	Comment 14 • 10 years ago

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

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 15 • 10 years ago

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

Test for nfc_manager and shrinking_ui is wanted.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 16 • 10 years ago

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

With tests it looks good.

	Kamil Leszczuk [:kamituel]
	Comment 17 • 10 years ago

Damn, I forgot to assign bug to myself ;)

	gasolin@mozilla.com Assignee
	Comment 18 • 10 years ago

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

test added and travis passed, please kindly review it again

	gasolin@mozilla.com Assignee
	Comment 19 • 10 years ago

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

thanks

	Alison Shiue Reporter
	Comment 20 • 10 years ago

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