Subject: Re: bug#69232: 30.0.50; [PATCH] EWW history navigation gets caught in a loop

From: Jim Porter <[email-masked]>
Date: Thu, 22 Feb 2024 09:18:47 -0800

On 2/22/2024 5:22 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Feb 2024 10:55:49 -0800
>> Cc: [email-masked]
>> From: Jim Porter <[email-masked]>
>>
>> On 2/19/2024 4:12 AM, Eli Zaretskii wrote:
>>> I'm not sure this is a bug fix, and I think this behavior change does
>>> need a NEWS entry.
>>
>> Ok, I added one.
> 
> Thanks, but I'm afraid it's somewhat confusing:
> 
>> +*** History navigation in EWW now works like other browsers.
>> +Previously, when navigating back and forward through page history in
>> +EWW, new history entries could get added to the history list.  Now, when
>> +navigating through history, EWW preserves the history list and only
>> +displays the relevant history entry.
> 
> This doesn't really explain the nature of the change in behavior.
> AFAIU, the previous behavior was that going back in browsing history
> could add the old entries to the history instead of removing them; now
> going back will _never_ add entries to the history.  Isn't that so?

In other browsers, you only add new entries to the back/forward history 
when you go to a totally new page (e.g. by clicking a link). If you just 
go back or forward, you should only change your position in the 
already-existing history. That's (roughly) what EWW does with the patch, 
with the exception that a new page doesn't go into the history immediately.

Previously, every time you went back or forward, it added entries to the 
end of the history list.

How does this look?

From 54b5820b0914a38bfb4ba3cd6efb3538d8540a4e Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sat, 17 Feb 2024 20:49:15 -0800
Subject: [PATCH] When navigating through history in EWW, don't keep adding to
 'eww-history'

This resolves an issue where navigating back and then forward kept
adding new history entries so you could never hit the "end" (bug#69232).

* lisp/net/eww.el (eww-history-position): Add docstring.
(eww-mode-map, eww-context-menu): Use correct predicates for when to
enable back/forward.
(eww-save-history): Save history entry in its original place when
viewing a historical page.
(eww-back-url): Set 'eww-history-position' based on the result of
'eww-save-history'.
(eww-forward-url): Set 'eww-history-position' directly, since
'eww-save-history' no longer adds a new entry in this case.

* etc/NEWS: Announce this change.
---
 etc/NEWS        |  7 +++++++
 lisp/net/eww.el | 39 ++++++++++++++++++++++++++++-----------
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4477116248e..34146e9201c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -982,6 +982,13 @@ When invoked with the prefix argument ('C-u'),
 This is useful for continuing reading the URL in the current buffer
 when the new URL is fetched.
 
+---
+*** History navigation in EWW now works like other browsers.
+Previously, when navigating back and forward through page history, EWW
+would add a new entry to the end of the history list each time.  Now,
+when navigating through history, EWW updates history elements in-place,
+preserving the overall history list.
+
 ** go-ts-mode
 
 +++
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 6ae1e6d3d0a..8196f222ad8 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -312,7 +312,10 @@ eww-valid-certificate
 
 (defvar eww-data nil)
 (defvar eww-history nil)
-(defvar eww-history-position 0)
+(defvar eww-history-position 0
+  "The 1-indexed position in `eww-history'.
+If zero, EWW is at the newest page, which isn't yet present in
+`eww-history'.")
 (defvar eww-prompt-history nil)
 
 (defvar eww-local-regex "localhost"
@@ -1129,9 +1132,9 @@ eww-mode-map
           ["Reload" eww-reload t]
           ["Follow URL in new buffer" eww-open-in-new-buffer]
           ["Back to previous page" eww-back-url
-           :active (not (zerop (length eww-history)))]
+           :active (< eww-history-position (length eww-history))]
           ["Forward to next page" eww-forward-url
-           :active (not (zerop eww-history-position))]
+           :active (> eww-history-position 1)]
           ["Browse with external browser" eww-browse-with-external-browser t]
           ["Download" eww-download t]
           ["View page source" eww-view-source]
@@ -1155,9 +1158,9 @@ eww-context-menu
     (easy-menu-define nil easy-menu nil
       '("Eww"
         ["Back to previous page" eww-back-url
-	 :visible (not (zerop (length eww-history)))]
+	 :active (< eww-history-position (length eww-history))]
 	["Forward to next page" eww-forward-url
-	 :visible (not (zerop eww-history-position))]
+	 :active (> eww-history-position 1)]
 	["Reload" eww-reload t]))
     (dolist (item (reverse (lookup-key easy-menu [menu-bar eww])))
       (when (consp item)
@@ -1280,16 +1283,20 @@ eww-back-url
   (interactive nil eww-mode)
   (when (>= eww-history-position (length eww-history))
     (user-error "No previous page"))
-  (eww-save-history)
-  (setq eww-history-position (+ eww-history-position 2))
+  (if (eww-save-history)
+      ;; We were at the latest page (which was just added to the
+      ;; history), so go back two entries.
+      (setq eww-history-position 2)
+    (setq eww-history-position (1+ eww-history-position)))
   (eww-restore-history (elt eww-history (1- eww-history-position))))
 
 (defun eww-forward-url ()
   "Go to the next displayed page."
   (interactive nil eww-mode)
-  (when (zerop eww-history-position)
+  (when (<= eww-history-position 1)
     (user-error "No next page"))
   (eww-save-history)
+  (setq eww-history-position (1- eww-history-position))
   (eww-restore-history (elt eww-history (1- eww-history-position))))
 
 (defun eww-restore-history (elem)
@@ -2289,11 +2296,21 @@ eww-bookmark-mode
 ;;; History code
 
 (defun eww-save-history ()
+  "Save the current page's data to the history.
+If the current page is a historial one loaded from
+`eww-history' (e.g. by calling `eww-back-url'), this will update the
+page's entry in `eww-history' and return nil.  Otherwise, add a new
+entry to `eww-history' and return t."
   (plist-put eww-data :point (point))
   (plist-put eww-data :text (buffer-string))
-  (let ((history-delete-duplicates nil))
-    (add-to-history 'eww-history eww-data eww-history-limit t))
-  (setq eww-data (list :title "")))
+  (prog1
+      (if (zerop eww-history-position)
+          (let ((history-delete-duplicates nil))
+            (add-to-history 'eww-history eww-data eww-history-limit t)
+            t)
+        (setf (elt eww-history (1- eww-history-position)) eww-data)
+        nil)
+    (setq eww-data (list :title ""))))
 
 (defvar eww-current-buffer)
 
-- 
2.25.1