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

Clarify document.open() and refactor window.open() #2672

Merged
merged 1 commit into from May 16, 2017

Conversation

2 participants
@annevk
Member

annevk commented May 12, 2017

No description provided.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 12, 2017

Member

This can be reviewed, but I plan to add more here on top of #2668 (to make document.open() invoke window.open() without actually saying it like that). If this is already reviewed I will do that as a separate commit for easy of review.

Noteworthy: the window open steps don't need this Window object passed as far as I can tell.

Member

annevk commented May 12, 2017

This can be reviewed, but I plan to add more here on top of #2668 (to make document.open() invoke window.open() without actually saying it like that). If this is already reviewed I will do that as a separate commit for easy of review.

Noteworthy: the window open steps don't need this Window object passed as far as I can tell.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 12, 2017

Member

document.open() actually does invoke window.open() in browsers; there are tests for that and everything. We should probably note that more explicitly, talking about how unusual it is.

Member

domenic commented May 12, 2017

document.open() actually does invoke window.open() in browsers; there are tests for that and everything. We should probably note that more explicitly, talking about how unusual it is.

<p>When the method is invoked, the user agent must run the following steps:</p>
<p>The <dfn>window open steps</dfn>, given a string <var>url</var>, a string <var>target</var>,
and a string <var>features</var>, are as follows:</p>

This comment has been minimized.

@domenic

domenic May 12, 2017

Member

Given how document.open() actually calls window.open(), I don't think this indirection will be necessary; it can go back to the previous paragraph (maybe omitting "the user agent must").

@domenic

domenic May 12, 2017

Member

Given how document.open() actually calls window.open(), I don't think this indirection will be necessary; it can go back to the previous paragraph (maybe omitting "the user agent must").

Show outdated Hide outdated source
<li><p>Otherwise, <span>navigate</span><!--DONAV window.open()--> <var>target browsing
context</var> to <var>resource</var>, with the <var><span>exceptions enabled flag</span></var>
set. If <var>new</var> is true, then <span data-x="replacement enabled">replacement must be
enabled</span>. The <span>source browsing context</span> is <var>source browsing context</var>.

This comment has been minimized.

@domenic

domenic May 12, 2017

Member

Ugh, the way "replacement enabled" is implicitly passed through from navigate to history traversal is annoying. That seems like a relatively easy fix at some point; perhaps worth filing a bug if there's not one already.

@domenic

domenic May 12, 2017

Member

Ugh, the way "replacement enabled" is implicitly passed through from navigate to history traversal is annoying. That seems like a relatively easy fix at some point; perhaps worth filing a bug if there's not one already.

This comment has been minimized.

@annevk

annevk May 16, 2017

Member

Done.

@annevk

annevk May 16, 2017

Member

Done.

Show outdated Hide outdated source
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 13, 2017

Member

Can you point me to such a test? A simple

window.open = function() {
  w(1)
}
w(document.open("", "", "")); // 1, 2, 3 makes Edge throw

tries to open a new browsing context in Edge and Firefox. It does seem to print 1 in Chrome and Safari though, but not sure why that would be desirable.

Member

annevk commented May 13, 2017

Can you point me to such a test? A simple

window.open = function() {
  w(1)
}
w(document.open("", "", "")); // 1, 2, 3 makes Edge throw

tries to open a new browsing context in Edge and Firefox. It does seem to print 1 in Chrome and Safari though, but not sure why that would be desirable.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 15, 2017

Member

(Note: there are some issues around testing document.open() in live-dom-viewer, I believe because it uses document.open() to do its work.)

It looks like it's a 2/2 split, with Chrome/Safari actually calling window.open(). See https://w3c-test.org/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html, which tests that it does not call window.open().

So I guess we can change it to the more reasonable behavior, but let's be sure to file bugs. I think that will also moves this change out of the "editorial" realm, as previously the spec could really be interpreted either way (and apparently was).

Member

domenic commented May 15, 2017

(Note: there are some issues around testing document.open() in live-dom-viewer, I believe because it uses document.open() to do its work.)

It looks like it's a 2/2 split, with Chrome/Safari actually calling window.open(). See https://w3c-test.org/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html, which tests that it does not call window.open().

So I guess we can change it to the more reasonable behavior, but let's be sure to file bugs. I think that will also moves this change out of the "editorial" realm, as previously the spec could really be interpreted either way (and apparently was).

@annevk annevk changed the title from Editorial: refactor window.open() to Clarify document.open() and refactor window.open() May 16, 2017

Clarify document.open() and refactor window.open()
document.open() should not invoke public window.open().
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 16, 2017

Member

I think this is good to go now.

Member

annevk commented May 16, 2017

I think this is good to go now.

@domenic domenic merged commit 29afad4 into master May 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@domenic domenic deleted the annevk/window.open branch May 16, 2017

foolip added a commit that referenced this pull request Oct 7, 2018

Change an exception name in document.open() to "InvalidAccessError"
This was changed in #2672 without
being explicitly discussed. This was discovered because the tests
still match the old behavior:
web-platform-tests/wpt#13411

domenic added a commit that referenced this pull request Oct 8, 2018

Change an exception name in document.open() to "InvalidAccessError"
This was changed in #2672 without
being explicitly discussed. This was discovered because the tests
still match the old behavior:
web-platform-tests/wpt#13411.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment