1117633 - System app should call setNFCFocusApp when App is in foreground

Status: RESOLVED FIXED

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

Description

[Dimi Lee [:dimi]]

As discuss in Bug 1105666, we will use BrowserAPI to set current focus app, so NFC module could notify correct app when receive nfc event.

This bug should implement in system app and call setNFCFocusApp app in suitable timing

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Hi Alive and Greg, I am wondering if you can help to fix this bug after gecko bug is fixed?

Comment 2

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

On it

Comment 3

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

Attachment: WIP

Hi, please test if this fits the gecko change. Also I realized maybe we need to unfocus at certain timing (for example, while switching apps) but I don't see how.

Comment 4

[Dimi Lee [:dimi]]

Hi Alive,
When implement this API i am thinking system app will always set current foreground app as NFC focus app(no matter if it will use NFC function or not).

For those APPs do not handle NFC event and is set NFC focus, NFC module will know that because those apps won't register event callback to NFC callback, so in this case NFC will not sent callback to the app and will sent to system app instead(bug 1082443).

Does it make sense to you? Or do you still need clear API if there is any case there is "NO" app should be set NFC focus ?

Comment 5

[Dimi Lee [:dimi]]

sorry remove need info incorrectly

Comment 6

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #4)
> Hi Alive,
> When implement this API i am thinking system app will always set current
> foreground app as NFC focus app(no matter if it will use NFC function or
> not).
> 
> For those APPs do not handle NFC event and is set NFC focus, NFC module will
> know that because those apps won't register event callback to NFC callback,
> so in this case NFC will not sent callback to the app and will sent to
> system app instead(bug 1082443).
> 
> Does it make sense to you? Or do you still need clear API if there is any
> case there is "NO" app should be set NFC focus ?

If the nfc-manager-tech-discovered event is sent to system app before nfc module in gecko triggers nfc onpeerready or onpeerfound callback than that is fine. i.e., system app should have the ability to determine if now is the proper time to trigger callback even when the app is set as nfc foreground.

Comment 7

[Dimi Lee [:dimi]]

As discussed with alive, GAIA will still need a way to clear current focus app, ex. When APP is in transition mode, GAIA could not determine which app is in foreground in this case.
So i will add a boolean parameter in setNFCFocusApp API to let GAIA could clear current focus app.

Comment 8

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

Comment on attachment 8543936 [details] [review]
WIP

Patch updated: use boolean in setNFCFocusApp to cancel the focus on need.

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8543936 [details] [review]
WIP

Hi Alive,
This WIP works good, thanks.
However there is one case I am not sure if it works as expect:
When doing App transition, ex, contact app -> gallery app
1.In the middle of transition => clear focus of contact app
2.Switch to galley app => set focus to contact app, then clear focus of contact app and will call set focus to galley at last.

Is step 2 as expect ?

Comment 10

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

Comment on attachment 8543936 [details] [review]
WIP

No test yet, want to hear from you before writing tests.

The story is we need to tell nfc module who is the top most window, and maybe there would be other modules need this information later. I created an abort event to indicate the sheet transition is not proceeded. I think the "update titlebar theme" part in statusbar could utilize the general 'windowopened' event instead of knowing the architecture the of 2 dimension windows.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8543936 [details] [review]
WIP

I feel like we can do something simpler.

The topMostWindow ends up being cached in multiple places and some events are almost duplicates.
I don't want to miss any cases but I don't think there's any performance-related reasons to add this complexity.
For example I think we can afford to call `setNFCFocusApp` on every |sheets-gesture-end| (removing the need for the |sheets-gesture-abord| event) and I think we can Service.query('getTopMostWindow') every time too.

But my main question is: can we "just" have AppWindow#_setActive also toggle `setNFCFocusApp` and make the edge gestures properly call `_setActive`? (the current sheet should be setActive(false) during the gesture)

I think it would cover all cases and make for a *much* simpler patch.

PS: the "App" in |setNFCFocusApp| seems redundant since it's exposed on an app iframe and takes a boolean (and not an app).

Comment 12

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

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Comment on attachment 8543936 [details] [review]
> WIP
> 
> I feel like we can do something simpler.
> 
> The topMostWindow ends up being cached in multiple places and some events
> are almost duplicates.
> I don't want to miss any cases but I don't think there's any
> performance-related reasons to add this complexity.
> For example I think we can afford to call `setNFCFocusApp` on every
> |sheets-gesture-end| (removing the need for the |sheets-gesture-abord|
> event) and I think we can Service.query('getTopMostWindow') every time too.

This sounds work, lemme try it.

> 
> But my main question is: can we "just" have AppWindow#_setActive also toggle
> `setNFCFocusApp` and make the edge gestures properly call `_setActive`? (the
> current sheet should be setActive(false) during the gesture)

setActive won't know who is on top of the window. Maybe the utilityTray, maybe the lockscreen....
And we don't want to set NFC active then.

> 
> I think it would cover all cases and make for a *much* simpler patch.
> 
> PS: the "App" in |setNFCFocusApp| seems redundant since it's exposed on an
> app iframe and takes a boolean (and not an app).

It's also my question, but I will leave it to platform guy.

Comment 13

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

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Comment on attachment 8543936 [details] [review]
> WIP
> 
> I feel like we can do something simpler.
> 
> The topMostWindow ends up being cached in multiple places and some events
> are almost duplicates.
> I don't want to miss any cases but I don't think there's any
> performance-related reasons to add this complexity.
> For example I think we can afford to call `setNFCFocusApp` on every
> |sheets-gesture-end| (removing the need for the |sheets-gesture-abord|
> event) and I think we can Service.query('getTopMostWindow') every time too.
> 
> But my main question is: can we "just" have AppWindow#_setActive also toggle
> `setNFCFocusApp` and make the edge gestures properly call `_setActive`? (the
> current sheet should be setActive(false) during the gesture)
> 
> I think it would cover all cases and make for a *much* simpler patch.
> 
> PS: the "App" in |setNFCFocusApp| seems redundant since it's exposed on an
> app iframe and takes a boolean (and not an app).

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> (In reply to Etienne Segonzac (:etienne) from comment #11)
> > Comment on attachment 8543936 [details] [review]
> > WIP
> > 
> > I feel like we can do something simpler.
> > 
> > The topMostWindow ends up being cached in multiple places and some events
> > are almost duplicates.
> > I don't want to miss any cases but I don't think there's any
> > performance-related reasons to add this complexity.
> > For example I think we can afford to call `setNFCFocusApp` on every
> > |sheets-gesture-end| (removing the need for the |sheets-gesture-abord|
> > event) and I think we can Service.query('getTopMostWindow') every time too.
> 
> This sounds work, lemme try it.
> 

The problem of using end event is comment 9. We will see going-out app becomes false then true then false.
I dare not to say this is not a problem, but I think we will need to know the sheet transition is canceled or succeeded later in other cases. That's why I introduce abort.

Comment 14

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> (In reply to Etienne Segonzac (:etienne) from comment #11)
> > Comment on attachment 8543936 [details] [review]
> > WIP
> > 
> > I feel like we can do something simpler.
> > 
> > The topMostWindow ends up being cached in multiple places and some events
> > are almost duplicates.
> > I don't want to miss any cases but I don't think there's any
> > performance-related reasons to add this complexity.
> > For example I think we can afford to call `setNFCFocusApp` on every
> > |sheets-gesture-end| (removing the need for the |sheets-gesture-abord|
> > event) and I think we can Service.query('getTopMostWindow') every time too.
> 
> This sounds work, lemme try it.
> 
> > 
> > But my main question is: can we "just" have AppWindow#_setActive also toggle
> > `setNFCFocusApp` and make the edge gestures properly call `_setActive`? (the
> > current sheet should be setActive(false) during the gesture)
> 
> setActive won't know who is on top of the window. Maybe the utilityTray,
> maybe the lockscreen....
> And we don't want to set NFC active then.

Also even we don't care about lockscreen/utilityTray case, we should not allow the active app able to proceed the nfc event while swipe navigation or during opening transition. So I will say setNfcActive when setActive is something we don't want. It is to bind NfcActive with visibility which just the reason why we have a new API instead of just using the result setVisible() in gecko.

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> > setActive won't know who is on top of the window. Maybe the utilityTray,
> > maybe the lockscreen....
> > And we don't want to set NFC active then.

FWIW we probably want to setActive(false) the app in those cases too (and it's already the case for the lockscreen).

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> Also even we don't care about lockscreen/utilityTray case, we should not
> allow the active app able to proceed the nfc event while swipe navigation or
> during opening transition. So I will say setNfcActive when setActive is
> something we don't want. It is to bind NfcActive with visibility which just
> the reason why we have a new API instead of just using the result
> setVisible() in gecko.

Just to be clear: my proposition was to add setActive calls in some cases other than the BrowserMixin#_setVisible method (like you said, if we only call setActive when we call setVisible we don't need the setActive API...).

And it looked like those cases matched well with the cases where we want to setNFCFocus(false).

I'm perfectly fine with just simplifying the WIP patch bit but, whatever solution we end up with, having a clear view of cases where setActive and setNFCFocus don't match will help.

Comment 16

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

Okay I think I got confused with setActive, setVisible and setNfcActive here. I will look into who is using setActive now.

Comment 17

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

(In reply to Etienne Segonzac (:etienne) from comment #15)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> > > setActive won't know who is on top of the window. Maybe the utilityTray,
> > > maybe the lockscreen....
> > > And we don't want to set NFC active then.
> 
> FWIW we probably want to setActive(false) the app in those cases too (and
> it's already the case for the lockscreen).
> 
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> > Also even we don't care about lockscreen/utilityTray case, we should not
> > allow the active app able to proceed the nfc event while swipe navigation or
> > during opening transition. So I will say setNfcActive when setActive is
> > something we don't want. It is to bind NfcActive with visibility which just
> > the reason why we have a new API instead of just using the result
> > setVisible() in gecko.
> 
> Just to be clear: my proposition was to add setActive calls in some cases
> other than the BrowserMixin#_setVisible method (like you said, if we only
> call setActive when we call setVisible we don't need the setActive API...).
> 
> And it looked like those cases matched well with the cases where we want to
> setNFCFocus(false).
> 

Unfortunately "only doing this" does not work. We are only doing _setActive() on setVisible(), but for this case:
App A->Inline Activity B->Inline Activity C
*When opening B or C, setVisible(true) for B/C is not called
*When C is closed, nobody will setVisible(true) for B again because of https://bugzilla.mozilla.org/show_bug.cgi?id=892371

Comment 18

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

Comment on attachment 8543936 [details] [review]
WIP

Changed some logic, need your feedback again, thanks.

Comment 19

[Dimi Lee [:dimi]]

Comment on attachment 8543936 [details] [review]
WIP

Hi alive,
It seems when enter transition mode it will not clear app's focus

- When launch app from home screen, it will call
1.set focus of launched app to true
2.set focus of previous app to false
3.set focus of launched app to true (second time)
4.set focus of launched app to true (third time)

- Enter transition mode
1.Do nothing

- In Transition mode then change to the other application
1.set focus of new app to true
1.set focus of new app to true (second time)
3.set focus of previous app to false

Comment 20

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

Comment on attachment 8543936 [details] [review]
WIP

So it appears we cannot "just" do call setNFCActive in _setActive because appWindow does not know that it is the top most window or not. See this case: have sim pin locked and open message app, it should be nfc active but in this patch we will set it to.

And I don't see somewhere in sheetTransition/stackManager to know the timing to call _setActive(true). See this case: open message app and only one app, do swipe navigation(now is _setActive(false)) and release the finger to let the app going back - nobody is doing _setActive(true) for the appWindow and tried doing that in StackManager.commit() does not work.

We need to talk if you still think all this is enough to implement :/

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8543936 [details] [review]
WIP

Made some comments on github but I think we have our compromise :)
Lots of cases though, let's make sure the tests document this properly.

Comment 22

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

Comment on attachment 8543936 [details] [review]
WIP

The more I did the more I think we should consider to do 'focus'/'setNFCFocus'/setVisibleForScreenReader together in setHierarchy but not in _setActive(). But for now let's not change too much for v2.2

* I am still using topmostwindowchanged event in AWM because I think it could be reused in statusbar.
* didntMove logic is changed in stackManager. It is because when there is only one app moving, queueBroadcast will not be called by anyone so stackManager have no _appIn/appOut information.

I am still confusing about the relationship between StackManager/EdgeSwipeDetector/SheetTransition. Some information is only kept in one of them, some are shared. It's not at high priority but let's discuss how to improve the module dependency here someday.

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8543936 [details] [review]
WIP

r=me with the test comments addressed

Comment 24

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

Comment on attachment 8543936 [details] [review]
WIP

Let's make sure Dimi confirm this works before merging.

Comment 25

[Dimi Lee [:dimi]]

Comment on attachment 8543936 [details] [review]
WIP

Hi Alive,
This patch looks good.
1. launch app only call setNFCFocus once
2. close app will clear focus
3. enter transition mode will clear focus
4. switch app after transition mode will also set focus to correct app.

Is there any other case you would like me to test ?

Comment 26

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #25)
> Comment on attachment 8543936 [details] [review]
> WIP
> 
> Hi Alive,
> This patch looks good.
> 1. launch app only call setNFCFocus once

Actually we want gecko to absorb this: the second setNFCFocus(true) to the first setNFCFocus(true) should be no-op.
Could you handle this in gecko? We workaround it right now.

> 2. close app will clear focus
> 3. enter transition mode will clear focus
> 4. switch app after transition mode will also set focus to correct app.
> 
> Is there any other case you would like me to test ?

Comment 27

[Dimi Lee [:dimi]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #25)
> > Comment on attachment 8543936 [details] [review]
> > WIP
> > 
> > Hi Alive,
> > This patch looks good.
> > 1. launch app only call setNFCFocus once
> 
> Actually we want gecko to absorb this: the second setNFCFocus(true) to the
> first setNFCFocus(true) should be no-op.
> Could you handle this in gecko? We workaround it right now.
> 
> > 2. close app will clear focus
> > 3. enter transition mode will clear focus
> > 4. switch app after transition mode will also set focus to correct app.
> > 
> > Is there any other case you would like me to test ?

Yes, we could handle this in gecko

Comment 28

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

I am going to land this and file followup if needed.

Comment 29

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

https://github.com/mozilla-b2g/gaia/commit/a47ec54a7040d6f689c3eabd2f23bb510c78c0cd

Comment 30

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

Feature-b2g:2.2+ because it's blocking bug 1082440.
Alive, could you request uplift to 2.2 ?

Comment 31

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

Comment on attachment 8543936 [details] [review]
WIP

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Blocking bug 1082440
[User impact] if declined:
Third party app will get nfc notification even it is in background
[Testing completed]: Y
[Risk to taking this patch] (and alternatives if risky): Regressions are found and fixed in bug 1125023 so we need to uplift it at the same time
[String changes made]: NaN

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 8543936 [details] [review]
WIP

PLease make sure to uplift patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1124346 at the same time to avoid any fallouts.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/b51d38a64cd28816906221dea088ba3a4d9f5034