Merge pull request #1285 from domchristie/tidy_same_page_anchor_visits_2

Simplify same page anchor visits

Author: jorgemanrubia

src/core/drive/history.js

@@ -1,11 +1,10 @@
-import { nextMicrotask, uuid } from "../../util"
+import { uuid } from "../../util"
 
 export class History {
   location
   restorationIdentifier = uuid()
   restorationData = {}
   started = false
-  pageLoaded = false
   currentIndex = 0
 
   constructor(delegate) {
@@ -15,7 +14,6 @@ export class History {
   start() {
     if (!this.started) {
       addEventListener("popstate", this.onPopState, false)
-      addEventListener("load", this.onPageLoad, false)
       this.currentIndex = history.state?.turbo?.restorationIndex || 0
       this.started = true
       this.replace(new URL(window.location.href))
@@ -25,7 +23,6 @@ export class History {
   stop() {
     if (this.started) {
       removeEventListener("popstate", this.onPopState, false)
-      removeEventListener("load", this.onPageLoad, false)
       this.started = false
     }
   }
@@ -81,32 +78,18 @@ export class History {
   // Event handlers
 
   onPopState = (event) => {
-    if (this.shouldHandlePopState()) {
-      const { turbo } = event.state || {}
-      if (turbo) {
-        this.location = new URL(window.location.href)
-        const { restorationIdentifier, restorationIndex } = turbo
-        this.restorationIdentifier = restorationIdentifier
-        const direction = restorationIndex > this.currentIndex ? "forward" : "back"
-        this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction)
-        this.currentIndex = restorationIndex
-      }
+    const { turbo } = event.state || {}
+    this.location = new URL(window.location.href)
+
+    if (turbo) {
+      const { restorationIdentifier, restorationIndex } = turbo
+      this.restorationIdentifier = restorationIdentifier
+      const direction = restorationIndex > this.currentIndex ? "forward" : "back"
+      this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction)
+      this.currentIndex = restorationIndex
+    } else {
+      this.currentIndex++
+      this.delegate.historyPoppedWithEmptyState(this.location)
     }
   }
-
-  onPageLoad = async (_event) => {
-    await nextMicrotask()
-    this.pageLoaded = true
-  }
-
-  // Private
-
-  shouldHandlePopState() {
-    // Safari dispatches a popstate event after window's load event, ignore it
-    return this.pageIsLoaded()
-  }
-
-  pageIsLoaded() {
-    return this.pageLoaded || document.readyState == "complete"
-  }
 }

src/core/drive/navigator.js

@@ -1,6 +1,6 @@
 import { getVisitAction } from "../../util"
 import { FormSubmission } from "./form_submission"
-import { expandURL, getAnchor, getRequestURL } from "../url"
+import { expandURL } from "../url"
 import { Visit } from "./visit"
 import { PageSnapshot } from "./page_snapshot"
 
@@ -139,20 +139,10 @@ export class Navigator {
     delete this.currentVisit
   }
 
+  // Same-page links are no longer handled with a Visit.
+  // This method is still needed for Turbo Native adapters.
   locationWithActionIsSamePage(location, action) {
-    const anchor = getAnchor(location)
-    const currentAnchor = getAnchor(this.view.lastRenderedLocation)
-    const isRestorationToTop = action === "restore" && typeof anchor === "undefined"
-
-    return (
-      action !== "replace" &&
-      getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) &&
-      (isRestorationToTop || (anchor != null && anchor !== currentAnchor))
-    )
-  }
-
-  visitScrolledToSamePageLocation(oldURL, newURL) {
-    this.delegate.visitScrolledToSamePageLocation(oldURL, newURL)
+    return false
   }
 
   // Visits

src/core/drive/visit.js

@@ -85,7 +85,6 @@ export class Visit {
     this.snapshot = snapshot
     this.snapshotHTML = snapshotHTML
     this.response = response
-    this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action)
     this.isPageRefresh = this.view.isPageRefresh(this)
     this.visitCachedSnapshot = visitCachedSnapshot
     this.willRender = willRender
@@ -113,10 +112,6 @@ export class Visit {
     return this.history.getRestorationDataForIdentifier(this.restorationIdentifier)
   }
 
-  get silent() {
-    return this.isSamePage
-  }
-
   start() {
     if (this.state == VisitState.initialized) {
       this.recordTimingMetric(TimingMetric.visitStart)
@@ -253,7 +248,7 @@ export class Visit {
       const isPreview = this.shouldIssueRequest()
       this.render(async () => {
         this.cacheSnapshot()
-        if (this.isSamePage || this.isPageRefresh) {
+        if (this.isPageRefresh) {
           this.adapter.visitRendered(this)
         } else {
           if (this.view.renderPromise) await this.view.renderPromise
@@ -281,17 +276,6 @@ export class Visit {
     }
   }
 
-  goToSamePageAnchor() {
-    if (this.isSamePage) {
-      this.render(async () => {
-        this.cacheSnapshot()
-        this.performScroll()
-        this.changeHistory()
-        this.adapter.visitRendered(this)
-      })
-    }
-  }
-
   // Fetch request delegate
 
   prepareRequest(request) {
@@ -353,9 +337,6 @@ export class Visit {
       } else {
         this.scrollToAnchor() || this.view.scrollToTop()
       }
-      if (this.isSamePage) {
-        this.delegate.visitScrolledToSamePageLocation(this.view.lastRenderedLocation, this.location)
-      }
 
       this.scrolled = true
     }
@@ -394,9 +375,7 @@ export class Visit {
   }
 
   shouldIssueRequest() {
-    if (this.isSamePage) {
-      return false
-    } else if (this.action == "restore") {
+    if (this.action == "restore") {
       return !this.hasCachedSnapshot()
     } else {
       return this.willRender

src/core/native/browser_adapter.js

@@ -24,7 +24,6 @@ export class BrowserAdapter {
 
     visit.loadCachedSnapshot()
     visit.issueRequest()
-    visit.goToSamePageAnchor()
   }
 
   visitRequestStarted(visit) {

src/core/session.js

@@ -216,6 +216,12 @@ export class Session {
     }
   }
 
+  historyPoppedWithEmptyState(location) {
+    this.history.replace(location)
+    this.view.lastRenderedLocation = location
+    this.view.cacheSnapshot()
+  }
+
   // Scroll observer delegate
 
   scrollPositionChanged(position) {
@@ -260,7 +266,7 @@ export class Session {
   // Navigator delegate
 
   allowsVisitingLocationWithAction(location, action) {
-    return this.locationWithActionIsSamePage(location, action) || this.applicationAllowsVisitingLocation(location)
+    return this.applicationAllowsVisitingLocation(location)
   }
 
   visitProposedToLocation(location, options) {
@@ -276,9 +282,7 @@ export class Session {
       this.view.markVisitDirection(visit.direction)
     }
     extendURLWithDeprecatedProperties(visit.location)
-    if (!visit.silent) {
-      this.notifyApplicationAfterVisitingLocation(visit.location, visit.action)
-    }
+    this.notifyApplicationAfterVisitingLocation(visit.location, visit.action)
   }
 
   visitCompleted(visit) {
@@ -287,14 +291,6 @@ export class Session {
     this.notifyApplicationAfterPageLoad(visit.getTimingMetrics())
   }
 
-  locationWithActionIsSamePage(location, action) {
-    return this.navigator.locationWithActionIsSamePage(location, action)
-  }
-
-  visitScrolledToSamePageLocation(oldURL, newURL) {
-    this.notifyApplicationAfterVisitingSamePageLocation(oldURL, newURL)
-  }
-
   // Form submit observer delegate
 
   willSubmitForm(form, submitter) {
@@ -334,9 +330,7 @@ export class Session {
   // Page view delegate
 
   viewWillCacheSnapshot() {
-    if (!this.navigator.currentVisit?.silent) {
-      this.notifyApplicationBeforeCachingSnapshot()
-    }
+    this.notifyApplicationBeforeCachingSnapshot()
   }
 
   allowsImmediateRender({ element }, options) {
@@ -428,15 +422,6 @@ export class Session {
     })
   }
 
-  notifyApplicationAfterVisitingSamePageLocation(oldURL, newURL) {
-    dispatchEvent(
-      new HashChangeEvent("hashchange", {
-        oldURL: oldURL.toString(),
-        newURL: newURL.toString()
-      })
-    )
-  }
-
   notifyApplicationAfterFrameLoad(frame) {
     return dispatch("turbo:frame-load", { target: frame })
   }

src/tests/functional/navigation_tests.js

@@ -299,7 +299,7 @@ test("following a same-origin link inside an SVG element", async ({ page }) => {
   await page.keyboard.press("Enter")
 
   await expect(page).toHaveURL(withPathname("/src/tests/fixtures/one.html"))
-  expect(await visitAction(page)).toEqual("advance")
+  expect(await visitAction(page)).toEqual("load")
 })
 
 test("following a cross-origin link inside an SVG element", async ({ page }) => {

src/util.js

@@ -248,6 +248,7 @@ export function findLinkFromClickTarget(target) {
   const link = findClosestRecursively(target, "a[href], a[xlink\\:href]")
 
   if (!link) return null
+  if (link.href.startsWith("#")) return null
   if (link.hasAttribute("download")) return null
 
   const linkTarget = link.getAttribute("target")