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

Add support for <embed> and <object> containerTypes. #65

Closed
wants to merge 2 commits into from

Conversation

tdresser
Copy link
Contributor

Fixes #52.

@tdresser tdresser requested a review from npm1 June 12, 2019 21:18
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
1. Set |attribution|'s {{TaskAttributionTiming/containerType}}, {{TaskAttributionTiming/containerName}}, {{TaskAttributionTiming/containerSrc}}, and {{TaskAttributionTiming/containerId}} to <code>null</code>.
1. If |culpritSettings| is not <code>null</code>
1. Let |container| be the |culpritSettings|'s <a>responsible browsing context</a>'s <a>browsing context container</a>.
1. If |container| is not <code>null</code>, set |attribution|'s {{containerId}} attribute to the value of |container|'s [=Element/ID=], or <code>null</code> if the ID is unset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can |container| be null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also DOMString nullness problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed DOMString nullness - I'm not sure if |container| can be null. @domenic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 95% sure it cannot be null. A BC container can have a null nested BC, but if a nested BC exists, then the BC container must exist, I believe.

</pre>
1. Set |attribution|'s {{containerType}} attribute to "<code>frame</code>".
1. Set |attribution|'s {{containerName}} attribute to the value of |container|'s <{frame/name}> content attribute, or the empty string if the attribute is absent.
1. Set |attribution|'s {{containerSrc}} attribute to the value of |container|'s <{frame/src}> content attribute, or the empty string if the attribute is absent.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to get this to link correctly, with the correct formatting. @domenic - any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like obsolete elements are intentionally (?) missing linkable <dfn>s for their attributes. The HTML spec itself just using <code>src</code>, see e.g. https://html.spec.whatwg.org/#frames "process frame attributes". So yeah, just doing that here is probably the way to go.

npm1 added a commit that referenced this pull request Nov 28, 2019
This is basically #65 but doing a new PR because rebasing would've taken me longer.
Fixes #52
@domenic domenic closed this in c7144c2 Dec 2, 2019
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.

The processing model only works for iframes, not object/embed/etc.
3 participants