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.

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

allow-modals usage of incumbent globals is wacky #1206

Closed
bzbarsky opened this issue May 5, 2016 · 2 comments · Fixed by #1473
Closed

allow-modals usage of incumbent globals is wacky #1206

bzbarsky opened this issue May 5, 2016 · 2 comments · Fixed by #1473

Comments

@bzbarsky
Copy link
Contributor

bzbarsky commented May 5, 2016

Things like alert say:

If the active sandboxing flag set of the active document of the responsible browsing context specified by the incumbent settings object has the sandboxed modals flag set, then abort these steps.

This makes no sense to me. If you have the incumbent settings already, why would you then go through the browsing context to the active document, which may have absolutely nothing to do with the incumbent global's most recent document?

@mikewest it looks like you wrote this bit. What was the intent here?

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 5, 2016

Oh, and to be clear, I consider the current text totally unacceptable and will be implementing something different in Gecko. I'm just trying to figure out what the something should be. Currently I'm leaning towards just using the sandboxing flag set of the newest document of the relevant settings object, but I could be convinced to use the incumbent's newest document, I guess.

@domenic
Copy link
Member

domenic commented May 5, 2016

relevant or current would be ideal in light of our recent discussions and the resulting advice in https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects, yeah. Current seems to align better with that advice, actually? Maybe we should tweak our advice?

@mikewest to be clear, here is what we are asking:

<!-- a.html -->
<!DOCTYPE html>
<html lang="en">
<title>Entry page</title>

<iframe src="b.html"></iframe>
<button onclick="frames[0].hello()">Hello</button>

<!--b.html -->
<!DOCTYPE html>
<html lang="en">
<title>Incumbent page</title>

<iframe src="c.html" id="c"></iframe>
<iframe src="d.html" id="d"></iframe>

<script>
  const c = document.querySelector("#c").contentWindow;
  const d = document.querySelector("#d").contentWindow;

  window.hello = () => {
    c.alert.call(d);
  };
</script>

When we click on the button in a.html, which global object's active document's sandboxed modals flag should be consulted?

  • entry: a.html
  • incumbent: b.html
  • current: c.html
  • relevant: d.html

I guess there are even more variations possible, since the current text says "the active document of the responsible browsing context specified by the incumbent settings object" instead of "the active document of the incumbent global object", but this example is complicated enough already, and I think we can hopefully all agree that the current text is not a good idea.

domenic added a commit that referenced this issue Jun 29, 2016
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome, as shown by
https://allow-modals-check-zzvyjxgdhg.now.sh.

This also helps the efforts in #1430.
domenic added a commit that referenced this issue Jun 29, 2016
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome and Firefox Nightly
(which are the only browsers which implement sandboxing of modals), as
shown by https://allow-modals-check-zzvyjxgdhg.now.sh/entry.html.

This also helps the efforts in #1430.
domenic added a commit that referenced this issue Jun 29, 2016
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome and Firefox Nightly
(which are the only browsers which implement sandboxing of modals), as
shown by https://allow-modals-check-pijoohuobf.now.sh/entry.html.

This also helps the efforts in #1430.
domenic added a commit that referenced this issue Jun 29, 2016
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome and Firefox Nightly
(which are the only browsers which implement sandboxing of modals), as
shown by https://allow-modals-check-yucxowumar.now.sh/entry.html.

This also helps the efforts in #1430.
@domenic domenic assigned domenic and unassigned mikewest Jul 5, 2016
domenic added a commit that referenced this issue Jul 13, 2016
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

Chrome and Firefox nightly implement a slightly different behavior,
choosing the Window's browsing context's active Document, instead of the
Window's associated Document. This can be different in edge cases as
discussed in #1473 (comment).
However, in Firefox they are not distinguishable, due to another
security check that prevents these edge cases from being triggered.
#1548 proposes to add a security check like Firefox's to the spec, which
would obviate the difference in all cases. In the meantime, the choice
of associated Document is more straightforward.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This fixes whatwg#1206. As noted there, the previous indirections were wacky
and unnecessary.

Chrome and Firefox nightly implement a slightly different behavior,
choosing the Window's browsing context's active Document, instead of the
Window's associated Document. This can be different in edge cases as
discussed in whatwg#1473 (comment).
However, in Firefox they are not distinguishable, due to another
security check that prevents these edge cases from being triggered.
whatwg#1548 proposes to add a security check like Firefox's to the spec, which
would obviate the difference in all cases. In the meantime, the choice
of associated Document is more straightforward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants