1101029 - Implement GlobalOverlayWindow

Status: RESOLVED FIXED

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

Description

[Romain Testard [:RT]]

The delivery of bug 1062180 (ability for the helper to laser point at the helpee's screen) requires system app work to allow BuddyUp to open an iframe overlayed on top of font-most app.
This bug covers that piece of work on the system app.

Comment 1

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

Where does this iframe come from?

Comment 2

[Jet Villegas (inactive)]

Is there already a design for how this iframe is invoked, sized, and populated? ie. API, security policy, event handling, etc.?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand what the Gecko bug is here. Have you got a testcase where Gecko differs from expected results?

As far as I know, you should be able to add an element to the system app that draws over the top of the foreground app with transparency. That element may or may not be an IFRAME. The keyboard app already does this.

Comment 4

[Romain Testard [:RT]]

Are you saying that this can be done today without platform work?
For info the use we're trying to deliver is detailed on 1062180.
If there is no platform work then great.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There isn't much detail in bug 1062180. AFAIK you don't need anything new from the platform here. See comment #3.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

We are looking for doing this via attention screen, Anthony and Vivien are discussing about it, so hopefully no platform work needed here.

Comment 8

[Anthony Ricaud (:rik)]

I've written down the plan on https://wiki.mozilla.org/BuddyUp/Phase2#Pointing_on_the_screen. Hopefully, that explains a bit more the separation of tasks. This includes a tiny bit of work on the platform side for the new type of window as far as I understand.

There is also a new use case for Contact quick view, see bug 1105695 and [1] [2].

[1] https://wiki.mozilla.org/Gaia/Contacts/Bookmarks#UX_Proposal
[2] https://wiki.mozilla.org/Gaia/Contacts/Bookmarks#Overlay_window

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

I'll be working on this in the next sprint (2.2 S2). If anyone can get to this before feel free to steal it from me.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: overlay.zip

The app I used to see if the feature is working.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: gaia.overlays.patch

The Gaia bits.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: gecko.overlays.patch

The Gecko bits.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For what it worth: I'm not working on this. But I just wanted to post this wip somewhere that does what you guys expect for buddy up as far as I have understood the need.

The gecko patch contains 2 things:
 - One is a new permission, and you want to discuss that with Jonas and Security folks.
 - The second part is just to make sure mozpasspointerevents works correctly for iframes created by window.open. One of those days (probably soon) it all this UpdateHitRegion code will be obsoleted by a better hit target management for APZ.

Comment 14

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

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #6)
> We are looking for doing this via attention screen, Anthony and Vivien are
> discussing about it, so hopefully no platform work needed here.

Please don't overload attention screens with new use cases. We want to deprecate that if possible.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko patch: Allow setting mozpasspointerevents dynamically

Robert, this patch allows to set the mozpasspointerevents iframe attribute dynamically. I tried updating pass pointer events from the nsFrameLoader mutation observer instead of the nsSubDocumentFrame one, but it isn't being fired for some reason when I set the iframe attribute. Don't know why...

I have to add tests for this, but I would love to hear your feedback about the current WIP patch.

Thanks for your help!

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27065

This is the WIP patch based on Vivien's work. It allows apps to create overlay windows via "window.open(..., 'overlay')". It requires apps to have the "overlay" permission granted only to privileged apps. The overlay window is chromeless for now (i.e. we don't show the rocketbar search box).

Since the contact bookmarks feature has been put on hold for now, the overlay window implemented in this patch is fully oriented to the BuddyUp use case. 

Some things that I believe that need to change if we want to use this overlay windows for other use cases:

1. The overlay window is the top most element.
----------------------------------------------
This is like this because BuddyUp needs it this way, but we probably don't want this window to be the top most window for other cases like the contact details overlay, where we want it to be on top of other apps, but not on top of the status bar, system dialogs or other "chrome UI" elements.

2. Home button clicks does not affect the overlay window
---------------------------------------------------------
The BuddyUp folks can correct me if I am wrong, but the way I understood the requirements, we need to ignore home clicks for the BuddyUp use case. If we want to give global support, we need to keep the overlay window opened even if the user hits the home button to switch between apps.

That should not be the behavior for other use cases. For example, if we open the contact details overlay, we want it to be closed if the user clicks on the home button. Otherwise, the UX will be pretty weird having the contact details floating on top of different apps.

Given 1. and 2. and considering also its security implications I believe we need to differentiate two use cases for this overlay windows.

A. Global overlay
-----------------
This is the BuddyUp use case. We need the overlay window to be top most element and we need to ignore home clicks.

We can allow triggering this kind of overlay with the "overlay-global" window.open feature (to be bike-shed).

Having a global overlay is quite security sensitive. I believe we need to add some chrome UI here to indicate that the overlay window is global and active. Maybe something like a visible border or some grayed out text.

- We also need to add a close button so the user can dismiss the overlay window. We cannot depend on the app authors to implement their own close button, and so this needs to be part of the overlay window chrome UI.

B. Local overlay
----------------
This is the contact details use case. We need the overlay window on top of non-system apps and elements and we need it to be closed when the user hits the home button.

We can allow triggering this kind of overlay with the "overlay" window.open feature (also to be bike-shed).

I think we don't need any kind of chrome UI here.

I'll be working on this modifications and I would love to here any feedback about this. Thanks!

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Test app

You can use this app to test the attached patches.

I uploaded a video of this working at https://vimeo.com/115642086

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #14)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #6)
> > We are looking for doing this via attention screen, Anthony and Vivien are
> > discussing about it, so hopefully no platform work needed here.
> 
> Please don't overload attention screens with new use cases. We want to
> deprecate that if possible.

That is not the current approach, Tim. We are creating a new type of AppWindow specific for this.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8542563 [details] [diff] [review]
Gecko patch: Allow setting mozpasspointerevents dynamically

Review of attachment 8542563 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know why the nsFrameLoader AttributeChanged doesn't work; it probably should. But this is fine.

::: dom/ipc/TabChild.cpp
@@ +3049,5 @@
> +    if (mUpdateHitRegion) {
> +      nsCOMPtr<nsIDocument> document(GetDocument());
> +      nsCOMPtr<nsIPresShell> presShell = document->GetShell();
> +      nsRefPtr<nsPresContext> presContext = presShell->GetPresContext();
> +      presContext->InvalidatePaintedLayers();

I would add null checks here, especially after GetShell.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> A. Global overlay
> -----------------
> This is the BuddyUp use case. We need the overlay window to be top most
> element and we need to ignore home clicks.
> 
> We can allow triggering this kind of overlay with the "overlay-global"
> window.open feature (to be bike-shed).
> 
> Having a global overlay is quite security sensitive. I believe we need to
> add some chrome UI here to indicate that the overlay window is global and
> active. Maybe something like a visible border or some grayed out text.
> 
> - We also need to add a close button so the user can dismiss the overlay
> window. We cannot depend on the app authors to implement their own close
> button, and so this needs to be part of the overlay window chrome UI.

I'll leave it to the security team to decide if we need a visible border or some such. Personally I'd actually be ok with not having that. I think it adds relatively little security as it's hard for users to fully realize the implications of that there might be an overlay. I.e. it's hard for users to understand just how clever and dangerous a click-jacking attack can be.

Hence I think it's our responsibility to ensure that only "good guys" can create this type of overlay. Which would mean that a visible border isn't really needed.

But again, I'll leave it to the security team to make the call here.

But I could see the use of having some on-screen UI for dismissing the overlay. Mainly for convenience.

I do think that, as you point out, the type of overlay here is quite buddy-up specific. So we should probably use a name that's more specific than "global-overlay". Maybe something like "global-clickthrough-overlay"


Aside: It might be cool if we left it up to the buddyup app to decide what parts of the overlay should be "click-through" and what part should be clickable. This way you could create some pretty powerful tutorial UIs for example. So only the <iframe> itself would be click-through. But anything rendered would by default not be, unless it use pointer-events:none.

But we should only do that if it's easy. We don't have any specific use cases right now so happy to leave it out.

> B. Local overlay
> ----------------
> This is the contact details use case. We need the overlay window on top of
> non-system apps and elements and we need it to be closed when the user hits
> the home button.
> 
> We can allow triggering this kind of overlay with the "overlay" window.open
> feature (also to be bike-shed).
> 
> I think we don't need any kind of chrome UI here.
> 
> I'll be working on this modifications and I would love to here any feedback
> about this. Thanks!

I think it'd be very cool if we were able to come up with a type of overlay that is safe enough that we can expose it to web content here.

Other use cases is for things like "share" activity handlers which wants to render just a quick UI on top of the current app. I.e. rather than render the UI for transitioning to a new app, the "share" handler could just render an overlay on top of the current app with just a quick "success" UI.

I think this could be safe as long as we make sure that no clicks can go through to that app. This way clickjacking can't be done. We could also "gray out" the app behind the overlay to make the UI more clear.

This is a feature that was requested by the Pocket app developers since they can render a similar UI on Android. Though on Android it requires additional permissions which would be good if we could avoid.

But yeah, in this type of overlay, clicking the home button should close the overlay. And it should be possible to close the overlay as easily as you can switch between apps. So bringing up the app switcher should likely also close the overlay.

Anyhow, it might be good to do this overlay type in a separate bug?

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Thanks for your feedback!

What Jonas says makes total sense to me, so I will stick to that unless anyone disagrees.

I'll leave this bug to implement the global overlay that allows passing clicks to the overlaid content and with a minimal chrome UI that allows the user to close it as it ignores the home button clicks. I'll need UX/UI design for this. Gregor, Candice, I am not sure who's in charge of UX/UI design for system front-end features. Could you please needinfo the appropriate person? Thanks!

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko patch: Allow setting mozpasspointerevents dynamically

Now with tests. Thanks Robert!

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko patch: Allow setting mozpasspointerevents dynamically

Actually, this is the right patch.

r? Jonas for the 'global-clickthrough-overlay' permission addition. This is been tested on the Gaia patch with marionette tests.

Thanks!

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8542571 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27065

Alive, this patch introduces the new GlobalOverlayWindow. Some notes about this kind of window:

- They are opened via window.open passing the 'global-clickthrough-overlay' feature.
- The 'global-clickthrough-overlay' permission is required.
- These windows are displayed on top of any other UI element, including system UI bits.
- We only allow one global overlay window at a time.
- We display this windows with transparent background.
- The only chrome UI for these windows is a close button that is currently displayed on the top left corner and it is pending UI design (to be done in a follow up bug).

I added a functional test but unfortunately it can't be triggered in tbpl until we update the b2g desktop that we use to run these tests there. I filed bug 1117125 for that.

Thanks in advance for your feedback.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8543263 [details] [diff] [review]
Gecko patch: Allow setting mozpasspointerevents dynamically

Review of attachment 8543263 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +3045,5 @@
>      mUpdateHitRegion = aEnabled;
> +
> +    // We need to trigger a repaint of the child frame to ensure that it
> +    // recomputes and sends its region.
> +    if (mUpdateHitRegion) {

Make this an early exit: if (!mUpdateHitRegion) return true;

@@ +3051,5 @@
> +      NS_ENSURE_TRUE(document, false);
> +      nsCOMPtr<nsIPresShell> presShell = document->GetShell();
> +      NS_ENSURE_TRUE(presShell, false);
> +      nsRefPtr<nsPresContext> presContext = presShell->GetPresContext();
> +      NS_ENSURE_TRUE(presContext, false);

I think we should return true in these cases.

Comment 26

[Gregor Wagner [:gwagner]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #21)
> Thanks for your feedback!
> 
> What Jonas says makes total sense to me, so I will stick to that unless
> anyone disagrees.
> 
> I'll leave this bug to implement the global overlay that allows passing
> clicks to the overlaid content and with a minimal chrome UI that allows the
> user to close it as it ignores the home button clicks. I'll need UX/UI
> design for this. Gregor, Candice, I am not sure who's in charge of UX/UI
> design for system front-end features. Could you please needinfo the
> appropriate person? Thanks!

There is already a NI for Rob. He will forward to the right people.

Comment 27

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

Comment on attachment 8542571 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27065

I have several nits, and noticed GOWM has not test, please have one in followup.

Comment 28

[Candice Serran (:cserran)]

removing ni since rob and jacqueline are already on here for UX support

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8542571 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27065

This needs to be landed by next Monday. Fabrice can you review the permission addition, please?

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8543263 [details] [diff] [review]
Gecko patch: Allow setting mozpasspointerevents dynamically

Review of attachment 8543263 [details] [diff] [review]:
-----------------------------------------------------------------

The added permission looks fine to me. I don't know what the process is for informing the marketplace about this new permission so that they can do the appropriate checks while they are reviewing apps.

Adding Paul for feedback so that he can help out once he gets back.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8543263 [details] [diff] [review]
Gecko patch: Allow setting mozpasspointerevents dynamically

Review of attachment 8543263 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the PermissionsTable.jsm change.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Thanks!

https://hg.mozilla.org/integration/b2g-inbound/rev/52f135a2f3ac
https://github.com/mozilla-b2g/gaia/commit/0549a24261b03ab27cc525d9ad3020a0733b3ab5

I filed bug 1119708 for the visual design and moved the ni? there.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52f135a2f3ac

Comment 34

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jonas Sicking (:sicking) from comment #30)
> Comment on attachment 8543263 [details] [diff] [review]
> Gecko patch: Allow setting mozpasspointerevents dynamically
> 
> Review of attachment 8543263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The added permission looks fine to me. I don't know what the process is for
> informing the marketplace about this new permission so that they can do the
> appropriate checks while they are reviewing apps.
> 
> Adding Paul for feedback so that he can help out once he gets back.

My only concern is that this a 'privileged' permission, not a certified one. It sounds more certified to me, but I'm guessing we made it certified since buddy up is privileged :/ If we supported installation of certified apps from marketplace, would that remove this requirement? Just a thought.

But as it stands, I agree with comment 20 there isn't much we can do to educate the user here, but it leaves a lot of the onus on marketplace to determine which apps are "good" apps. That sounds like a difficult ask to me. I would expect many apps to want to use this to show pop-ups etc - we probably need to come with a policy of acceptable use for this API at a minimum. What type of apps do we expect to be allowed to use this API? What if I want to use this API to show ads, is that allowed? How will we make the call on what is acceptable usage or not? Adding Andrew from marketplace for input.

Comment 35

[Andrew Williamson [:eviljeff]]

(In reply to Paul Theriault [:pauljt] from comment #34)
> (In reply to Jonas Sicking (:sicking) from comment #30)
> > Comment on attachment 8543263 [details] [diff] [review]
> > Gecko patch: Allow setting mozpasspointerevents dynamically
> > 
> > Review of attachment 8543263 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The added permission looks fine to me. I don't know what the process is for
> > informing the marketplace about this new permission so that they can do the
> > appropriate checks while they are reviewing apps.
> > 
> > Adding Paul for feedback so that he can help out once he gets back.
> 
> My only concern is that this a 'privileged' permission, not a certified one.
> It sounds more certified to me, but I'm guessing we made it certified since
> buddy up is privileged :/ If we supported installation of certified apps
> from marketplace, would that remove this requirement? Just a thought.

I want avoid opening that particular can of worms if possible... and as Marketplace wouldn't be able to do an awful lot different in terms of reviewing/displaying/controlling with certified apps than privileged I'm not sure what the difference would be at that point between the two types.

> But as it stands, I agree with comment 20 there isn't much we can do to
> educate the user here, but it leaves a lot of the onus on marketplace to
> determine which apps are "good" apps. That sounds like a difficult ask to
> me. I would expect many apps to want to use this to show pop-ups etc - we
> probably need to come with a policy of acceptable use for this API at a
> minimum. What type of apps do we expect to be allowed to use this API? What
> if I want to use this API to show ads, is that allowed? How will we make the
> call on what is acceptable usage or not? Adding Andrew from marketplace for
> input.

what does the api enable (in summary)?  From quickly reading the bug I guess it lets the app display a transparent iframe fullscreen as an overlay.  Some questions:
* how would a user know the overlay is there/the app is running? (is there any chrome to indicate?)
* similarly, if there were multiple apps using this API what would happen - would they overlay over each other?  would the user be able to know that overlay A is from app A rather app B?
* how can a user close the overlay?  (I read above that home stops the app?  Or not - the global/local differentiation in #c16 wasn't clear to me.)
* does the overlay capture touch events for the whole screen? Or allow them to pass through?
* does the API grant any kind of priority in terms of RAM (i.e. what would happen in a low memory situation)

I guess we would allow ads on the UX if it was part of feature and was short lived (e.g. it pops up to perform a function and then the user dismisses it).  A persistent overlay, or an unsolicited ad, not so much.  (cc'ing adora too)

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Andrew Williamson [:eviljeff] from comment #35)
> what does the api enable (in summary)?  From quickly reading the bug I guess
> it lets the app display a transparent iframe fullscreen as an overlay.  Some
> questions:
> * how would a user know the overlay is there/the app is running? (is there
> any chrome to indicate?)

This is still pending UX design, but we are currently showing a close button on the top left corner of the screen.

> * similarly, if there were multiple apps using this API what would happen -
> would they overlay over each other?  would the user be able to know that
> overlay A is from app A rather app B?

Only one global overlay is allowed at a time, so if there is already one global overlay a request to open a new one will be rejected.

> * how can a user close the overlay?  (I read above that home stops the app? 
> Or not - the global/local differentiation in #c16 wasn't clear to me.)

The user can close the global overlay by clicking in the close button that we render along with the global overlay UI. These overlays ("global") are not affected by the home button, while "local" overlays will be (once they are implemented on bug 1116779).

> * does the overlay capture touch events for the whole screen? Or allow them
> to pass through?

The global overlay window allows passing through touch events by default except for elements with pointer-events:auto

> * does the API grant any kind of priority in terms of RAM (i.e. what would
> happen in a low memory situation)
> 

AFAIK the existence of the overlay window should not affect the priority of the overlaid content as far as it is visible in the screen, so we'll have the default behavior in a low memory situation.

> I guess we would allow ads on the UX if it was part of feature and was short
> lived (e.g. it pops up to perform a function and then the user dismisses
> it).  A persistent overlay, or an unsolicited ad, not so much.  (cc'ing
> adora too)

IMHO we should only allow this kind of overlays (global) to a very limited set of apps with use cases similar to BuddyUp (i.e. remote support, screensharing, etc.). Use cases like advertisement should be allowed only with local overlays (bug 1116779).

Comment 37

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> IMHO we should only allow this kind of overlays (global) to a very limited
> set of apps with use cases similar to BuddyUp (i.e. remote support,
> screensharing, etc.). Use cases like advertisement should be allowed only
> with local overlays (bug 1116779).

That sounds reasonable to me - I wonder if we can enforce enforce that kind of a policy in the review process though. What do you think, lisa/andrew?

Comment 38

[Lisa Brewster [:adora]]

We can update the app review criteria to specify what the API -can't- be used for, as long as there are legitimate security/user manipulation reasons.  We can make recommendations on what scenarios would be good use cases for this API, but we can't put a hard limit on the allowable use cases up front.  

I would also be willing to add a disclaimer that this API can have far reaching implications, and offer to pre-review app ideas so that developers don't waste time writing code that we would reject. (ie. "I want to write an app that does foo, would that be ok?")

Comment 39

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Lisa Brewster [:adora] from comment #38)
> We can update the app review criteria to specify what the API -can't- be
> used for, as long as there are legitimate security/user manipulation
> reasons.  We can make recommendations on what scenarios would be good use
> cases for this API, but we can't put a hard limit on the allowable use cases
> up front.  
> 
> I would also be willing to add a disclaimer that this API can have far
> reaching implications, and offer to pre-review app ideas so that developers
> don't waste time writing code that we would reject. (ie. "I want to write an
> app that does foo, would that be ok?")

Late response, but thats sounds great thanks!