702880 - Allow pages to explicitly change visibilityState of child iframes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The b2g home screen includes a little "window manager" that needs to do what traditional window managers do, put "windows" (iframes) in the foreground and background.  It's important that putting a window in the "background" forces a VisibilityChange event in its frame, so that if it's e.g. a resource-intensive game it can throttle down its resource usage.  But at the same time, the window manager would like to use these background "windows" in its UI, e.g. for a window switcher.

But, currently the only way for an outer frame to change an inner iframe's state is by removing it from the document.  And obviously when that happens, the child frame can't be displayed.  That situation is OK for now but we have to trade off battery for "real" UI, which sucks.

bz and I discussed adding an API on iframes to allow explicitly changing their visibility state.  That would be an ideal solution to this problem.  There was mention of this being privileged to avoid having to sell other UAs on the API, but that's sort of against the principle we're trying to follow in b2g so far.  Maybe moz-prefixing is an acceptable compromise for now?  I can't think of any security issues raised by this off-hand, but so far only privileged apps will need to use it (home screen and browser).  Either way is fine with me initially.

bz mentioned wanting to only allow this between different docshell types, but I'm not sure we'll be able to preserve that in b2g; it depends on what type of docshell the home screen gets.  The setup so far is
  <window>
    <browser type="???">           <== home screen
  --- needs to set viz ---
      <iframe type="magic">        <== generic app
      <iframe type="magic">        <== browser
    --- needs to set viz ---
        <iframe type="content">    <== "tab" in browser

If the home screen gets a chrome-type docshell, then we'll only have explicit sets across different docshell types.  Otherwise, we won't.

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: WIP: Add a setActive method on mozbrowsers

I am working on this now, the current api is

> document.createElement('iframe');
> iframe.mozbrowser = true;
> var req = iframe.setActive(false);
> req.onsuccess = function() { ....

Before I put it in review wanted a bit of feedback on the API, I dont love the name setActive, thats slight bikeshedding though.

My main problem is that its currently writeonly, I have no easy way to test if an frame is currently active or not, and adding an .isActive flag isnt particularly easily since the chrome process doesnt have access to the docshell (of the frame content process), We cant check docShell.isActive as that can only work async and an isActive().onsuccess is a ridiculous api.

I can keep an isActive flag on the chrome process, just worried that it would be pretty easy to get it in an inconsistent state.

Attached the current patch so far (with just the setActive() implemented)

Comment 2

[Justin Lebar (not reading bugmail)]

> I dont love the name setActive, thats slight bikeshedding though.

setIsActive?  :)

It's not really "active" though -- it's visibility, right?  We're doing this because making an iframe display:none doesn't make it invisible?

> I can keep an isActive flag on the chrome process, just worried that it would be pretty easy to get 
> it in an inconsistent state.

It depends on what we want the isActive flag to mean.  Does it mean "this docshell is marked as active" or does it return the last value we passed in for isActive()?  I'd think it should do the latter, unless there's some reason to read the docshell's true active state?

> My main problem is that its currently writeonly,

But I'm OK with write-only for now.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The visibility state is an enumeration, so I would recommend making a more flexible API.

Comment 4

[Justin Lebar (not reading bugmail)]

The spec says that we can't change the visibility state when the iframe is display:none'ed.  What's your specific suggestion for making this API more flexible?

We certainly could break the spec's rule for <iframe moz{app,browser}>.  I don't know how hard that would be, but my guess is "kinda tricky".

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #4)
> The spec says that we can't change the visibility state when the iframe is
> display:none'ed.  What's your specific suggestion for making this API more
> flexible?
> 

We can't setActive() or whatever for display:none either, right?

I'm just suggesting we make an API that allows setting the state to an enumerated value, since we may want to use any of { "hidden", "visible", "prerender", "unloaded" } or future extensions in a window manager, not just visible/hidden.

Comment 6

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

First off, I assume we're only planning on doing this for mozbrowser/mozapp iframes, right? If I'm wrong about that please let me know and ignore the rest of this comment.

Technically we can do whatever we want for a mozbrowser/mozapp iframe since they are not standardized yet. We could even say that display:none on such a frame does indeed mark the containing document as not-visible.

I'm not sure if that's a terribly good idea though, for various reasons that I'm happy to discuss if needed.

I think the API that we expose to XUL-browsers is just a boolean flag. The unloaded state is handled by gecko since it knows when a document has been unloaded. Likewise I think we would know that for prerender if we decided to implement that.

The only part that gecko doesn't know is if a document is currently displayed enough to be considered visible.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonas Sicking (:sicking) from comment #6)
> First off, I assume we're only planning on doing this for mozbrowser/mozapp
> iframes, right? If I'm wrong about that please let me know and ignore the
> rest of this comment.
> 

Initially, but I don't know of any reason yet to limit this to mozbrowser/mozapp.

> I think the API that we expose to XUL-browsers is just a boolean flag. The
> unloaded state is handled by gecko since it knows when a document has been
> unloaded. Likewise I think we would know that for prerender if we decided to
> implement that.
> 

That only works if gecko is control of prerender.  I don't know why would want to make that assumption.  Another state that I think would be useful is "poster" or "snapshot", to allow apps to show a different UI for a task switcher or something, which gecko definitely won't control.

That part of the discussion doesn't really bear on whether this is just for mozbrowser/mozapp, only on the API we expose.

Comment 8

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

The problem with putting the full state in control of the parent frame, is that that means that the parent frame also has to handle things like the "unloaded" state. Which makes it a property of the contained Document rather than the container Window/iframe.

Alternatively we create an API where you can set an arbitrary visibilityState on the parent iframe, and then just ignore that state in cases when we think that Gecko knows better, such as for the "unloaded" state.

But that means that we'll also have to define which strings do things like slow down setTimeout and requestAnimationFrame.

We'd also have to change more of the backend since that's currently using an enum rather than an arbitrary string.

Comment 9

[Justin Lebar (not reading bugmail)]

I think we're making this way more complicated than it needs to be.

First, this is only needed atm for mozbrowser/mozapp.  Can we deal with whether we want it for general content separately?

Second, at the moment, we want to set the state to visible/hidden.  That's a boolean.  If at some later date we have a need to explicitly set the state to some wider domain of values, we can change things.  Because we restricted our scope to mozbrowser/mozapp, there won't be much code to change.

Experience with a prototype is always informative.

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: Add setVisible method to mozbrowser

There didnt seem to be much consensus on this api, so putting up the simplest version, changed to setVisible(flag) still write only

I preferred handling the logic (type check) in the Child to keep any logic in the same place, the parent is just marshalling, but that means an extra IPC we dont need so can move it back to the parent if needed (also I couldnt find a decent example of the format of the error I should throw, so that probably needs changed)

if this is to be readable as well as writable, or to be an enumeration, I will probably need some decision on how to sensibly provide the return value through an sync api, because we really cant do iframe.isVisible(function() (imo)

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: Add setVisible method to mozbrowser

Removed test debug statements

Comment 12

[Justin Lebar (not reading bugmail)]

>@@ -124,16 +127,34 @@ BrowserElementChild.prototype = {
>     ctx.drawWindow(content, 0, 0, content.innerWidth,
>                    content.innerHeight, "rgb(255,255,255)");
>     sendAsyncMsg('got-screenshot', {
>       id: data.json.id,
>       screenshot: canvas.toDataURL("image/png")
>     });
>   },
> 
>+  _recvSetVisible: function(data) {
>+    if (typeof data.json.visible !== 'boolean') {
>+      sendAsyncMsg('got-setvisible', {
>+        id: data.json.id,
>+        error: true,
>+        errorMsg: 'TYPE_ERROR: active flag must be a boolean'
>+      });

We usually don't type check in the DOM.  For example,

  window.addEventListener('load', function() { ... }, "abc");

works without error, even though the last param should be a boolean.

I think XPCOM will be happy for you to do |docshell.isActive = "abc"| -- it should cast "abc" to a bool -- but if that doesn't work, you could always do |docShell.isActive = !!data.json.visible|.

>+      return;
>+    }
>+    if (docShell.isActive !== data.json.visible) {
>+      docShell.isActive = data.json.visible;
>+    }
>+    sendAsyncMsg('got-setvisible', {
>+      id: data.json.id,
>+      visible: data.json.visible
>+    });
>+  },

If we get rid of the type-checking, we can also get rid of this message, and the DOMRequest, and make setVisible just a one-way thing?

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Add setVisible method to mozbrowser

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 626209 [details] [diff] [review]
Add setVisible method to mozbrowser

Nice.

Comment 15

[Dale Harvey (:daleharvey)]

pushed to try

https://tbpl.mozilla.org/?tree=Try&rev=36c4d7773c27

Comment 16

[Justin Lebar (not reading bugmail)]

Are you happy with the try results here?  Would you like me to push this?

Comment 17

[Dale Harvey (:daleharvey)]

Yup happy to get this one pushed, cheers

Diving into the rendering for the other bug (getScreenshot), will know a hell of a lot more by the time it gets fixed, hopefully soon

Comment 18

[Justin Lebar (not reading bugmail)]

Sorry, this has bitrotted.  Would you mind rebasing this patch so it applies cleanly?

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: Add setVisible method to mozbrowser

Updated to fix bitrot

Comment 20

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce695c26fc5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ce695c26fc5

Comment 23

[Justin Lebar (not reading bugmail)]

This API is only accessible by pages which embed <iframe mozbrowser>, so the security review of this bug should fall under the larger browser-api security review.