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

Which navigations should add/replace new history entry after initial empty document? #6491

Closed
rakina opened this issue Mar 15, 2021 · 33 comments · Fixed by #6595
Closed

Which navigations should add/replace new history entry after initial empty document? #6491

rakina opened this issue Mar 15, 2021 · 33 comments · Fixed by #6595
Labels
interop Implementations are not interoperable with each other topic: history topic: navigation

Comments

@rakina
Copy link
Member

rakina commented Mar 15, 2021

I recently tested out how navigations after initial empty document affect history in various browsers and found some interesting cases on how session history is updated after a navigation from an initial empty document. Interestingly, the behavior also differs depending on whether the initial empty document is on an iframe vs opened window, although they both go through "create a new browsing context" to create the initial empty document..

iframe loaded with unset src or about:blank

Repro at https://rakina.github.io/aboutblankiframe.html, https://rakina.github.io/unsetiframe.html

Navigate to Chrome Firefox Safari
about:blank replaces replaces appends
about:blank#foo replaces appends appends
title1.html (same-origin page) replaces appends replaces
example.com (cross-origin page) replaces appends replaces
about:blank#foo => title1.html replaces, replaces appends, appends appends, appends
about:blank#foo => example.com replaces, replaces appends, appends appends, appends

"replaces" means the history.length stays the same and we can't navigate back to the previous URL, while "appends" means history.length is increased by and 1 and we can navigate back.

Chrome seems to always replace the entry for the initial empty document after every navigation (even same-document/fragment navigations).

On Safari, after any navigation that appends (even to "about:blank#foo"), the "replace" behavior is gone. So if you go from initial empty document => about:blank#foo => example.com, you'll end up with 3 history entries.

Additionally, if you navigate to about:blank#foo then to example.com then go back in Firefox, the resulting about:blank#foo page will have a different origin than the main frame.

window.open to unspecified URL or about:blank

Repro at https://rakina.github.io/windowopenblank.html

Navigate to Chrome Firefox Safari
initial state history.length == 0 history.length == 0 history.length == 1
about:blank replaces, length++ replaces, length++ replaces, length++
about:blank#foo replaces, length++ replaces, length++ replaces, length++
title1.html (same-origin page) replaces, length++ replaces, length++ replaces, length unchanged
example.com (cross-origin page) replaces, length++ replaces, length++ replaces, length unchanged
about:blank#foo => about:blank => go back about:blank, same-origin with opener about:blank, different-origin with opener about:blank, different-origin with opener
example.com => about:blank length == 2 length == 2 length == 2

The replacement behavior seems to be quite consistent in this case which is nice.

In Chrome & Firefox, the opened window starts with history.length of 0, while in Safari it starts with history.length of 1. Chrome & Firefox generally behave the same way with history.length here, and Safari mostly follows too (just starts with a different a value), though there are some weird cases (then again, history.length is generally unreliable so I guess this doesn't really matter?)

Again, there's some interesting cases with origin. I have no idea how the origin is calculated, just noting some interesting findings.

My questions:

  • Do we have special cases in the spec for navigations after the initial empty document? (I tried looking around the 'navigate' algorithm and browsing context creation, but I don't think I see any)
  • The spec implies a Window's associated Document can change if it was an initial empty document, but I can't find anything that changes the associated document. Am I missing something?
  • Are we supposed to keep the opener's origin when navigating away then back to initial empty documents?

My suggestions:

  • The window.open replacement behavior seems to be quite consistent, so it probably makes sense to change the iframe behavior to follow it? This means Safari & Firefox should change their behavior to follow Chrome.
  • For history.length in the window.open case, perhaps it makes sense to start with 1 since we do add the initial empty document to the session history.

Would love to hear people's thoughts. cc @domenic @jakearchibald @csreis @annevk @smaug---- @cdumez

Apologies if this is already discussed somewhere else, I searched for existing related issues and they don't seem to talk about this case specifically and also might be a bit outdated (#546, #490).

@jakearchibald
Copy link
Contributor

  • Do we have special cases in the spec for navigations after the initial empty document? (I tried looking around the 'navigate' algorithm and browsing context creation, but I don't think I see any)

Not that I've seen

I've been trying to find this for a while too! Looks like it's step 5 https://html.spec.whatwg.org/multipage/browsing-the-web.html#initialise-the-document-object

  • Are we supposed to keep the opener's origin when navigating away then back to initial empty documents?

As a developer, that's the behaviour that would make most sense to me.

  • The window.open replacement behavior seems to be quite consistent, so it probably makes sense to change the iframe behavior to follow it? This means Safari & Firefox should change their behavior to follow Chrome.

I agree, then it isn't down to timing. If you append <iframe src="https://example.com">, all browsers add an iframe with about:blank then replacement-navigate it to example.com. It seems to make sense to make this always-replace.

It feels like the issue discussed in #1191 goes away with the always-replace-initial-nav behaviour. Maybe that means we can all fix the bug where child contexts don't navigate if their parent is also navigated #5767 (comment).

That makes the most sense to me. I was really surprised to hear it starts at 0. Just to confirm: You're saying it should start at 1, and then remain 1 after navigation (since the navigation is "replace").

@domenic
Copy link
Member

domenic commented Mar 15, 2021

Do we have special cases in the spec for navigations after the initial empty document? (I tried looking around the 'navigate' algorithm and browsing context creation, but I don't think I see any)

I'm not sure this is what you mean, but historyHandling gets set to "replace" in a few cases:

The spec implies a Window's associated Document can change if it was an initial empty document, but I can't find anything that changes the associated document. Am I missing something?

Right, as Jake pointed out, this is due to https://html.spec.whatwg.org/#initialise-the-document-object step 5/6. In the initial about:blank case, we "do nothing" (step 5). In all other cases, we create a new window.

However, actually changing the "associated Document" seems to be broken... at least, I can't find where that happens. It seems like it should happen immediately. I wonder if we broke this recently... /cc @domfarolino

Are we supposed to keep the opener's origin when navigating away then back to initial empty documents?

This makes sense to me, but I suspect this was never really considered. This is the case you're triggering via about:blank#foo, right? I suspect people just didn't think of that.

The window.open replacement behavior seems to be quite consistent, so it probably makes sense to change the iframe behavior to follow it? This means Safari & Firefox should change their behavior to follow Chrome.

+1. Also, always doing replace matches the spec better I believe, and seems simpler to understand.

For history.length in the window.open case, perhaps it makes sense to start with 1 since we do add the initial empty document to the session history.

My understanding is that web developers sometimes use history.length > 0 to test if history.back() will do something useful. Anything that preserves that connection works for me.

Apologies if this is already discussed somewhere else, I searched for existing related issues and they don't seem to talk about this case specifically and also might be a bit outdated (#546, #490).

Do you think this issue covers all the issues discussed in those, and we can close them and roll them into here?

@domenic domenic added interop Implementations are not interoperable with each other topic: history topic: navigation labels Mar 15, 2021
@domenic
Copy link
Member

domenic commented Mar 15, 2021

However, actually changing the "associated Document" seems to be broken... at least, I can't find where that happens. It seems like it should happen immediately. I wonder if we broke this recently... /cc @domfarolino

I think it actually happens via history traversal, some steps later. E.g. if you look at the page load processing model for HTML files, it first creates the document, and then "before any script runs", it calls "update the session history with the new page", which will call "traverse the history", which will eventually call "set the active document".

I'm going to add a note to make this clearer since it's so easy to miss.

domenic added a commit that referenced this issue Mar 15, 2021
This should help address some of the questions that came up in #6491.
@Yay295
Copy link
Contributor

Yay295 commented Mar 15, 2021

My understanding is that web developers sometimes use history.length > 0 to test if history.back() will do something useful.

This seems like a broken test to me. Without knowing anything about how browser history works, I wouldn't expect history.back() to work unless history.length > 1. It seems like testing against 0 would only work in Safari anyways. Did you mean history.length > 1 instead?

@domenic
Copy link
Member

domenic commented Mar 16, 2021

Yes, I apologize for the typo.

@rakina
Copy link
Member Author

rakina commented Mar 16, 2021

I'm not sure this is what you mean, but historyHandling gets set to "replace" in a few cases:

https://html.spec.whatwg.org/#concept-form-submit step 20 (form submit)
https://html.spec.whatwg.org/#window-open-steps step 14.6.1 (window.open)
https://html.spec.whatwg.org/#initialise-the-document-object step (iframes... maybe also overlaps window.open? Hmm.)

Ah thanks, looking at the setters for "replace" I also found some other cases:

However some callers to navigate does not do the "replace", which sounds bad:

Overall having the caller to "navigate" set the "replace" bit for this case seems error-prone, and we might forget if we add more callers. Do you think it makes sense to actually make the "replace on every navigation when the document is the initial empty document" step part of "navigate"?

I think it actually happens via history traversal, some steps later. E.g. if you look at the page load processing model for HTML files, it first creates the document, and then "before any script runs", it calls "update the session history with the new page", which will call "traverse the history", which will eventually call "set the active document".

I'm going to add a note to make this clearer since it's so easy to miss.

Awesome, thanks!

Are we supposed to keep the opener's origin when navigating away then back to initial empty documents?

As a developer, that's the behaviour that would make most sense to me.

This makes sense to me, but I suspect this was never really considered. This is the case you're triggering via > about:blank#foo, right? I suspect people just didn't think of that.

Yeah, it seems like Safari & Firefox doesn't treat about:blank in fragments as about:blank (at least in some cases). I think they should (like Chrome), and I think this aligns with the fetch spec for about:blank (it checks just the path)

For history.length in the window.open case, perhaps it makes sense to start with 1 since we do add the initial empty document to the session history.

That makes the most sense to me. I was really surprised to hear it starts at 0. Just to confirm: You're saying it should start at 1, and then remain 1 after navigation (since the navigation is "replace").

My understanding is that web developers sometimes use history.length > 1 to test if history.back() will do something useful. Anything that preserves that connection works for me.

Yeah, I'm proposing we always start with 1, and if we do "replace" (which in my proposal is always the case) it will stay at 1. After that if we're not on the initial empty document anymore, we should update it accordingly (increase on append, stay the same on replace/reload).

Apologies if this is already discussed somewhere else, I searched for existing related issues and they don't seem to talk about this case specifically and also might be a bit outdated (#546, #490).

Do you think this issue covers all the issues discussed in those, and we can close them and roll them into here?

I think we cover #546 in this issue - "The iframe keeps its content" should be the behavior for case 3 & 4 there if we go with the "always replace" proposal.

I'm not sure about #490 - it seems like it's referencing spec text that no longer exists. I think @ArthurSonzogni looked into the synchronous about:blank navigation a while ago (see doc) and found that Firefox doesn't support it while Chrome does. Ideally we'd like to follow Firefox, but looks like there are some dependencies to that behavior in Chrome at least...

domenic added a commit that referenced this issue Mar 16, 2021
This should help address some of the questions that came up in #6491.
@domenic
Copy link
Member

domenic commented Mar 16, 2021

Overall having the caller to "navigate" set the "replace" bit for this case seems error-prone, and we might forget if we add more callers. Do you think it makes sense to actually make the "replace on every navigation when the document is the initial empty document" step part of "navigate"?

Hmm, I see, good catch. I guess the current spec's philosophy is that the "initial navigation" should be replace, i.e. the navigation to foo if you're doing <iframe src="foo"> or window.open("foo"). Whereas you're advocating for, any navigation that takes you from the initial about:blank should be a replace.

I suspect the latter is more correct, and in that case, indeed a spec change like you suggest would be good. But of course we should double-check with web platform tests, if there aren't any already.

BTW, we should try to revive #4691 so we can have a clearer spec idea of what the initial about:blank Document actually is.

@annevk
Copy link
Member

annevk commented Mar 17, 2021

I'm pretty sure you are influencing the results in Firefox by manipulating the contents of the initial about:blank document. bz explained this to me at some point and the general idea is that we attempt to determine if there was a document of sorts there as the user might wish to navigate back to it. (I think this is discussed in some issue against this repository as well.) I personally think it would be fine to remove this behavior. I'll see if others agree.

@rakina
Copy link
Member Author

rakina commented Mar 17, 2021

Hmm, I see, good catch. I guess the current spec's philosophy is that the "initial navigation" should be replace, i.e. the navigation to foo if you're doing <iframe src="foo"> or window.open("foo"). Whereas you're advocating for, any navigation that takes you from the initial about:blank should be a replace.

Yeah I guess so, but the spec is inconsistent currently if it is trying to do that (e.g. "location-object navigate" always replaces). I think having WPT would be good once we have some agreement here. Also it sounds like we need to test various methods of navigation, as @jakearchibald and I found some interesting differences in Safari when navigating with src vs link yesterday :(

I'm pretty sure you are influencing the results in Firefox by manipulating the contents of the initial about:blank document. bz explained this to me at some point and the general idea is that we attempt to determine if there was a document of sorts there as the user might wish to navigate back to it. (I think this is discussed in some issue against this repository as well.) I personally think it would be fine to remove this behavior. I'll see if others agree.

Ah thanks. I've updated http://rakina.github.io/aboutblankiframe.html to not modify the document, and also made a version with src not set at all (instead of set to about:blank) at http://rakina.github.io/unsetiframe.html. But they both still behave the same way as the original post. I wonder if I'm doing anything wrong, hmm. I navigate the iframes by setting the src attributes, not sure if that matters.

Overall I think not replacing if it's not just a "placeholder initial empty document" sounds good and probably what web devs would want, but I wonder if there's an easy way to actually track that? I saw #546 (comment) in the other thread, which covers document.open(), but not other ways of updating the DOM.

If it's not feasible to spec that, maybe checking history entry (instead of checking document) will be good enough? document.open() and other same-document navigations would create new entries (replacing the initial entry), so navigations after those would append instead of replace.

@annevk
Copy link
Member

annevk commented Mar 17, 2021

Thank you @rakina for finding that (and also for writing up this issue). Re-reading that what seems to happen is that if src isn't set, Firefox will navigate to about:blank upon creation of the <iframe>, which ends up replacing the initial about:blank with about:blank. (That navigation can be canceled with document.open() or another navigation attempt, e.g., in a <script> element following the <iframe>. Navigations that happen in that timeframe seem to result in replacement. And perhaps the motivation there was similar, that if someone didn't navigate about:blank straight away, it was probably significant in some way.)

I'm not sure how attached we are to any of this at this point, though changes in this area are always risky.

@ArthurSonzogni
Copy link
Member

ArthurSonzogni commented Mar 18, 2021

Yes. When src in <iframe> or url in window.open(url) are not set (or are considered empty), we consider the src to be about:blank and a navigation happens. This is the case in both Chrome and Firefox.

In the past Chrome used to support many navigations synchronously (about:blank, about:srcdoc, javascript: document commit, MHTML iframe, ...).
A synchronous navigation in a multi-process architecture is a pain. It is not possible to implement it correctly. Fortunately Firefox implements every navigations asynchronously. It has been a benediction for me. Today, we removed synchronous navigations, it remains only the initial re-navigation to about:blank when the |src| is considered empty. I hope we can get rid of that too, and converge with Firefox.

I just wanted to chime in to tell this about:blank navigation is not specific Chrome. The specificity is that Chrome handle it synchronously and Firefox asynchronously.

@jakearchibald
Copy link
Contributor

Are hash change navigations async in Firefox now?

@domenic
Copy link
Member

domenic commented Apr 13, 2021

@rakina I've been doing some work on this area (see e.g. #6566 and #6579) and I'd love to close out this bug with "replace everywhere" behavior. Would you be able to convert the cases you've tested into web platform tests, that expect always-replace and... something appropriate for history.length? That seems like the hardest part; hopefully the spec change is just some refactoring to centralize switching historyHandling to replace.

@rakina
Copy link
Member Author

rakina commented Apr 15, 2021

Thanks @domenic for working on clarifying the spec! I think I can help write WPTs in the next week or so (for this and #6213). I've never written a navigation WPT before so I think I'll try to follow web-platform-tests/wpt#28480 and also ask you questions :)

FYI the behavior in Chrome for the window.open() case is being fixed a little in https://crrev.com/c/2794069, but I think we have no way to differentiate "initial empty document" and "second initial empty document" right now :/

From briefly reading #6566, I think setting the src or specifying the URL when doing window.open() (event to "about:blank" or a javascript URL) will change the document to no longer be the initial about:blank document - is that correct?

@domenic
Copy link
Member

domenic commented Apr 15, 2021

I've never written a navigation WPT before so I think I'll try to follow web-platform-tests/wpt#28480 and also ask you questions :)

Sounds good! Note that I did get somewhat unexpected results for those tests so it's possible I did them wrong. Feel free to go a different route if you have better ideas.

FYI the behavior in Chrome for the window.open() case is being fixed a little in https://crrev.com/c/2794069

Looks great!

I think we have no way to differentiate "initial empty document" and "second initial empty document" right now :/

I'm not quite sure I understand in which case there'd be a second initial empty document (although I appreciate the link ^_^). Is this the case you're referring to below where you navigate to a second (non-"initial") about:blank?

From briefly reading #6566, I think setting the src or specifying the URL when doing window.open() (event to "about:blank" or a javascript URL) will change the document to no longer be the initial about:blank document - is that correct?

Yeah, I think that's right, at least for the spec as-is. Since a new Document gets created, and initial about:blank-ness is tied to the Document.

Note that (also per the spec today) navigating to about:blank#foo would not remove the initial about:blank-ness since it bails out of the navigate algorithm early while staying on the same document.

@rakina
Copy link
Member Author

rakina commented Apr 16, 2021

I'm not quite sure I understand in which case there'd be a second initial empty document (although I appreciate the link ^_^). Is this the case you're referring to below where you navigate to a second (non-"initial") about:blank?

Yeah I was talking about the case @annevk referred to in #6491 (comment), where setting src to something will differ than not setting src at all in Firefox (same as the currently specified behavior?).

In Chrome, we treat the document resulting from "src is set to about:blank" and "src is not set" as the same, as we navigate to the second about:blank synchronously (see code and doc). We treat them both as the initial empty document because we can't tell the difference :( (But I think this @zetafunction is working to remove this case for #3267?)

Anyways, I have draft WPTs for the iframe case in web-platform-tests/wpt#28541, will work on the window.open() tests next week. I also tried to test the "src set to about:blank" case but I don't really know how to wait for the load of the non-initial empty document about:blank. I think iframe.onload() will fire for the initial empty document load? (+ it's hard to cater for both Chrome and Firefox's timing - Chrome's second about:blank will commit synchronously while Firefox's will be asynchronous). Please let me know if you have ideas! Or maybe we should just not test the "src set to about:blank" case for now? Hmm..

@domenic
Copy link
Member

domenic commented Apr 16, 2021

Eeek, I see, what a scary mess :).

In general, my instinct is to separate the question of window object reuse (#3267) from the question replace vs. push tests (this issue), if we can. It might end up being the case that we only perform window reuse in some cases, but we do replace in more cases.

To me it looks like the current spec has the same behavior for <iframe> <iframe src="">, <iframe src="https://invalid:url">, and <iframe src="about:blank">. And I believe that behavior is that they all load a second, non-initial about:blank, doing window object reuse and replacement for that load.

Then, if you load a third document (which I suppose must be something besides about:blank? remembering that fragment navigations are same-document) it'll either do replace or push, depending on whether the second non-initial about:blank has finished loading. And it will never do window object reuse.

I like how the current spec treats all those different cases the same.

I also tried to test the "src set to about:blank" case but I don't really know how to wait for the load of the non-initial empty document about:blank. I think iframe.onload() will fire for the initial empty document load? (+ it's hard to cater for both Chrome and Firefox's timing - Chrome's second about:blank will commit synchronously while Firefox's will be asynchronous). Please let me know if you have ideas! Or maybe we should just not test the "src set to about:blank" case for now? Hmm..

I'm hopeful that we can test src set to about:blank, and in particular test that it's the same as empty src or missing src.

I agree the timing seems quite tricky. It's hard to tell which load events will fire when. Maybe you can poll for when iframe.contentDocument changes identity, using step_wait? Although I guess that might not work in Chrome if it commits the second about:blank synchronously; you'd never be able to observe the "before" value. Hmm.

@rakina
Copy link
Member Author

rakina commented Apr 19, 2021

OK I've written some more tests in https://crrev.com/c/2831564 / web-platform-tests/wpt#28541. I used a load event listener + setTimeout(100) to hopefully make it work in Firefox, but I actually have only tested it in Chrome. I don't know if there's an easy way to run WPTs on other browsers, any suggestions than just porting it into a non-WPT page?

So here's a table of my current understanding of what counts as "initial empty document" vs not. The window.open() case seems to be straightforward because navigate won't run for unset/about:blank URL, but the iframe case is quite complicated because of the shared attribute processing steps. I think the step won't run on initial insertion when src is unset (so <iframe> will stay in the initial empty document). Let me know if I got things wrong.

Chrome's behavior for iframes is probably mostly based on this check, which uses the "has committed real load" concept instead of "initial empty document". I think former stays true as long as the loaded URL in the iframe is about:blank (even after many navigations). Also we won't replace after the document is explicitly open()ed... I think @natechapin added it here, but I don't really understand the case :(

Browsing context creation case Current document is initial empty document? Navigation on current document will replace?
window.open() Yes Yes
window.open("about:blank") Yes Yes (in Firefox & Safari, Chrome is currently no but it's being fixed)
window.open(204-url) (won't create new doc) Yes, we stayed in the initial empty doc Yes in Chrome, ??? in Firefox and Safari
iframe with unset src Yes? Because this step will not run? Yes in Chrome and Safari, No in Firefox
iframe with src=about:blank, before new document loaded Yes, we're still on initial empty doc N/A in Chrome, ??? in Firefox and Safari
iframe with src=about:blank, after new document loaded No, replaced with new about:blank Yes in Chrome, No in Firefox and Safari
iframe with src=204-url (won't create new doc) Yes, we stayed in the initial empty doc Yes in Chrome, ??? in Firefox and Safari

@domenic
Copy link
Member

domenic commented Apr 19, 2021

OK I've written some more tests in https://crrev.com/c/2831564 / web-platform-tests/wpt#28541

These look really great!

I don't know if there's an easy way to run WPTs on other browsers, any suggestions than just porting it into a non-WPT page?

You can start up the server (e.g. using the first line in https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_tests_in_content_shell.md#Running-WPT-Tests-in-Content-Shell) and then launch Firefox. You can also check out the results under "Show all checks" in web-platform-tests/wpt#28541 which gives us the following links:

I think the step won't run on initial insertion when src is unset (so <iframe> will stay in the initial empty document). Let me know if I got things wrong.

Oh, you're right, I missed that. That's an annoying special-case.

Do you think there's any chance of unifying these cases, perhaps by making all of them perform no navigation on initial insertion? I.e. change the last step of "process the iframe attributes" to

Otherwise, run the shared attribute processing steps for iframe and frame elements given element and initialInsertion.

and add a step between the current steps 1 and 2 of "shared attribute processing steps for iframe and frame elements" that says something like

If initialInsertion is true and URL is "about:blank", then return.

?

Chrome's behavior for iframes is probably mostly based on this check, which uses the "has committed real load" concept instead of "initial empty document". I think former stays true as long as the loaded URL in the iframe is about:blank (even after many navigations).

That's unfortunate, hmm.

Also we won't replace after the document is explicitly open()ed

This is intentional as all browsers seem to behave this way; see b31e145 and the linked issues/PRs.

@domenic
Copy link
Member

domenic commented Apr 19, 2021

Do you think there's any chance of unifying these cases, perhaps by...

I guess we would also have to fire a load event, like window.open() does. Hmm. Maybe not worth it, at least for now...

@rakina
Copy link
Member Author

rakina commented Apr 20, 2021

Thanks! I think I've covered enough cases in the WPTs now (Chrome, Firefox, Safari results). No browser passes all the tests, but they're all quite close :) Some things to note:

  • The iframe-src-aboutblank-wait-for-load.html test still asserts Chrome and Safari's behavior (will replace even after the non-initial-empty document about:blank loaded). Probably we should change it to follow Firefox instead
  • Testing fragment navigations in iframe works OK, but I don't think I'm able to make it work for window.open()-ed tab in the bots (but they run just fine locally with Chrome's layout test runner :/)
  • Navigating with window.open(url, "_self") on a window.open()-ed tab works in Chrome, but times out in Firefox and Safari (the test waits for the load even to fire to send a postMessage to the opener - it never arrived in Firefox & Safari, hmm.. maybe we should just remove it)

Should people just review web-platform-tests/wpt#28541 or should I make a new PR? Or maybe it's nicer to review this on Chromium Gerrit's UI?

@domenic
Copy link
Member

domenic commented Apr 20, 2021

The iframe-src-aboutblank-wait-for-load.html test still asserts Chrome and Safari's behavior (will replace even after the non-initial-empty document about:blank loaded). Probably we should change it to follow Firefox instead

Agreed, at least for now.

Testing fragment navigations in iframe works OK, but I don't think I'm able to make it work for window.open()-ed tab in the bots (but they run just fine locally with Chrome's layout test runner :/)

This is pretty mysterious :(. If we can't make it work, probably we should remove the test.

Navigating with window.open(url, "_self") on a window.open()-ed tab works in Chrome, but times out in Firefox and Safari (the test waits for the load even to fire to send a postMessage to the opener - it never arrived in Firefox & Safari, hmm.. maybe we should just remove it)

This test still seems valuable to keep.

Should people just review web-platform-tests/wpt#28541 or should I make a new PR? Or maybe it's nicer to review this on Chromium Gerrit's UI?

Probably we should review web-platform-tests/wpt#28541. Generally folks like @hsivonen and @annevk prefer to use the GitHub UI instead of the Chromium code review tool. It just means you'll commit any changes by going back and modifying Gerrit, which is a bit awkward but probably fine.

@annevk
Copy link
Member

annevk commented Apr 20, 2021

I don't necessarily mind looking at things in Gerrit, but I'm definitely more familiar with GitHub.

@domenic
Copy link
Member

domenic commented Apr 20, 2021

Looking at some of the failures on wpt.fyi (e.g. this one in Chrome) it seems like the wpt test runner is not coping well with window.open() tests of this sort in general. That might be worth raising with the infra team...

@rakina
Copy link
Member Author

rakina commented Apr 21, 2021

OK, I think I've mostly fixed the tests now (see Chrome, Firefox, Safari results). Some of the window.open() fragment tests still fail mysteriously in Chrome, but they run in Firefox (although they fail because Firefox didn't do replacement) and some runs but fails in Safari. PTAL at web-platform-tests/wpt#28541 :)

@domenic
Copy link
Member

domenic commented Apr 22, 2021

We seem really close now. At a high level, my big questions are:

On specific questions:

In web-platform-tests/wpt#28541 (comment) @rakina says

Firefox times out in most cases though, maybe they don't fire load for initial empty document loads? Or the load here is actually caused by Chrome and potentially Safari's non-initial empty document about:blank commit, hmm...

https://html.spec.whatwg.org/#window-open-steps says that the initial about:blank in window.open() should fire a load event, so I think if Firefox isn't firing some event here then it's doing something wrong...

Also in web-platform-tests/wpt#28541 (comment) @rakina says

Also, unfortunately it looks like the behavior for iframe vs window.open() on is different for navigations after fragment navigations. For iframes, even after fragment navigations, we will still do replacement. For window.open(), we won't do a replace after the initial empty document. Not sure which behavior is nicer here :/

Am I correct the scenario is the following one?

  1. Start on initial about:blank
  2. Navigate that same document to about:blank#foo
  3. Navigate to /common/blank.html

and for iframes, both (2) and (3) are a replace, but for window.open(), only (2) is a replace?

I think the iframe behavior makes the most sense, if I am understanding the scenario correctly. I hope we can unify them; do you think that'd be possible for Chrome?

In spec terms, #6595 proposes to replace if the browsing context is still on its initial about:blank Document, which is defined as

if browsingContext's session history's size is 1 and browsingContext's session history[0]'s document's is initial about:blank is true.

That definition gives the iframe behavior, since a replace fragment navigation does not change the size, and a fragment navigation does not change the document or its "is initial about:blank" boolean value.

@zetafunction
Copy link

zetafunction commented Apr 30, 2021

This is a very long thread, so I haven't really caught up. However, I've noticed one thing as I've read through.

For browsing context creation case:

iframe with unset src

The behavior of "Navigation on current document will replace?" is currently noted as:

Yes in Chrome and Safari, No in Firefox

But this does replace in Firefox, as far as I can tell. I tested with the following snippet in the inspector:

const myIframe = document.createElement('iframe');
myIframe.addEventListener(
    'load',
    () => {
      myIframe.contentWindow.somethingRandom = 'Hello world!';
      console.log(`Navigating iframe to ${location}`);
      myIframe.contentWindow.location = location;
      myIframe.addEventListener('load', () => { console.log(myIframe.contentWindow.somethingRandom); }, { once: true });
    },
    { once: true });
document.body.appendChild(myIframe);

This logs undefined, which is what I would expect if the Window object was replaced. Note that the behavior is different with this slightly rearranged snippet:

const myIframe = document.createElement('iframe');
document.body.appendChild(myIframe);
myIframe.contentWindow.somethingRandom = 'Hello world!';
console.log(`Navigating iframe to ${location}`);
myIframe.contentWindow.location = location;
myIframe.addEventListener('load', () => { console.log(myIframe.contentWindow.somethingRandom); }, { once: true });

This prints Hello world!.​ This hints that the navigation to about:blank when inserting an <iframe> with no src is asynchronous in Firefox.

Interestingly enough, the standard does seem to say that the shared attribute processing steps should be skipped in this case (see step 2 of https://html.spec.whatwg.org/#process-the-iframe-attributes). I will admit that when I first read this though, I got it backwards and thought that the steps would unconditionally run on initial insertion...

@domenic
Copy link
Member

domenic commented Jun 22, 2021

@rakina, ping on #6491 (comment) ?

@rakina
Copy link
Member Author

rakina commented Jun 24, 2021

Sorry for the 2 months delay! I think the spec and WPT PRs are describing what we want:

  • Initial empty document should create a new session history entry
    • This is not really tested by the WPTs, because Chrome and Safari starts at 0 for the window.open() case, and I'm not really sure how to test this for iframes.... I guess it should be tested for at least the window.open() case, but as a separate test (instead of expecting every test to start with 1 history entry, because that will make most of the tests fail in Chrome & Safari).
  • Every navigation (that's not already a history/reload navigation) from an initial empty document should replace the initial empty document's session history entry
    • Tested by all WPTs.
  • Same-document navigations should not change the "initial empty document"-ness.
    • Tested by all WPTs with fragment in its name, by navigating after a fragment navigation. This seems to be what the current behavior is in iframes, but not for opened windows. Chrome and Safari doesn't do replacement, while the fragment navigation failed(?) on Firefox (but it also does on iframes, so I guess Firefox just don't handle fragment navigations on the initial empty document nicely).
  • Cross-document navigations should remove the "initial empty document"-ness. This includes reloads, I think.
    • Tested by all WPTs by navigating to another URL, and it looks like everyone agrees here. No test for the reload case though (should I add one?)

The above are mostly consistent in all the browsers (Firefox & Chrome passes all tests for iframe-nosrc.html & iframe-src-204.html, while Safari passes 5/7 because it behaves differently on link clicks/form submissions vs other navigation cases).

The below cases are where most of the differences lie (although they're already part of the spec....).

  • window.open() to an unset URL, about:blank, an empty URL, or a URL that never commits will stay in the "initial empty document"

    • This is because navigate won't run for these cases.
    • Tested by window-open-* WPTs. Currently most of the tests (except where the URL is set to about:blank) times out in Firefox and Safari though. Would appreciate if somebody can look at the test and tell me if I'm doing something bad there (I waited for the load event)
  • iframes with src set to about:blank, an empty URL, or invalid URL will start an asynchronous navigation to a non-initial about:blank document.

    • This is because "shared attribute processing steps" will trigger a navigation to a non-initial about:blank that will replace the initial empty document.
    • Tested by iframe-src-aboutblank-* WPTs.
    • This is currently only correctly implemented in Firefox (as @zetafunction said - sorry, my table above is very out of date), where the non-initial about:blank document is committed asynchronously. Both Safari and Chrome seems to commit the non-initial about:blank document synchronously, but then proceeded to treat it the same way as the initial empty document. So they both fail the tests in iframe-src-aboutblank-wait-for-load.html which expect navigations to not do replacement.
  • iframes with unset src or a URL that never commits will stay in the "initial empty document"

    • This is because "process-the-iframe-attributes" is only called during the initial insertion, so "shared attribute processing steps" won't run and trigger the . Otherwise the iframe might be navigated to a non-initial empty document.
    • Tested by iframe-nosrc and iframe-src-204* WPTs. All browser seems to agree here, although under the hood at least Chrome's implementation is a bit wrong.

Link to Firefox, Chrome, and Safari results for easier comparison.

So, if the above points are not controversial, I think the next step is just polishing the spec PR, and the WPTs. I'll spend some more time to try to figure out why the window-open tests are timing out (or someone more proficient in WPTs than me can help here too if they want :D). I'm also currently trying to make Chrome's behavior follow the proposed behavior here (or at least, the ones that are already part of the spec).

Answering some questions:

Am I correct the scenario is the following one?

Start on initial about:blank
Navigate that same document to about:blank#foo
Navigate to /common/blank.html
and for iframes, both (2) and (3) are a replace, but for window.open(), only (2) is a replace?

I think the iframe behavior makes the most sense, if I am understanding the scenario correctly. I hope we can unify them; do you think that'd be possible for Chrome?

Yep this is the case, and yes the iframe behavior should be possible to implement for the window.open() behavior in Chrome too. Hope this is OK with others too?

https://html.spec.whatwg.org/#window-open-steps says that the initial about:blank in window.open() should fire a load event, so I think if Firefox isn't firing some event here then it's doing something wrong...

Thanks, thats helpful to know. I'll do some more investigations then...

@domenic
Copy link
Member

domenic commented Jun 24, 2021

Awesome, thank you so much for the detailed overview Rakina!

I guess it should be tested for at least the window.open() case, but as a separate test (instead of expecting every test to start with 1 history entry, because that will make most of the tests fail in Chrome & Safari).

I agree, that would be great to add. But it doesn't need to block resolving this issue and the corresponding PR/WPTs. Maybe you could file a tracking issue so that we don't forget to write such tests?

Regarding your comment in the spec PR, can you add tests for history.pushState() as well as fragment navigations? My suspicion is that that should not get converted into a replace, as at least in the spec it never goes through "navigate", but if it does in multiple browsers, then that'd be good new information that we should incorporate here.

Regarding the tests failing in Firefox, if it's related to a separate spec non-compliance with load events then you don't necessarily need to change the tests, since it's catching a real interop issue. But probably it'd be good to boil that down to a minimal case which could be written up as a separate isolated WPT and filed as a separate Firefox bug.

Once we have a diagnosis on why the tests are failing in Firefox, I think we can loop in Firefox engineers with a clear description of what the delta is between their current behavior and the proposed spec, and then we'll be on track to land everything!

@rakina
Copy link
Member Author

rakina commented Jul 7, 2021

I guess it should be tested for at least the window.open() case, but as a separate test (instead of expecting every test to start with 1 history entry, because that will make most of the tests fail in Chrome & Safari).

I agree, that would be great to add. But it doesn't need to block resolving this issue and the corresponding PR/WPTs. Maybe you could file a tracking issue so that we don't forget to write such tests?

I've added window-open-history-length.html for this now! But also noticed that actually checking for explicit history.length == 1 or 2 in the tests is actually better. Chrome (and I think Firefox) starts with 0 but the next navigation will increase the length to 1 (although I guess this still counts as a "replacement", since we still can't go back to the initial empty doc entry?). 😬

Regarding your comment in the spec PR, can you add tests for history.pushState() as well as fragment navigations? My suspicion is that that should not get converted into a replace, as at least in the spec it never goes through "navigate", but if it does in multiple browsers, then that'd be good new information that we should incorporate here.

OK I've added history.pushState and replaceState tests and looks like these are blocked by security checks, except for the iframe case in Safari somehow?
(Chrome's iframe & window.open(), Firefox's iframe & window.open(), Safari's window.open())
I tried about:blank#foo and also the main frame/opener's URL since the iframe/opened window will inherit their origin (CMIIW?) but it's still blocked, so it's probably another implementation bug.

Anyways, I think the tests are in OK enough shape after some reorganization, and I've removed some stuff, including the part that used load event, which I think isn't really needed now. I'll try to re-confirm the cases manually and summarize + tag people in the WPT thread this week!

@domenic
Copy link
Member

domenic commented Jul 7, 2021

I've added window-open-history-length.html for this now!

Looks great!

OK I've added history.pushState and replaceState tests and looks like these are blocked by security checks, except for the iframe case in Safari somehow?

Oh, wow, that is bizarre! I see no spec support for such an exception, and the actual exceptions thrown in Chrome and Firefox don't seem like anything I'd expect.

So IMO that's a separate set of browser bugs we should track. I'd push for just fixing them but if we find out that this behavior exists for a good reason then we can revisit.

It looks like you wrote the tests to expect a replace (not a push), and that passes in one case. Whereas if we made the tests match the current spec and expect a push, then the tests would pass in zero cases. So I guess I'll update #6595 to convert pushState to replaceState in such cases.

@rakina
Copy link
Member Author

rakina commented Nov 12, 2021

FYI we're implementing the "always replace initial empty document" behavior in Chrome (crrev.com/c/3237493), making us follow the behavior expected in the WPTs.

domenic added a commit that referenced this issue Jan 12, 2022
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 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.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
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 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 whatwg#6491.
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: history topic: navigation
Development

Successfully merging a pull request may close this issue.

7 participants