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

Define an active service worker for a srcdoc iframe #3714

Closed
wants to merge 5 commits into from

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented May 25, 2018

Attempting to close w3c/ServiceWorker#765

@annevk we discussed this on irc, but:

  • This change doesn't currently cover parent SW registration that happened after the srcdoc iframe was created.
  • This change doesn't cover about:blank

For the former, I don't know if we need to resolve this at the first iteration (I'm not sure that's a critical case, as it only impact the first view with late SW registration, so most resource loads are likely to have already happen by the time the SW is registered).
For the latter, can you elaborate on why it matters in practice? Does about:blank load resources?


/iframe-embed-object.html ( diff )

@wanderview
Copy link
Member

For the latter, can you elaborate on why it matters in practice? Does about:blank load resources?

They totally can. Consider:

let f = document.createElement('iframe');
document.body.appendChild(f);
await new Promise(resolve = f.onload(resolve));

// any inherited service worker controller should itnercept
f.contentWindow.fetch(someURL);

// dynamically added resources should be intercepted by inheritted controller
let i = f.contentWindow.document.createElement('img');
i.src = someURL;
f.contentWindow.document.body.appendChild(i);

@yoavweiss
Copy link
Contributor Author

Got it! Thanks :)

source Outdated
data-x="attr-iframe-srcdoc">srcdoc</code> document</span>. The
<span data-x="concept-environment-active-service-worker">active service worker</span>
of the resulting <code>Document</code>'s <span>environment</span>
should be set to the
Copy link
Member

Choose a reason for hiding this comment

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

"should"? Why not "must"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! Thanks for catching that :)

source Outdated
<span data-x="concept-environment-active-service-worker">active service worker</span>
of the resulting <code>Document</code>'s <span>environment</span> must be set to the
<span data-x="concept-environment-active-service-worker">active service worker</span>
of the <code>iframe</code> element's <span>node document</span>'s <span>environment</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is wrong here. This also doesn't quite seem like the right place to define this, since this would miss out <object> or <embed> navigated to about:blank.

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 indentation and added handling for <object> (<embed> about:blank doesn't seem to be defined)

must be set to the
<span data-x="concept-environment-active-service-worker">active service worker</span>
of the <code>iframe</code> element's <span>node document</span>'s
<span>environment</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine for now, at least until we better define document creation.

@yoavweiss
Copy link
Contributor Author

Related tests are at web-platform-tests/wpt#11078

source Outdated
data-x="concept-event-fire">fire an event</span> named <code data-x="event-load">load</code>
at the <code>object</code> element. <span class="note">No <code
instead, the user agent must:
<ul>
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 <ol>, preceded by a newline.

Wrapping is also not consistently at 100 columns for changed lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@annevk annevk requested a review from domenic May 28, 2018 13:17
@annevk
Copy link
Member

annevk commented May 28, 2018

@domenic should review this too.

@domenic
Copy link
Member

domenic commented May 29, 2018

This doesn't seem great. Although document creation might be a bit iffy, environment settings object creation is pretty well-defined, and this circumvents it. In particular, the text introduced in this PR contradicts steps 5 and 6 of https://html.spec.whatwg.org/#set-up-a-window-environment-settings-object. And in doing so it raises questions that I, at least, don't know the answer to, like how this from-a-distance override interacts with reserved environments.

I'd suggest instead modifying "set up a window environment settings object" to set the active service worker based on the Document, by checking if window's associated Document is an iframe srcdoc document. That seems legit, similar to what is done for referrer policy.

For the about:blank case... I guess you'd need to get a bit more hand-wavey if you wanted to locate that correctly inside the "set up a window environment settings obejct" algorithm. E.g. use a phrase like "if Document is the initial about:blank document for an iframe element".

An alternative is to explicitly let "set up a window environment settings object" do its thing (during the call to "Navigate"), and then override it after navigation completes. This would be similar to what this PR does, but: you'd say "set ... to" instead of "must be", and you'd need to fix the setting for about:blank to be after navigation, not before. This seems messier and more likely to be wrong (or at least, not obviously right) in the case of reserved environments. But it might work.

@yoavweiss
Copy link
Contributor Author

Thanks for the feedback. If I read reserved environments correctly (and it's fairly likely that I don't), they are only passed to set up a window environment settings object when navigating across documents, which (again IIUC) is not what iframes do.

Is that correct? If so, where is the active service worker set for iframes?

@yoavweiss
Copy link
Contributor Author

Closing this PR as I'm continuing the work at #3725

@yoavweiss yoavweiss closed this May 30, 2018
@domenic
Copy link
Member

domenic commented May 30, 2018

I am pretty sure iframes navigate across documents. How else would they load pages?

@annevk
Copy link
Member

annevk commented May 30, 2018

Yeah, they navigate. Initial about:blank load might be magical though.

@yoavweiss
Copy link
Contributor Author

OK, I misunderstood then

<span data-x="concept-environment-active-service-worker">active service worker</span>
of the resulting <code>Document</code>'s <span>environment</span> must be set to the
<span data-x="concept-environment-active-service-worker">active service worker</span>
of the <code>iframe</code> element's <span>node document</span>'s <span>environment</span>.</p></li>

Choose a reason for hiding this comment

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

  1. What about other types of frames, such as FRAME or OBJECT? Should they get service workers as well?
  2. Is it always OK to use the parent browsing context's active document's environment? https://html.spec.whatwg.org/multipage/origin.html#origin seems to imply that about:blank doesn't always inherit the origin of the parent browsing context's active document, so this would cause a mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about other types of frames, such as FRAME or OBJECT? Should they get service workers as well?

<object> was part of the previous PR, but I indeed left it out when changing where the processing is done. I'll add both.

Is it always OK to use the parent browsing context's active document's environment? https://html.spec.whatwg.org/multipage/origin.html#origin seems to imply that about:blank doesn't always inherit the origin of the parent browsing context's active document, so this would cause a mismatch?

That's a good point. I'm not sure, but I'll find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zetafunction - regarding the comment on object, I got confused. Please see #3725 for the latest PR.

You points regarding <frame> and about:blank origin still stand, I believe.

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

Successfully merging this pull request may close these issues.

serviceworker for iframes with srcdoc
6 participants