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

Removes the nullable attribute from window.parent #1089

Merged
merged 7 commits into from
Aug 6, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Aug 3, 2021

Re: microsoft/TypeScript#44684 (comment)

I read the spec, and it really didn't seem like null was possible - it is either the parent, or itself:

https://dev.w3.org/html5/spec-LC/browsers.html#dom-parent

The parent IDL attribute on the Window object of a Document in a browsing context b must return the WindowProxy object of the parent browsing context, if there is one (i.e. if b is a child browsing context), or the WindowProxy object of the browsing context b itself, otherwise (i.e. if it is a top-level browsing context or a detached nested browsing context).

Maybe this could be true in environments other than dom?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@saschanaz
Copy link
Contributor

saschanaz commented Aug 3, 2021

This is the latest spec: https://html.spec.whatwg.org/multipage/browsers.html#dom-parent

It has an example, which I modified to allow copy-pasting to console:

var element = document.createElement("iframe");
document.body.append(element);
var iframeWindow = element.contentWindow;
element.remove();

console.log(iframeWindow.top === null);
console.log(iframeWindow.parent === null);
console.log(iframeWindow.frameElement === null);

All returns true. But admittedly a rare situation, so your call. I guess at least it should be in the comment that it can be null?

@orta
Copy link
Contributor Author

orta commented Aug 3, 2021

Yeah, thanks for digging!

I'll add a comment saying it's possibly null - I think this is probably worth the ergonomics change

@saschanaz
Copy link
Contributor

Should it say why it's not nullable in the lib file?

@@ -17230,7 +17230,8 @@ interface Window extends EventTarget, AnimationFrameProvider, GlobalEventHandler
readonly pageXOffset: number;
/** @deprecated This is a legacy alias of `scrollY`. */
readonly pageYOffset: number;
readonly parent: WindowProxy | null;
/** Refers to either the parent WindowProxy, or itself. If you create a child context, then remove the parent you could get null though. */
Copy link
Contributor

@saschanaz saschanaz Aug 3, 2021

Choose a reason for hiding this comment

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

How about:

Refers to either the parent WindowProxy, or itself.

It can rarely be null e.g. for contentWindow of an iframe that is already removed from the parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having it multiline doesn't really work because it's in two places with different indentations, so I made this a single line

Copy link
Contributor

Choose a reason for hiding this comment

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

I think \n automatically does the indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. postMessage

inputfiles/comments.json Outdated Show resolved Hide resolved
Orta Therox and others added 3 commits August 6, 2021 14:39
Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot merged commit 76512b3 into main Aug 6, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2021

Merging because @saschanaz is a code-owner of all the changes - thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants