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

Always replace the initial about:blank Document #6595

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 19, 2021

Previously, the caller of the navigate algorithm would (usually) check for the browsing context still being on the initial about:blank Document, and if so, switch its history handling to "replace". However, a few call sites missed this: e.g., following hyperlinks or window.open() when they were targeted at an existing browsing context.

This change instead centralizes the the conversion of "default" navigations into "replace" navigations for the initial about:blank Document.

This also fixes a minor bug where the conversion of "default" into "replace" navigations for same-URL fragment navigations was not taking place correctly, due to the check being located after the fragment navigation step.

Closes #6491.

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


/browsers.html ( diff )
/browsing-the-web.html ( diff )
/form-control-infrastructure.html ( diff )
/history.html ( diff )
/iframe-embed-object.html ( diff )
/links.html ( diff )
/window-object.html ( diff )

@annevk
Copy link
Member

annevk commented Apr 20, 2021

I've asked @hsivonen to take a look. Editorially, it seems we now ignore windowType in <a>/<area> and <form> navigations so we probably want to rename that variable there or use "the first return value" as Fetch does. Looks good otherwise.

@domenic
Copy link
Member Author

domenic commented Jun 22, 2021

This should be reassed after #6714 makes it in...

@domenic domenic force-pushed the always-replace-initial-blank branch from 7537d5d to 19f0fb1 Compare June 23, 2021 21:22
source Outdated Show resolved Hide resolved
Previously, the caller of the navigate algorithm would (usually) check for the browsing context still being on the initial about:blank Document, and if so, switch its history handling to "replace". However, a few call sites missed this: e.g., following hyperlinks or window.open() when they were targeted at an existing browsing context.

This change instead centralizes the the conversion of "default" navigations into "replace" navigations for the initial about:blank Document.
@domenic domenic force-pushed the always-replace-initial-blank branch from 8b0c261 to 3f18633 Compare August 10, 2021 17:59
@domenic
Copy link
Member Author

domenic commented Aug 10, 2021

OK, this is now rebased on top of #6869, which means we I got rid of the long note explaining non-initial about:blank stuff. Yay!

We should get web-platform-tests/wpt#29745 (i.e. the tests for #6869) merged. Then I think this should be relatively ready. However it looks like per the tests for this PR, i.e. web-platform-tests/wpt#28541, no browser implements this right now. My understanding from @rakina is that Chrome wants to do so, and Rakina is actively working to make Chrome pass all those tests. Is that correct?

@domenic
Copy link
Member Author

domenic commented Aug 10, 2021

I guess the tests in web-platform-tests/wpt#28541 probably need updating, post-#6869. That might improve their pass rate.

@rakina
Copy link
Member

rakina commented Aug 11, 2021

I've updated the tests in web-platform-tests/wpt#28541 now that all iframe variants will stay in the initial empty document, and the iframe cases now looks good except for the pushState/replaceState case due to differences in security checks #6836

My understanding from @rakina is that Chrome wants to do so, and Rakina is actively working to make Chrome pass all those tests. Is that correct?

For the iframe cases, yes. Although maybe we shouldn't block on #6836. For the main frame case, it's still blocked on ongoing discussions for the refactoring effort for now, hopefully we'll reach a conclusion that's easy to spec, somewhat compatible with other browsers, and not break existing sites :)

@domenic
Copy link
Member Author

domenic commented Aug 11, 2021

OK, great. Let's put this on hold until those discussions conclude as to what the best path forward is for the window open cases.

I think we should have some tentative resolution on #6836 before finalizing this, but we don't have to have it fixed in all browsers.

@domenic
Copy link
Member Author

domenic commented Nov 12, 2021

OK, so per #6491 (comment) I think it's time to resurrect this, as Chrome has figured out the last remaining blockers and is proceeding with implementing this.

Contrary to some of my earlier claims, per the tests Firefox and Safari do not always replace the initial about:blank document today. (They pass 46/88 and 65/88 of the tests, respectively.) So this would be a requested behavior change for them too. @annevk @hsivonen, does this seem like a reasonable path? I'm pretty excited about it, as it cleans up a tricky area of poor interop, by going with a very simple and elegant solution.

@hsivonen
Copy link
Member

Sorry about deferring the review for so long; this issue turned out to be about something substantially different from what I expected it to be about.

I don't have an informed opinion on how the history should behave here. Perhaps @smaug---- does.

(How history behaves at least used to be incidentally sensitive to a large number of Gecko-only tests, so having WPTs that test a specific proven-Web-compatible behavior would be great for the purpose of sorting out that incidental test expectations outside WPT. My problem when I looked at this a decade ago was that I didn't know what was incidental and what was essential about the Gecko-only tests.)

My views about about:blank can be summarized as follows:

  1. When a browsing context is created, the initial about:blank document should materialize into it synchronously and not by parsing.
  2. If the browsing context doesn't have another URL to navigate to, events that would normally fire as part of the parse should fire on the initial about:blank document to the extent feasible and to the extent required by Web compat but not synchronously with the browsing context creation but from another task queued during the browsing context creation.
  3. In particular, if the browsing context doesn't immediately start navigating away from the initial about:blank, the about:blank document that resides in the browsing context when load for it fires should be the one created in point 1. (Gecko can currently replace it with a different about:blank document, which seems to be a compat problem.)

@rakina
Copy link
Member

rakina commented Dec 8, 2021

Thanks and sorry for the late reply also!
@hsivonen I think your points is consistent with how the initial empty document is specced right now - we removed the non-initial-empty-document about:blank that is committed immediately after the initial empty document from the spec, see with #6863.

@domenic:

Contrary to some of my earlier claims, per the tests Firefox and Safari do not always replace the initial about:blank document today. (They pass 46/88 and 65/88 of the tests, respectively.)
I think most of the failing tests with Firefox is because it doesn't handle fragment/same-document navigations in about:blank, and on top of that in Chrome/Safari/Firefox there are a bunch of tests that timeout due to window.open(). Maybe I should just remove the window.open tests, hmm..

@domenic
Copy link
Member Author

domenic commented Dec 13, 2021

and on top of that in Chrome/Safari/Firefox there are a bunch of tests that timeout due to window.open(). Maybe I should just remove the window.open tests, hmm..

That seems kind of bad, as that's important coverage... do you have any more insight into why they're timing out? Is it a wpt.fyi-specific problem?

@rakina
Copy link
Member

rakina commented Dec 14, 2021

and on top of that in Chrome/Safari/Firefox there are a bunch of tests that timeout due to window.open(). Maybe I should just remove the window.open tests, hmm..

That seems kind of bad, as that's important coverage... do you have any more insight into why they're timing out? Is it a wpt.fyi-specific problem?

Oh, sorry - what I said was kinda ambiguous. What I meant was not the tests that opens window.open(url) in new tab - but instead using window.open(url, "_self") to navigate an existing frame/tab (we also try to navigate with setting location.href, etc). Navigating with window.open(url, "_self") on an opened window times out in Firefox and Safari somehow, but not Chrome, and also not when navigating on iframes somehow. Maybe referring to _self but calling it from the opener window confuses things? I don't really know what's going on there.

With removing the window.open(url, "_self") navigation tests, the results looks better.
Chrome passes 74/85 (current Dev, soon it will pass almost all)

Firefox passes 49/85. The failing tests are:

Safari passes 69/85. The failing tests are:

So Safari's behavior is pretty much similar to Chrome except with some cases with specific navigation methods, while Firefox failures looks like things that should be fixed anyways?

@domenic
Copy link
Member Author

domenic commented Dec 14, 2021

It still seems like the window.open(url, "_self") cases give good coverage, for a specific and somewhat-strange method of navigation. So I think we should keep them, and just acknowledge that Firefox and Safari have bugs in those areas, if they're not getting the same results as location.href setting does.

But yeah, removing those (since it's probably a separate bug) was helpful for the analysis of how close everyone is to the spec. So thanks for doing that. IMO we should proceed with this!

@annevk, care to give us the green checkmark?

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 asked @smaug---- to take a look as well, but I think it is okay if Chrome ships this.

There's a "the the" in the commit message you want to fix before landing.

@domenic domenic merged commit a3df3e9 into main Jan 12, 2022
@domenic domenic deleted the always-replace-initial-blank branch January 12, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Which navigations should add/replace new history entry after initial empty document?
4 participants