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

View Transition API not working for router.history navigation calls #2983

Open
scarabcoder opened this issue Dec 11, 2024 · 14 comments
Open

Comments

@scarabcoder
Copy link

Which project does this relate to?

Router

Describe the bug

When navigating through browser history manually using the Tanstack Router BrowserHistory API, native browser transitions (from the new View Transitions API) are not happening. They do occur when using the native window.history API.

Example use case would be a "Back" button that navigates the user back in history.

Your Example Website or App

https://stackblitz.com/edit/github-axehctcy

Steps to Reproduce the Bug or Issue

  1. Load the example app on the "Home" page
  2. Navigate to the "About" page (note the browser default fade transition)
  3. Click the "Back to Previous Page with router.history.back()" button text
  4. Navigation will have no transition

Expected behavior

Navigation should use the same transition that navigating back with the window.history.back() or native browser back button uses. Notice the other button in the example does transition.

Screenshots or Videos

No response

Platform

  • OS: Mac OS
  • Browser: Chrome
  • Version: 131.0.6778.109

Additional context

The notify() call in the browser history source appears to override or cancel out the default transition animation. Is it possible that we do not need to call browser history subscribers in some history events, since native browser events like the user clicking the back button do not do so either?

@schiller-manuel
Copy link
Contributor

schiller-manuel commented Dec 12, 2024

I don't see any animations, can you please check if your is complete?

Also: Did this work before?
I recently fixed a bug that occurred in React 19 that might be related: https://github.com/TanStack/router/releases/tag/v1.87.3

@scarabcoder
Copy link
Author

@schiller-manuel Loom on the same project:

https://www.loom.com/share/5a0b0f59c0644233af5a9d2e8d9c9b67?sid=a320be18-540e-4f2d-9521-e3d0daf877c0

I don't believe it was working before, but I only recently started turning on transitions on our application. The issue is present on v1.87.9.

@schiller-manuel
Copy link
Contributor

where in the example code is this transition defined?

@scarabcoder
Copy link
Author

@schiller-manuel src/main.tsx line 10:

const router = createRouter({
  routeTree,
  defaultPreload: 'intent',
  defaultViewTransition: true,
});

@schiller-manuel
Copy link
Contributor

schiller-manuel commented Dec 12, 2024

the problem here is that notify() is called twice in case router.history.back() is called.

  1. back()
  2. notify()
  3. onPushPop()
  4. notify()

@scarabcoder
Copy link
Author

@schiller-manuel So it sounds like the window event listener for browser history navigation events is doubling up with the router history back() event both calling notify()?

Is the solution to simply remove notify() from the back() navigation function from the router history, or is that going to break non-browser environments?

@schiller-manuel
Copy link
Contributor

or is that going to break non-browser environments?

yes this will break those other history implementations. so notify() must be called there still, but not for browser history

I did not try it, but forward() and go() might have the same problem?

@scarabcoder
Copy link
Author

Yeah, it's present on forward(), go(), and back(), so relative history changes.
Here's my attempt at a fix, I could open a PR. It just adds a new option to createHistory that disables the notify() functionality for those three calls, relying on the browser events instead. When createBrowserHistory() calls createHistory() it turns that option on, otherwise the default behavior is to call notify() (current functionality).

scarabcoder@cb824e0

@schiller-manuel
Copy link
Contributor

another solution would be to not let history call notify inside tryNavigation, but let each history impl call history.notify() when they want to.

would probably be a bit cleaner and gives more control to the history implementations.

@scarabcoder
Copy link
Author

but let each history impl call history.notify() when they want to.

Well, the issue is that subscribers and notify() aren't present until after the history object is created in createHistory, so they can't be included in the implementation of createBrowserHistory and createMemoryHistory.

It would be a larger change to the API to make createHistory take a function parameter to build an object.

@schiller-manuel
Copy link
Contributor

browser history already calls history.notify() in onPushPop()

@scarabcoder
Copy link
Author

Oh duh. I'll see if I can open a PR tonight then. I agree that that implementation would look cleaner.

Only potential issue is that it would break anyone's custom implementation of createHistory() that isn't the current memory or browser existing implementations.

@schiller-manuel
Copy link
Contributor

you are right about that potential break.
can you please add a flag on history, that when not set to true, will still call notify inside tryNavigation() ?
something like "legacy_disableNotify"

@schiller-manuel
Copy link
Contributor

this will be fixed by #3017

here is the example from above with the PR built package: https://stackblitz.com/edit/github-axehctcy-wceasuwu?file=package-lock.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants