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

[Jim Porter <jporterbugs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 2/22/2024 5:22 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Feb 2024 10:55:49 -0800
>> Cc: 69232@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> 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?

[-- Attachment #2: 0001-When-navigating-through-history-in-EWW-don-t-keep-ad.patch --]
[-- Type: text/plain, Size: 5132 bytes --]

From 54b5820b0914a38bfb4ba3cd6efb3538d8540a4e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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