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.

Sign up for GitHub

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

Jump to bottom

Change initial about:blank navigation behavior for iframes and popups #6869

Merged
merged 8 commits into from
Aug 10, 2021
Merged

Change initial about:blank navigation behavior for iframes and popups #6869

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2f751c9
Do not navigate to about:blank on iframe insertion
domenic Jul 16, 2021
bdf3e17
No load events anywhere
domenic Jul 21, 2021
a39ae5e
Fix bug in last commit and refactor window open steps a lot
domenic Jul 21, 2021
af0c760
Review comments
domenic Jul 26, 2021
cd5f327
Do *not* check query and fragment
domenic Jul 27, 2021
e0dc4c9
URL and history update steps
domenic Aug 2, 2021
5bc2d13
Fire the load event sync
domenic Aug 5, 2021
504d117
Expand the note per review
domenic Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 50 additions & 48 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30676,36 +30676,30 @@ interface <dfn interface>HTMLIFrameElement</dfn> : <span>HTMLElement</span> {
<li><p>Otherwise, if <var>element</var> has a <code data-x="attr-iframe-src">src</code> attribute
specified, or <var>initialInsertion</var> is false, then run the <span>shared attribute
processing steps for <code>iframe</code> and <code>frame</code> elements</span> given
<var>element</var>.</p></li>
<var>element</var> and <var>initialInsertion</var>.</p></li>
</ol>

<p id="otherwise-steps-for-iframe-or-frame-elements">The <dfn>shared attribute processing steps
for <code>iframe</code> and <code>frame</code> elements</dfn>, given an element
<var>element</var>, are:</p>
<var>element</var> and a boolean <var>initialInsertion</var>, are:</p>

<ol>
<li>
<p>If <var>element</var> has no <code data-x="attr-iframe-src">src</code> attribute specified,
or its value is the empty string, let <var>url</var> be the <span>URL</span>
"<code>about:blank</code>".</p>
<!-- https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2641 -->
<li><p>Let <var>url</var> be the <span>URL record</span> <code>about:blank</code>.</p></li>

<p>Otherwise, <span data-x="parse a url">parse</span> the value of <var>element</var>'s <code
data-x="attr-iframe-src">src</code> attribute, relative to <var>element</var>'s <span>node
document</span>.</p>

<p>If that is not successful, then let <var>url</var> be the <span>URL</span>
"<code>about:blank</code>". Otherwise, let <var>url</var> be the <span>resulting URL
record</span>.</p>
</li>
<li><p>If <var>element</var> has a <code data-x="attr-iframe-src">src</code> attribute specified,
and its value is not the empty string, then <span data-x="parse a url">parse</span> the value of
that attribute relative to <var>element</var>'s <span>node document</span>. If this is
successful, then set <var>url</var> to the <span>resulting URL record</span>.</p></li>

<li><p>If there exists an <span>ancestor browsing context</span> of <var>element</var>'s
<span>nested browsing context</span> whose <span>active document</span>'s <span
data-x="concept-document-url">URL</span>, ignoring <span
data-x="concept-url-fragment">fragments</span>, is equal to <var>url</var>, then return.</p></li>
<!-- https://www.hixie.ch/tests/adhoc/html/frames/iframes/ 008.html and 009.html -->
<!-- https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1969 -->
<!-- I'm assuming that "resolve" will normalize things like scheme and hostname case -->

<li><p>If <var>url</var> is <code>about:blank</code> and <var>initialInsertion</var> is true,
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 an existing problem I think, but what if it's about:blank?test or about:blank#test?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent is that it's an exact comparison to the about:blank URL record. However I agree this is not very clear. WDYT of introducing an auxiliary definition (in HTML) for "is the about:blank URL", which does proper component-wise comparisons?

Or I guess we could just compare the serialization to the string "about:blank".

I believe @rakina had tests for this in a different PR but we should probably expand web-platform-tests/wpt#29745 to include them too.

Copy link
Member

Choose a reason for hiding this comment

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

Comparing components seems ideal, but either is okay I think.

Copy link
Member

Choose a reason for hiding this comment

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

I've added about:blank#foo and about:blank?foo to the tests. Everything seems to be OK in Chrome and Safari except for 1 (load event on <iframe> element for about:blank#foo) which is probably a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably also worth checking that the URI of the resulting document is actually updated for about:blank?foo and about:blank#foo, so that location.href, location.search, location.hash, and document.documentURI are correct. If we just stuck with the initial about:blank document & fire a load event in that situation naively, we wouldn't update these parameters.

A probably unrelated issue, but it appears that in Chrome about:blank?foo has location.search == "?foo" but Firefox appears to have location.search == "".

Copy link
Member

@rakina rakina Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Chrome does something so that the resulting iframe is on about:blank?foo.
What is that something? Is it a full navigation from the initial about:blank to another "initial about:blank?foo"?

Ah, I see, sorry for missing the big question! In Chrome, new browsing contexts always start with the initial about:blank document, but then we do the second "initial" navigation synchronously on it when the src is not set/empty/matches about:blank. It is a full navigation, so it would update the URL, and create a new document, etc, but it happens synchronously, and we still treat it as the "initial empty document" after.

So for about:blank it is equivalent to not navigating at all and just firing the load event, like in this PR.
Both about:blank#foo and about:blank?foo also goes through the synchronous navigation path, updating the URL but leaving the "initial empty document status" true.

  • For about:blank#foo I guess it's fine to just make it fire a navigation in the spec too, since it will be a same-document navigation.
  • For about:blank?foo though, hmm, I wonder if it makes more sense to just update the URL from here, or trigger a full navigation but... indicate that it's synchronous and keeps the "initial about:blank status"? Or maybe if that's awkward maybe we can look into changing Chrome's behavior to only happen on just about:blank exactly (and empty/invalid src) [or at least filing a bug for it :)] as I suspect changing the behavior of about:blank?foo is not as risky as changing the behavior of those other cases (not really sure tho)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, right, this makes sense given your original #6863!

Do you think such a "synchronous navigation" would be the same as running the URL and history update steps (with isPush set to false, i.e. replacement)? I think that would be equivalent to updating both the document's URL and the current session history entry's URL to about:blank?foo or about:blank#foo. (But, using the shared primitive of "URL and history update steps" makes me feel better than doing that directly.)

That might be the easiest way to spec something that matches 2/3 browsers without introducing new concepts.

Copy link
Member

@rakina rakina Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think such a "synchronous navigation" would be the same as running the URL and history update steps (with isPush set to false, i.e. replacement)?

Ooh, right, we already have that. I think that works! (and that gave me inspiration on what to migrate those cases to in the implementation, thanks :D)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I pushed a change that does the URL and history update steps for the "matches about:blank" branch. I'd love to get another review from @mystor, and @rakina and @annevk should feel free to take another look as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping @mystor, would you be able to take another look?

then return.</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved

<li><p>Let <var>resource</var> be a new <span data-x="concept-request">request</span> whose <span
data-x="concept-request-url">URL</span> is <var>url</var> and whose <span
Expand Down Expand Up @@ -80816,25 +80810,24 @@ dictionary <dfn dictionary>WindowPostMessageOptions</dfn> : <span>PostMessageOpt
tab.</p>
</li>

<li><p>Let <var>new</var> be true if <var>windowType</var> is either "<code data-x="">new and
unrestricted</code>" or "<code data-x="">new with no opener</code>", and false otherwise.</p></li>

<li><p>If <var>target browsing context</var> is null, then return null.</p></li>

<li><p>If <var>new</var> is true, then <span>set up browsing context features</span> for
<var>target browsing context</var> given <var>tokenizedFeatures</var>. <ref
spec="CSSOMVIEW"></p></li>

<li><p>Let <var>urlRecord</var> be the <span>URL</span> "<code>about:blank</code>".</p></li>

<li>
<p>If <var>url</var> is not the empty string or <var>new</var> is true, then:
<p>If <var>windowType</var> is either "<code data-x="">new and unrestricted</code>" or "<code
data-x="">new with no opener</code>", then:</p>

<ol>
<li><p><span>Set up browsing context features</span> for <var>target browsing context</var>
given <var>tokenizedFeatures</var>. <ref spec="CSSOMVIEW"></p></li>

<li><p>Let <var>urlRecord</var> be the <span>URL record</span>
<code>about:blank</code>.</p></li>

<li><p>If <var>url</var> is not the empty string, then <span data-x="parse a url">parse</span>
<var>url</var> relative to the <span>entry settings object</span>, and set <var>urlRecord</var>
to the <span>resulting URL record</span>, if any. If the <span>parse a URL</span> algorithm
failed, then throw a <span>"<code>SyntaxError</code>"</span> <code>DOMException</code>.</p></li>
failed, then throw a <span>"<code>SyntaxError</code>"</span>
<code>DOMException</code>.</p></li>

<li><p>Let <var>request</var> be a new <span data-x="concept-request">request</span> whose
<span data-x="concept-request-url">URL</span> is <var>urlRecord</var>.</p></li>
Expand All @@ -80843,42 +80836,51 @@ dictionary <dfn dictionary>WindowPostMessageOptions</dfn> : <span>PostMessageOpt
data-x="concept-request-referrer">referrer</span> to "<code
data-x="">noreferrer</code>".</p></li>

<li><p>Let <var>window</var> be <var>target browsing context</var>'s <span>active
window</span>.</p></li>
<li><p>If <var>urlRecord</var> is not <code>about:blank</code>, then
<span>navigate</span><!--DONAV window.open()--> <var>target browsing context</var> to
<var>request</var>, with <var><span>exceptionsEnabled</span></var> set to true, <var
data-x="navigation-hh">historyHandling</var> set to "<code data-x="hh-replace">replace</code>",
and the <span>source browsing context</span> set to <var>source browsing context</var>.</p></li>
</ol>
</li>

<li><p>If <var>urlRecord</var> is "<code>about:blank</code>" and <var>new</var> is true, then
<span>queue a global task</span> on the <span>networking task source</span> given
<var>window</var> to <span data-x="concept-event-fire">fire an event</span> named <code
data-x="event-load">load</code> at <var>window</var>, with the <var>legacy target override
flag</var> set.</p>
<li>
<p>Otherwise:</p>

<ol>
<li>
<p>Otherwise:</p>
<p>If <var>url</var> is not the empty string, then:</p>

<ol>
<li><p>Let <var>historyHandling</var> be "<code data-x="hh-replace">replace</code>" if
<var>new</var> is true; otherwise "<code data-x="hh-default">default</code>".</p></li>
<li><p>Let <var>urlRecord</var> be the <span>URL record</span>
<code>about:blank</code>.</p></li>

<li><p><span data-x="parse a url">Parse</span> <var>url</var> relative to the <span>entry
settings object</span>, and set <var>urlRecord</var> to the <span>resulting URL
record</span>, if any. If the <span>parse a URL</span> algorithm failed, then throw a
<span>"<code>SyntaxError</code>"</span> <code>DOMException</code>.</p></li>

<li><p>Let <var>request</var> be a new <span data-x="concept-request">request</span> whose
<span data-x="concept-request-url">URL</span> is <var>urlRecord</var>.</p></li>

<li><p>If <var>noreferrer</var> is true, then set <var>request</var>'s <span
data-x="concept-request-referrer">referrer</span> to "<code
data-x="">noreferrer</code>".</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved

<li><p><span>Navigate</span><!--DONAV window.open()--> <var>target browsing context</var> to
<var>request</var>, with <var><span>exceptionsEnabled</span></var> set to true, <var
data-x="navigation-hh">historyHandling</var> set to <var>historyHandling</var>, and the
<var>request</var>, with <var><span>exceptionsEnabled</span></var> set to true and the
<span>source browsing context</span> set to <var>source browsing context</var>.</p></li>
</ol>
</li>

<li><p>If <var>noopener</var> is false, then set <var>target browsing context</var>'s
<span>opener browsing context</span> to <var>source browsing context</var>.</p></li>
</ol>
</li>

<li><p>If <var>noopener</var> is true or <var>windowType</var> is "<code
data-x="">new with no opener</code>", then return null.</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved

<li>
<p>Otherwise, if <var>new</var> is false, set <var>target browsing context</var>'s <span>opener
browsing context</span> to <var>source browsing context</var>.</p>

<p class="note">If <var>new</var> is true this is done as part of <span>creating a new auxiliary
browsing context</span>.</p>
</li>

<li><p>Return <var>target browsing context</var>'s <code>WindowProxy</code> object.</p></li>
</ol>

Expand Down Expand Up @@ -119020,7 +119022,7 @@ interface <dfn interface>HTMLFrameSetElement</dfn> : <span>HTMLElement</span> {
<li><p>If <var>element</var> has a <code undefined data-x="attr-frame-src">src</code> attribute
specified, or <var>initialInsertion</var> is false, then run the <span>shared attribute
processing steps for <code>iframe</code> and <code>frame</code> elements</span> given
<var>element</var>.</p></li>
<var>element</var> and <var>initialInsertion</var>.</p></li>
</ol>

<p>The <code>frame</code> element <span>potentially delays the load event</span>.</p>
Expand Down