1071510 - [NFC] app screenshot disappear on shrinking UI when share URL/images/videos/contacts

Status: VERIFIED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Alison Shiue]

Attachment: Shrinking_UI_issue.png

KK build on tinderbox
Gaia-Rev        3742913e11f69e789dcb0aa0dedf2e5572da0129
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/df42b05782aa
Build-ID        20140922185144
Version         34.0a2

STR:
1. Share URL/images/videos/contacts via NFC
2. Check shrinking UI

Expect result:
No problem

Actual result:
app screenshot disappear on shrinking UI

Comment 1

[Alison Shiue]

[Blocking Requested - why for this release]:
Obvious UI error

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

This is caused by Bug 1044125 which among other things removed showing screenshot overlay from AppWindow.setVisible method [1]. ShrinkingUI still uses the old version [2].

Alive do we need to show screenshot instead of the app view when doing the tilt for NFC share? I've done a quick test, if I remove the line at [2] the app still stays visible while tilted. This has some advantages:
- we don't trigger the unnecessary visibilitychange event (app is still in the foreground but titled)
- we don't see the screen flashing white before hiding the app and showing screenshot -> better user experience 
- since the app is still visible, when sharing video, we don't stop it while it plays, this solves Bug 1038998
- this solves Bug 1066764    

I'm not sure about the performance impact if we won't be hiding the app. I don't have a low memory Flame to test this, seems like the vide playing while sharing case could be a potential problem, but still these are css transformations here. If we would go with this solution we would fix three bugs at once. What do you think? Is this acceptable?

[1] https://github.com/mozilla-b2g/gaia/blob/e1b809c158993a565a4bcd8987d86b7d2604d677/apps/system/js/app_window.js#L207
[2] https://github.com/mozilla-b2g/gaia/blob/9f871837cef6e6b9b8985e894cf35fca27f3eedf/apps/system/js/shrinking_ui.js#L213

Comment 3

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

Hmm, I think we should still send app to background at this timing (before shrinkingUI is moved into apps) because app needs to know visibility state to stop something.

What we could try to do here is let AppWindowManager or ShrinkingUI broadcast 'shrinking-start'/'shrinking-stop' event to appWindow, and then in appWindow's event handler, get-show/hide the screenshot correspondingly.

Hi aus, here's one another regression from bug 1044125. I am not sure what's your plan but let's fix it since this looks to me a blocker.

:tauzen if you wanna to take let me know.

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

I consulted Alive on IRC, and will try to fix this. If it get's too complicated I'll reach out for help.

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

Hi Alive, could you take a look at this: https://github.com/tauzen/gaia/commit/cba9b52280134f797b463346a271980d3bf362f2
Is this what you had in mind? This still needs some testing, but seems to solve this problem and also Bug 1066764.

Comment 6

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #5)
> Hi Alive, could you take a look at this:
> https://github.com/tauzen/gaia/commit/
> cba9b52280134f797b463346a271980d3bf362f2
> Is this what you had in mind? This still needs some testing, but seems to
> solve this problem and also Bug 1066764.

looks good! please add tests both for appWindow and shrinkingUI and request review.

Comment 8

[Aus Lacroix [:aus]]

:tauzen, looks like you have this one under control. :) If there's anything I can do to help though, please let me know.

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pull-request-1071510.txt

Added tests and comments, Alive could you review this now? Try build is still progressing.

Comment 10

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

Comment on attachment 8495116 [details] [review]
pull-request-1071510.txt

r=me with nit, thank you!

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

Thanks for the review! Fixed the nit and Gaia-try is green.

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8495116 [details] [review]
pull-request-1071510.txt

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Bug 1044125 
[User impact] if declined:
Poor user experience while doing NFC share - the screenshot disappears leaving a blank app frame. 
[Testing completed]:
Unit tests passing, on device testing complete.
[Risk to taking this patch] (and alternatives if risky):
Low - AppWindow changes specific only to NFC sharing scenario. 
[String changes made]:
None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/886765442d5b1073fa588fe61f8729766a47105b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/188eb969d8d0f246d85ef648f8981e700ac15b0a

Comment 15

[Alison Shiue]

Verified on
[2.1]
Gaia-Rev        08be48c71d0b4999cedee89fe81de1a03c66436f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/6e7e0a39f73b
Build-ID        20140930160205
Version         34.0a2

[2.2]
Gaia-Rev        77ef35f5429bc3dfe9ca192b9aacc3c0bf8857de
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2ae57957e4bb
Build-ID        20140930160207
Version         35.0a1

Comment 16

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pull-request-1071510_2.0.txt

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Uplift of Bug 1062136 to 2.0
[User impact] if declined:
Shrinking UI disappears, broken NFC Sharing function, additional bug 1080486(should be marked as duplicate?) was raised for this.
[Testing completed]:
Tested on Flame, local unit test passing, gaia-try (progressing): https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=39383b882f03
[Risk to taking this patch] (and alternatives if risky):
Low, nfc sharing specific changes in App window.
[String changes made]:
None

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1062136#c47 and https://bugzilla.mozilla.org/show_bug.cgi?id=1062136#c51 for more details.

Comment 18

[Alison Shiue]

Hi Bhavana, please help this approval-gaia-v2.0 request, thank you.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/c6c6116ca225c2c934220ae6867e5a3256d65e00

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/c6c6116ca225c2c934220ae6867e5a3256d65e00

Comment 22

[Alison Shiue]

Verified on
Gaia-Rev        c6c6116ca225c2c934220ae6867e5a3256d65e00
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/24a2aa6bf1c4
Build-ID        20141015160202
Version         32.0