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

Should window.navigation fire events fire on page load? #31

Closed
domenic opened this issue Feb 5, 2021 · 6 comments
Closed

Should window.navigation fire events fire on page load? #31

domenic opened this issue Feb 5, 2021 · 6 comments
Labels
change A proposed change to the API, not foundational, but deeper than the surface

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 5, 2021

On a cross-document navigation to a new page, we could fire one or more of:

  • window.navigation.currentEntry fires navigateto
  • window.navigation fires currententrychange

The timing would be debatable. After DOMContentLoaded? After pageshow? Should it differ for restores-from-bfcache vs. fresh network navigations?

Another data point is that popstate and hashchange do not fire on page load. So, this would be a departure from those. I'm not sure what direction that indicates, though: have people been frustrated that popstate doesn't fire on page load? Or would they be more frustrated if it did?

@matt-buland-sfdc
Copy link

IMO, I'd prefer to get a currententrychange on page load. The lack of popstate and hashchange on page load are kinda like gotchas -- the developer needs to know that's the case, and add a second event handler somewhere like load. A single event consolidates the mental model without any gotchas. Practically speaking, most apps use a routing framework, so that nuance is generally handled already. As a notable change, a routing framework may need to adapt to the rules of currententrychange, depending on what their handlers aim to do (e.g. if they render on currententrychange, then it could potentially be duplicate with connectedCallback or load).

@tbondwilkinson
Copy link
Contributor

tbondwilkinson commented Feb 11, 2021

Ugh there was a bug for a long time where safari would trigger the popstate on pageload. What a nightmare. But the nightmare wasn't that it fired, just that it was incompatible with other browsers.

I think that I too like getting navigateTo and currententrychange notifications for cross-page navigations.

This is especially relevant because in a world where we don't have bfache, loading HTML from network cache is actually pretty confusing. Imagine:

  1. Page loads /
  2. Page navigates to /#foo=bar and does additional rendering, specifically using the hash fragment to prevent a full page reload from the server.
  3. Page cross-navigates to some URL
  4. browser navigates back to /#foo=bar, loads the cached HTML page (no bfcache) and shows /

The application would PROBABLY like to re-do whatever additional rendering it did in 2, but it can't do that unless it knows the navigation took place. Having to add a pageshow handler for what is essentially just a back navigation feels weird - and in fact, you might want to KNOW that it was a back navigation from a cross-document page, which the pageshow event doesn't really tell you.

@domenic
Copy link
Collaborator Author

domenic commented Feb 17, 2021

OK, cool, I'm pretty sold then. The question is what the timing should be.

The connection to pageshow is interesting. pageshow (for non-bfcache navigations) fires quite late, at the same time as the load event (i.e. after all scripts and images and iframes have finished). Is that what people would actually want for this use case?

I guess in general there's a tradeoff with these events, where earlier lets your page start doing important stuff faster, but if it's too early then the script listening for the event might not be loaded. This would be solved by moving to promises (e.g. an appHistory.currentChanged promise which replaces itself every time appHistory.current does), but I'd like to get traction on whatwg/html#1936 first before porting that pattern over here...

@tbondwilkinson
Copy link
Contributor

That's interesting I never considered the wrinkle about pageshow triggering as late as the load event.

I do think that we should at least wait to fire these events until the scripts on the page have loaded. Otherwise, it's just so easy to miss these events and then people would just end up reading from current entry which is useful, but doesn't for instance tell you that this is in fact a back navigation to a previous page, unless you do hacks like put little tokens into history state so you know you've been to this page before (which, we currently do).

@domenic domenic added the change A proposed change to the API, not foundational, but deeper than the surface label Feb 17, 2021
@domenic domenic added this to the Might block v1 milestone Nov 4, 2021
@smaug----
Copy link

I would expect there to be some event and it should fire consistently after pageshow.

@domenic domenic changed the title Should appHistory fire events fire on page load? Should window.navigation fire events fire on page load? Mar 14, 2022
@domenic
Copy link
Collaborator Author

domenic commented Mar 17, 2022

@smaug----, @annevk, @natechapin and I had an offline discussion and we became convinced it's too weird to fire currententrychange or other events when the entry isn't actually changing. (Because it's always been at the same value, from even before script could start executing.) That is, in every other case, you could do

const oldEntry = navigation.currentEntry;
navigation.oncurrententrychange = () => {
  console.assert(navigation.currentEntry !== oldEntry);
};

but that assert would fail if we started firing the event on page load.

Based on the comments in this thread, it does sound like it'd be useful for developers, if there were some event which fired either at currententrychange time, or at pageshow time. So in the future we might want to add such an event. But we're not confident in the pageshow timing; for example, that might be too late? If you want to centralize your client-side rendering logic, delaying that on any iframes and such seems like a bad idea.

We're hopeful that routing frameworks that use the navigation API will help guide the way here, by figuring out what timing is most useful for web developers, and bundling that up into a single event. Then, in the future, we could add some kind of route or newentryready or similar event, to make these cases more convenient.

But in the meantime, creating a shared function that runs in response to both currententrychange and pageshow, or currententrychange and DOMContentLoaded, or in currententrychange plus in the top level of a carefully-chosen <script>, is probably the best path for web developers.

To the point

Ugh there was a bug for a long time where safari would trigger the popstate on pageload. What a nightmare. But the nightmare wasn't that it fired, just that it was incompatible with other browsers.

I will add a web platform test that nothing navigation API-related fires on page load, so as to avoid this issue in the future :)

@domenic domenic closed this as completed Mar 17, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 17, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 18, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 18, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 18, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 19, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 26, 2022
…vents on initial load, a=testonly

Automatic update from web-platform-tests
Navigation API: test that there are no events on initial load

See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}

--

wpt-commits: 49fcc31c450fc3dcb0d01061200a30ee37576459
wpt-pr: 33236
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 27, 2022
…vents on initial load, a=testonly

Automatic update from web-platform-tests
Navigation API: test that there are no events on initial load

See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}

--

wpt-commits: 49fcc31c450fc3dcb0d01061200a30ee37576459
wpt-pr: 33236
aosmond pushed a commit to aosmond/gecko that referenced this issue Mar 28, 2022
…vents on initial load, a=testonly

Automatic update from web-platform-tests
Navigation API: test that there are no events on initial load

See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}

--

wpt-commits: 49fcc31c450fc3dcb0d01061200a30ee37576459
wpt-pr: 33236
aosmond pushed a commit to aosmond/gecko that referenced this issue Mar 28, 2022
…vents on initial load, a=testonly

Automatic update from web-platform-tests
Navigation API: test that there are no events on initial load

See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}

--

wpt-commits: 49fcc31c450fc3dcb0d01061200a30ee37576459
wpt-pr: 33236
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
See WICG/navigation-api#31 and in particular
the concern about previous non-interop with popstate, which we want to
head off this time.

Bug: 1183545
Change-Id: If16a61a8b0a8846d2bcdbb05b3b7230e0f842b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3533169
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982939}
NOKEYCHECK=True
GitOrigin-RevId: ece85ebfd5d0f2ca9292def2018d8eae2fa3a984
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change A proposed change to the API, not foundational, but deeper than the surface
Projects
None yet
Development

No branches or pull requests

4 participants