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

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

Merged
merged 8 commits into from
Aug 10, 2021

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 16, 2021

For iframes: previously, we did a confusing thing where we would navigate to a non-initial about:blank. 2/3 engines instead just fire a synchronous load event at the iframe element, with no navigation. This is a bit simpler, and matches the popup case a bit better (after the below modifications).

For popups: previously, we fired the load event (but not the pageshow event) when the popup stays on the initial about:blank. 2/3 engines instead fire no events in this case, and the remaining one only fires it in the explicit "about:blank" case but not in the empty string case.

In both cases, it wasn't quite clear what to do when navigating to something like about:blank#foo or about:blank?foo. The spec now makes it clear that such cases cause URL updates of the new browsing context but not full navigations. In particular, for now at least the the initial about:blank-ness of the Document is retained. (In browsers, it seems like it's always retained for replacement vs. push purposes, but is only sometimes retained for Window object reuse purposes. #3267 tracks sorting that out.)

This also refactors the window open steps so that they have two primary branches, depending on whether to create a new window or not. Previously the steps were sorta unified, but there were a lot of consultations of the "new" variable throughout, which made it hard to understand the differences between the cases.

Closes #6863.

(See WHATWG Working Mode: Changes for more details.)


/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/iframe-embed-object.html ( diff )
/obsolete.html ( diff )
/urls-and-fetching.html ( diff )
/window-object.html ( diff )

Previously, we did a confusing thing where we would navigate to a non-initial about:blank. 2/3 engines instead do something observably equivalent to not navigating at all; they just fire a load event. This is much simpler, and matches window.open().

Closes #6863.
@domenic domenic added topic: events interop Implementations are not interoperable with each other topic: browsing context labels Jul 16, 2021
@domenic domenic requested a review from rakina July 16, 2021 22:23
@annevk
Copy link
Member

annevk commented Jul 19, 2021

This also means you do not get a load or pageshow event on <iframe>.contentWindow. (Both fire in Firefox.)

cc @smaug---- @mystor

@domenic
Copy link
Member Author

domenic commented Jul 19, 2021

Did you check the PR text? We explicitly fire load. Good point that we should also fire pageshow!

@domenic
Copy link
Member Author

domenic commented Jul 19, 2021

Although hmm pageshow does not fire for about:blank in window.open() (https://html.spec.whatwg.org/#window-open-steps step 14.5). I guess we should test both window.open() and initial iframe insertion...

@annevk
Copy link
Member

annevk commented Jul 20, 2021

Oh, but that does not align with Chrome and Safari, right? They only fire load on <iframe> as far as I can tell per my demo.

@mystor
Copy link
Contributor

mystor commented Jul 20, 2021

cc @hsivonen who IIRC looked at this in 2011, and might have thoughts.

I think we've intermittently poked at this issue before. Like mentioned re: Chrome in #6863, I expect we have many tests and other internal code which depend on the existing navigation behaviour and its side-effects, so this would be a fairly significant project for us to change.

Although hmm pageshow does not fire for about:blank in window.open() (https://html.spec.whatwg.org/#window-open-steps step 14.5). I guess we should test both window.open() and initial iframe insertion...

That's probably a good idea to do. I might be misremembering, but IIRC in Gecko right now we'll stay on the initial about:blank if you call window.open(), but navigate if you call window.open("about:blank").

@domenic
Copy link
Member Author

domenic commented Jul 20, 2021

Oh, but that does not align with Chrome and Safari, right? They only fire load on <iframe> as far as I can tell per my demo.

Ah OK, if they only fire load then the current spec patch is correct as-is. (I.e., matches 2/3 browsers.) But we should be sure to include that in the tests.

this would be a fairly significant project for us to change.

In case it helps, I can tell you that this kind of thing is a fairly significant area of developer interop pain; we've had a lot of internal reports of frustrated Googlers working on large products which often try to create blank iframes/popups and modify their contents. I'm hopeful this can be part of a general effort to straighten out behavior differences across browsers in history and navigation :).

source Show resolved Hide resolved
@domenic domenic changed the title Do not navigate to about:blank on iframe insertion Change initial about:blank navigation behavior for iframes and popups Jul 21, 2021
@domenic
Copy link
Member Author

domenic commented Jul 21, 2021

I've updated the OP here with the new scope of the changes. I'm still hoping @rakina can make some time for tests :). (Edit: looks like she's OOO this week.)

@rakina
Copy link
Member

rakina commented Jul 22, 2021

Thanks all!
I made WPTs for the content-replacement and events bit. It verifies Chrome and Safari behaves the same way, and Firefox's window.open() and window.open('') (but not window.open('about:blank') behaves the same as Chrome and Safari.

From the blog post @mystor linked it looks like this behavior (Chrome & Safari synchronously commits the "initial empty document", Firefox replaces with a non-initial about:blank asynchronously) is the same as 10 years ago. The conclusion in the post seems to be consistent with this PR too: create the initial about:blank synchronously, don't replace it later if pointed to about:blank, and don't fire events synchronously.

I can definitely see changing this behavior in the implementation being non-trivial, but hopefully the web-exposed part won't be a problem:

  • I can't imagine a use case that depend on synchronously-added content to <iframe src='about:blank'>/window.open('about:blank') being replaced asynchronously with about:blank (CMIIW!)
  • The load and pageshow events are already not fired on Chrome and Safari, so hopefully not a lot of websites depend on this. The event handlers can be easily migrated to happen synchronously as well.

With this, I hope we can go forward with this PR :)

[I am OOO but felt like replying. Will be OOO again after this!]

@domenic
Copy link
Member Author

domenic commented Jul 22, 2021

Awesome, thanks so much Rakina, and especially for taking the time off from your OOO! Those tests look pretty comprehensive so I think this should be ready to merge after it gets some review. @annevk's review would be helpful especially after my last refactoring commit.

source Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I found a couple issues, not all of them novel though so up to you if you want to fix those here.


<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

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

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?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@rakina rakina left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @domenic and others :)

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I read it again and I still think the logic rewrite is correct, but it's tricky...

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Jul 28, 2021
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Aug 3, 2021
@domenic
Copy link
Member Author

domenic commented Aug 5, 2021

There have been a few changes since this was last reviewed, so if @annevk, @rakina, @mystor, or @smaug---- would be willing to take a look, that'd be much appreciated.

@domenic
Copy link
Member Author

domenic commented Aug 5, 2021

I'll also note that I've kept the OP updated with a more detailed list of what changes this ended up making, and the current status.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This still looks good to me, assuming I'm correct that the remaining Window-object reuse question is about subsequent navigation of this about:blank document. Obviously it would be great if the other tagged people could give their input as well as they noticed issues I hadn't.

source Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: browsing context topic: events
Development

Successfully merging this pull request may close these issues.

Proposal: stop triggering navigations to about:blank on iframe insertion
4 participants