Skip to content

Commit

Permalink
differentiate between back and forward navigation
Browse files Browse the repository at this point in the history
Fixes #3536.
Relates to #3513.

Browser history API strikes back (again). Because live navigation shares
the id of the view between live navigations, the following scenario could
happen:

1. User visits page 1 (state: initial)
2. User patches the page (state: patch)
3. User visits page 2 (state: redirect)
4. User patches the page (state: patch)
5. User uses browser back button (state from 3 -> live nav)
6. User uses browser back button (state from 2 -> PATCH)

In step 6, the user is still on page 2, but LiveView would try to patch
the page instead of performing a live navigation. This is only a problem
if the two pages use the same underlying LiveView module, otherwise the
patch request would fallback to a navigation. Also, we have an unnecessary
live navigation instead of a patch at step 5.

To fix this, we now differentiate if we are navigation backwards of
forwards and store the corresponding `backType` in the history state to
do the correct type of navigation on popState, which was already
mentioned as an idea in #3513.

=======================

1. User visits page 1 (initial nav)
state: initial

pre-push: update state to {type: "patch"}
2. User patches the page
post-push: push new state {type: "patch"}

pre-push: update state to {type: "patch", backType: "redirect"}
3. User visits page 2 (live nav)
post-push: new state {type: "redirect"}

pre-push: update state to {type: "redirect", backType: "patch"}
4. User patches the page
post-push: new state {type: "patch"}

5. User uses browser back button (popState backType "patch")
-> patch from the patched page back to non-patched page 2.

6. User uses browser back button (popState backType "redirect")
-> live nav from the page 2 to patched page 1

=======================

Finally, we can also prevent the unnecessary remount when navigating all
the way back properly (see #3335).
  • Loading branch information
SteffenDE committed Nov 30, 2024
1 parent b1dfe00 commit 341688d
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 8 deletions.
1 change: 1 addition & 0 deletions assets/js/phoenix_live_view/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const PHX_AUTO_RECOVER = "auto-recover"
export const PHX_LV_DEBUG = "phx:live-socket:debug"
export const PHX_LV_PROFILE = "phx:live-socket:profiling"
export const PHX_LV_LATENCY_SIM = "phx:live-socket:latency-sim"
export const PHX_LV_HISTORY_POSITION = "phx:nav-history-position"
export const PHX_PROGRESS = "progress"
export const PHX_MOUNTED = "mounted"
export const PHX_RELOAD_STATUS = "__phoenix_reload_status__"
Expand Down
55 changes: 48 additions & 7 deletions assets/js/phoenix_live_view/live_socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ import {
PHX_LV_DEBUG,
PHX_LV_LATENCY_SIM,
PHX_LV_PROFILE,
PHX_LV_HISTORY_POSITION,
PHX_MAIN,
PHX_PARENT_ID,
PHX_VIEW_SELECTOR,
Expand Down Expand Up @@ -169,6 +170,7 @@ export default class LiveSocket {
onBeforeElUpdated: closure()},
opts.dom || {})
this.transitions = new TransitionSet()
this.currentHistoryPosition = parseInt(this.sessionStorage.getItem(PHX_LV_HISTORY_POSITION)) || 0
window.addEventListener("pagehide", _e => {
this.unloaded = true
})
Expand Down Expand Up @@ -687,10 +689,19 @@ export default class LiveSocket {
})
window.addEventListener("popstate", event => {
if(!this.registerNewLocation(window.location)){ return }
let {type, id, root, scroll} = event.state || {}
let {type, backType, id, root, scroll, position} = event.state || {}
let href = window.location.href

DOM.dispatchEvent(window, "phx:navigate", {detail: {href, patch: type === "patch", pop: true}})
// Compare positions to determine direction
let isForward = position > this.currentHistoryPosition

type = isForward ? type : (backType || type)

// Update current position
this.currentHistoryPosition = position || 0
this.sessionStorage.setItem(PHX_LV_HISTORY_POSITION, this.currentHistoryPosition.toString())

DOM.dispatchEvent(window, "phx:navigate", {detail: {href, patch: type === "patch", pop: true, direction: isForward ? "forward" : "backward"}})
this.requestDOMUpdate(() => {
if(this.main.isConnected() && (type === "patch" && id === this.main.id)){
this.main.pushLinkPatch(event, href, null, () => {
Expand Down Expand Up @@ -769,8 +780,20 @@ export default class LiveSocket {
historyPatch(href, linkState, linkRef = this.setPendingLink(href)){
if(!this.commitPendingLink(linkRef)){ return }

Browser.pushState(linkState, {type: "patch", id: this.main.id}, href)
DOM.dispatchEvent(window, "phx:navigate", {detail: {patch: true, href, pop: false}})
// Increment position for new state
this.currentHistoryPosition++
this.sessionStorage.setItem(PHX_LV_HISTORY_POSITION, this.currentHistoryPosition.toString())

// store the type for back navigation
Browser.updateCurrentState((state) => ({...state, backType: "patch"}))

Browser.pushState(linkState, {
type: "patch",
id: this.main.id,
position: this.currentHistoryPosition
}, href)

DOM.dispatchEvent(window, "phx:navigate", {detail: {patch: true, href, pop: false, direction: "forward"}})
this.registerNewLocation(window.location)
}

Expand All @@ -787,8 +810,21 @@ export default class LiveSocket {
this.withPageLoading({to: href, kind: "redirect"}, done => {
this.replaceMain(href, flash, (linkRef) => {
if(linkRef === this.linkRef){
Browser.pushState(linkState, {type: "redirect", id: this.main.id, scroll: scroll}, href)
DOM.dispatchEvent(window, "phx:navigate", {detail: {href, patch: false, pop: false}})
// Increment position for new state
this.currentHistoryPosition++
this.sessionStorage.setItem(PHX_LV_HISTORY_POSITION, this.currentHistoryPosition.toString())

// store the type for back navigation
Browser.updateCurrentState((state) => ({...state, backType: "redirect"}))

Browser.pushState(linkState, {
type: "redirect",
id: this.main.id,
scroll: scroll,
position: this.currentHistoryPosition
}, href)

DOM.dispatchEvent(window, "phx:navigate", {detail: {href, patch: false, pop: false, direction: "forward"}})
this.registerNewLocation(window.location)
}
done()
Expand All @@ -797,7 +833,12 @@ export default class LiveSocket {
}

replaceRootHistory(){
Browser.pushState("replace", {root: true, type: "patch", id: this.main.id})
Browser.pushState("replace", {
root: true,
type: "patch",
id: this.main.id,
position: this.currentHistoryPosition // Preserve current position
})
}

registerNewLocation(newLocation){
Expand Down
4 changes: 4 additions & 0 deletions assets/js/phoenix_live_view/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,10 @@ export default class View {
if(this.root === this){
this.formsForRecovery = this.getFormsForRecovery()
}
if(this.isMain() && window.history.state === null){
// set initial history entry if this is the first page load
this.liveSocket.replaceRootHistory()
}

if(liveview_version !== this.liveSocket.version()){
console.error(`LiveView asset version mismatch. JavaScript version ${this.liveSocket.version()} vs. server ${liveview_version}. To avoid issues, please ensure that your assets use the same version as the server.`)
Expand Down
3 changes: 2 additions & 1 deletion assets/test/live_socket_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ describe("LiveSocket", () => {
let liveSocket = new LiveSocket("/live", Socket, {sessionStorage: override})
liveSocket.getLatencySim()

expect(getItemCalls).toEqual(1)
// liveSocket constructor reads nav history position from sessionStorage
expect(getItemCalls).toEqual(2)
})
})
4 changes: 4 additions & 0 deletions test/e2e/support/navigation.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ defmodule Phoenix.LiveViewTest.E2E.Navigation.Layout do
let liveSocket = new LiveSocket("/live", window.Phoenix.Socket, {params: {_csrf_token: csrfToken}})
liveSocket.connect()
window.liveSocket = liveSocket
window.addEventListener("phx:navigate", (e) => {
console.log("navigate event", JSON.stringify(e.detail))
})
</script>
<style>
Expand Down
89 changes: 89 additions & 0 deletions test/e2e/tests/navigation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,92 @@ test("scrolls hash el into view after live navigation (issue #3452)", async ({pa
expect(scrollTop).toBeGreaterThanOrEqual(offset - 500)
expect(scrollTop).toBeLessThanOrEqual(offset + 500)
})

test("navigating all the way back works without remounting (only patching)", async ({page}) => {
await page.goto("/navigation/a")
await syncLV(page)
networkEvents = []
await page.getByRole("link", {name: "Patch this LiveView"}).click()
await syncLV(page)
await page.goBack()
await syncLV(page)
expect(networkEvents).toEqual([])
// we only expect patch navigation
expect(webSocketEvents.filter(e => e.payload.indexOf("phx_leave") !== -1)).toHaveLength(0)
// we patched 2 times
expect(webSocketEvents.filter(e => e.payload.indexOf("live_patch") !== -1)).toHaveLength(2)
})

// see https://github.com/phoenixframework/phoenix_live_view/pull/3513
// see https://github.com/phoenixframework/phoenix_live_view/issues/3536
test("back and forward navigation types are tracked", async ({page}) => {
let consoleMessages = []
page.on("console", msg => consoleMessages.push(msg.text()))
const getNavigationEvent = () => {
const ev = consoleMessages.find((e) => e.startsWith("navigate event"))
consoleMessages = []
return JSON.parse(ev.slice(15))
}
// initial page visit
await page.goto("/navigation/b")
await syncLV(page)
consoleMessages = []
networkEvents = []
// type: redirect
await page.getByRole("link", {name: "LiveView A"}).click()
await syncLV(page)
expect(getNavigationEvent()).toEqual({
href: "http://localhost:4004/navigation/a",
patch: false,
pop: false,
direction: "forward"
})
// type: patch
await page.getByRole("link", {name: "Patch this LiveView"}).click()
await syncLV(page)
expect(getNavigationEvent()).toEqual({
href: expect.stringMatching(/\/navigation\/a\?param=.*/),
patch: true,
pop: false,
direction: "forward"
})
// back should also be type: patch
await page.goBack()
await expect(page).toHaveURL("/navigation/a")
expect(getNavigationEvent()).toEqual({
href: "http://localhost:4004/navigation/a",
patch: true,
pop: true,
direction: "backward"
})
await page.goBack()
await expect(page).toHaveURL("/navigation/b")
expect(getNavigationEvent()).toEqual({
href: "http://localhost:4004/navigation/b",
patch: false,
pop: true,
direction: "backward"
})
await page.goForward()
await expect(page).toHaveURL("/navigation/a")
expect(getNavigationEvent()).toEqual({
href: "http://localhost:4004/navigation/a",
patch: false,
pop: true,
direction: "forward"
})
await page.goForward()
await expect(page).toHaveURL(/\/navigation\/a\?param=.*/)
expect(getNavigationEvent()).toEqual({
href: expect.stringMatching(/\/navigation\/a\?param=.*/),
patch: true,
pop: true,
direction: "forward"
})
// we don't expect any full page reloads
expect(networkEvents).toEqual([])
// we only expect 3 navigate navigations (from b to a, back from a to b, back to a)
expect(webSocketEvents.filter(e => e.payload.indexOf("phx_leave") !== -1)).toHaveLength(3)
// we expect 3 patches (a to a with param, back to a, back to a with param)
expect(webSocketEvents.filter(e => e.payload.indexOf("live_patch") !== -1)).toHaveLength(3)
})

0 comments on commit 341688d

Please sign in to comment.