Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The fullscreen enabled flag is not set for the initial about:blank document in an iframe? #1385

Closed
foolip opened this issue Jun 3, 2016 · 11 comments
Closed

The fullscreen enabled flag is not set for the initial about:blank document in an iframe? #1385

foolip opened this issue Jun 3, 2016 · 11 comments
Assignees
@annevk

Comments

@foolip
Copy link
Member

foolip commented Jun 3, 2016

https://html.spec.whatwg.org/#initialise-the-document-object

Here the bits about fulscreen come after the "Implement the sandboxing" call.

https://html.spec.whatwg.org/#creating-a-new-browsing-context

Here there's nothing about fullscreen.

Am I missing something, or does this mean that for the initial about:blank document, the flag can never be set? Here's a test that in Gecko allows the iframe to go fullscreen. (It does in Chromium as well, but that's because of https://bugs.chromium.org/p/chromium/issues/detail?id=403730)

<!doctype html>
<script>
function loaded() {
  var iframe = document.querySelector('iframe');
  var doc = iframe.contentDocument;
  var body = doc.body;
  var button = doc.createElement('button');
  button.innerText = 'toogle fullscreen';
  button.onclick = function() {
    if (doc.fullscreenElement) {
      doc.exitFullscreen();
    } else {
      body.requestFullscreen();
    }
  };
  body.appendChild(button);
}
</script>
<iframe allowfullscreen onload="loaded()"></iframe>

@annevk @upsuper @jernoble (can't find GitHub account of Ali Alabbas)
@annevk
Copy link
Member

annevk commented Jun 8, 2016 •
edited
Loading

We should probably check when allowfullscreen was added if there was a reason, but it looks like it should have been added to the #implement-the-sandboxing algorithm rather than inline.

@annevk
Copy link
Member

annevk commented Jun 8, 2016

It looks like #implement-the-sandboxing used to be part of initialising a new document 8359fee.

@annevk
Copy link
Member

annevk commented Jun 8, 2016

Looks like an oversight in 52a28a2?

Though in general having multiple places to initialize a document and all that comes with it is a very brittle setup...

@annevk
Copy link
Member

annevk commented Jun 8, 2016

Are those the only two places we create and initialize documents? I'm happy to work on this.

@annevk annevk self-assigned this Jun 9, 2016
annevk added a commit that referenced this issue Jun 9, 2016
@annevk
Use fullscreen logic for initial about:blank documents too
d26fec3 
Fixes #1385.

(It seems this was an oversight
8359fee resulting from having two
places to initialize document objects as the result of navigation.
Cleaning that up is a larger separate task.)
@annevk annevk mentioned this issue Jun 9, 2016
Merged
@upsuper
Copy link
Member

upsuper commented Jun 10, 2016

Firefox changes to the spec behavior in bug 1270648, which breaks actual web content like bug 1279613.

@annevk
Copy link
Member

annevk commented Jun 12, 2016

@upsuper does the PR address that issue? It seems like it does, but I'm not a 100% sure.

@upsuper
Copy link
Member

upsuper commented Jun 12, 2016

I'm not quite familiar with this, but I'll try to understand what is changed there. I suppose sandbox is also affected, so it's not something fullscreen-specific.

cc @bzbarsky

@bzbarsky
Copy link
Contributor

@upsuper The current text of the spec is clearly wrong. We should be setting the fullscreen enabled flag as needed for initial about:blank, just like for every other document.

@upsuper
Copy link
Member

upsuper commented Jun 15, 2016 •
edited
Loading

@annevk It seems to me your change works. I'm a bit concerned about the naming "implement the sandboxing", though.

@annevk
Copy link
Member

annevk commented Jun 16, 2016

I see, we can probably rename that. Having said that, I suspect that eventually we'll merge both call sites of that algorithm at which point we can just inline it and the name will be gone.

Having two different places to set up the global environment in the standard is clearly broken and has led to many subtle bugs.

foolip pushed a commit that referenced this issue Jun 21, 2016
@annevk @foolip
Use fullscreen logic for initial about:blank documents too
c2a8d60 
Fixes #1385.

(It seems this was an oversight
8359fee resulting from having two
places to initialize document objects as the result of navigation.
Cleaning that up is a larger separate task.)
@foolip
Copy link
Member Author

foolip commented Jun 21, 2016

@aliams FYI

@annevk annevk mentioned this issue Jun 21, 2016
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@annevk annevk

Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@upsuper @foolip @bzbarsky @annevk