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

HTML: Extend Test Coverage for _parent named browsing context (7.1.5) #5150

Merged

Conversation

lyzadanger
Copy link
Contributor

This PR extends some existing tests for the _parent named browsing context and adds some new ones.

The Webkit/Chrome case-sensitivity bug noted in #5145 is indeed manifest here, again, as expected. I will be opening bugs shortly and will link back.

There's also an interesting difference in browser behavior WRT choose-browsing-context-parent-003.html.

According to the spec:

The opener IDL attribute on the Window object, on getting, must return the WindowProxy object of the browsing context from which the current browsing context was created (its opener browsing context)

(emphasis mine)

Safari and Chrome are both steadfastly retaining a browsing context's original opener, even if another browsing context later opens a different document into that context/window. FireFox, however, will replace the window.opener of the _parent context with the window that invokes .open (assuming the opener isn't disowned, etc.).

I have two hunches here: Firefox's behavior is the errant one and this is likely going to affect more than just the named _parent context. But I'm not 100% certain on the former.

Supporting files have been lightly organized but there is still more work to do in this section as I proceed.

@wpt-pr-bot
Copy link
Collaborator

Notifying @ayg, @jdm, @jgraham, @zcorpan, and @zqzhang. (Learn how reviewing works.)

@ghost
Copy link

ghost commented Mar 15, 2017

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 2f87ce3501be9c9e4dd48ba18f655065d40ea41b
Using browser at version BuildID 20170326110231; SourceStamp f5e214144799889e2408c4841351f4053f00544e
Starting 10 test iterations
All results were stable

All results

7 tests ran
/html/browsers/windows/browsing-context-names/browsing-context-choose-existing.html
Subtest Results Messages
OK
The browsing context must be chosen if the given name is same as its name PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-001.html
Subtest Results Messages
OK
The parent browsing context must be chosen if the given name is _parent PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-002.html
Subtest Results Messages
OK
choosing _parent context: multiple nested contexts PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-003.html
Subtest Results Messages
TIMEOUT
_parent should reuse window.parent context TIMEOUT Test timed out
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-004.html
Subtest Results Messages
OK
choosing _parent context should be case-insensitive PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-self-1.html
Subtest Results Messages
OK
The current browsing context must be chosen if the given name is '_self' PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html
Subtest Results Messages
TIMEOUT
The current browsing context must be chosen if the given name is empty string NOTRUN

@ghost
Copy link

ghost commented Mar 15, 2017

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 2f87ce3501be9c9e4dd48ba18f655065d40ea41b
Using browser at version 59.0.3047.0 dev
Starting 10 test iterations
All results were stable

All results

7 tests ran
/html/browsers/windows/browsing-context-names/browsing-context-choose-existing.html
Subtest Results Messages
OK
The browsing context must be chosen if the given name is same as its name PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-001.html
Subtest Results Messages
OK
The parent browsing context must be chosen if the given name is _parent PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-002.html
Subtest Results Messages
OK
choosing _parent context: multiple nested contexts PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-003.html
Subtest Results Messages
OK
_parent should reuse window.parent context PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-parent-004.html
Subtest Results Messages
TIMEOUT
choosing _parent context should be case-insensitive TIMEOUT Test timed out
/html/browsers/windows/browsing-context-names/browsing-context-choose-self-1.html
Subtest Results Messages
OK
The current browsing context must be chosen if the given name is '_self' PASS
/html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html
Subtest Results Messages
TIMEOUT
The current browsing context must be chosen if the given name is empty string NOTRUN

window.addEventListener('message', t.step_func_done(e => {
assert_equals(e.data.name, 'parentWin');
}));
window.frames['embedded'].src = 'resources/parent-iframe-1.html';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get confused about the intent here, if it's trying to set src on the iframe or on its WindowProxy. In Safari, if the iframe happens to not have a name attribute, this would return the WindowProxy. I think that is a bug per https://html.spec.whatwg.org/#named-access-on-the-window-object - but testing that should be separate.

Here it would be clearer to just use document.getElementById('embedded').

@zcorpan
Copy link
Member

zcorpan commented Mar 16, 2017

Are the TIMEOUT/NOTRUN results in /html/browsers/windows/browsing-context-names/browsing-context-choose-parent-004.html and /html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html expected?

@lyzadanger
Copy link
Contributor Author

@zcorpan

Are the TIMEOUT/NOTRUN results in /html/browsers/windows/browsing-context-names/browsing-context-choose-parent-004.html and /html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html expected?

For 004, yes! I'm about to open bugs against both Chrome and Webkit regarding that—both are erroneously (I believe) treating named browsing contexts (e.g. _top, _parent) as case-sensitive. The spec says they should be case-insensitive.

For self-2.thml...that one's a pre-existing test. I have tackled _blank and _parent named contexts so far: _top and _self are next. I'll take a look at that test when I work on the _self coverage!

@lyzadanger
Copy link
Contributor Author

As promised, bugs opened on case-sensitive _parent issue:

@ghost
Copy link

ghost commented Mar 23, 2017

These tests are now available on w3c-test.org

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<div id="log"></div>
<iframe id="embedded" src="resources/parent-iframe-1.html" name="parentWin" style="display:none"></iframe>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to after the script. In principle there is a race condition otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah! That's the reason I had the hard-to-read assignment of src to that iframe in the script and not in the markup initially. OK, I'll rectify.

@zcorpan
Copy link
Member

zcorpan commented Mar 24, 2017

So in Gecko for /html/browsers/windows/browsing-context-names/browsing-context-choose-parent-003.html I get a TIMEOUT, that is because of the behavior you describe in the OP, correct?

@lyzadanger
Copy link
Contributor Author

@zcorpan Yep! I had to go remind myself by re-reading my original opening comment. Yes, this looks potentially buggy in Gecko.

@zcorpan
Copy link
Member

zcorpan commented Mar 27, 2017

@zcorpan
Copy link
Member

zcorpan commented Mar 27, 2017

Not sure why 39948b2 is now in this PR. I suppose "squash and merge" does the right thing, but leaving this open if you want to clean up this branch before landing @lyzadanger

@lyzadanger
Copy link
Contributor Author

@zcorpan @jgraham asked me to rebase onto #5196 before it was merged because the CI was wedging. I can re-rebase my branch to get rid of it. Will do shortly.

- Add tests for reused `parent` context
- Add tests for case-sensitivity
- Organize supporting files
@lyzadanger lyzadanger force-pushed the browsing-context-names-parent branch from f125f12 to 070a1f4 Compare March 27, 2017 16:10
@lyzadanger lyzadanger merged commit e27e750 into web-platform-tests:master Mar 27, 2017
@bzbarsky
Copy link
Contributor

bzbarsky commented May 5, 2017

Safari and Chrome are both steadfastly retaining a browsing context's original opener

Do they? Simple testcase:

<!DOCTYPE html>
<input type="button" onclick="alert(opener)" value="check opener">
<input type="button" onclick="window.open(location.href, '_self')" value="do open">

Click the "check opener" button, observe null, click the "do open" button, click the "check opener" button, observe non-null. In both Safari and Chrome.

@bzbarsky
Copy link
Contributor

bzbarsky commented May 5, 2017

I filed whatwg/html#2636 to get this sorted out in the spec. Given the behavior on the testcases there, I have no idea how Blink or WebKit pass html/browsers/windows/browsing-context-names/browsing-context-choose-parent-003.html from this PR, unless they somehow special-case the "opener would be one of our descendants" case or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants