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

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

[Jim Porter]

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

If you navigate back in EWW history, and then forward, you can never hit 
the "end": it keeps adding duplicate history elements, even though 
you're not visiting any new pages. To see this in action, start from 
`emacs -Q`, then:

M-x eww RET fsf.org RET
M-x eww RET gnu.org RET
H    ;; Notice that there's one item in the history: the FSF page[1]
q    ;; Close history window
l    ;; Go back one in the history to the FSF page
H    ;; Notice that there are two items in the history
r    ;; Go forward one, back at the GNU page
r    ;; Go forward again, now at the FSF page(?!)
r    ;; Ditto, now at the GNU page
r    ;; Repeat ad infinitum
H    ;; Now there are many entries, alternating between GNU and FSF

Attached is a patch that fixes this. Now, 'eww-save-history' will update 
the history entry in-place when viewing a historical page, and 
'eww-back-url' / 'eww-forward-url' take that into account. I also fixed 
the predicates for when the back/forward menu items were enabled.

I think this is just a straightforward bug fix, so I didn't add a NEWS 
entry. I could add one though if it seems worthwhile.

[1] EWW doesn't immediately add pages to the history when you navigate 
to them. Maybe it should, but that can be addressed another day.

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

From 2e926fdd83a2eb6b68cc21bf84ea778e923d3e55 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".

* 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.
---
 lisp/net/eww.el | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

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

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

[Eli Zaretskii]

> Date: Sat, 17 Feb 2024 20:59:57 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> 
> If you navigate back in EWW history, and then forward, you can never hit 
> the "end": it keeps adding duplicate history elements, even though 
> you're not visiting any new pages. To see this in action, start from 
> `emacs -Q`, then:
> 
> M-x eww RET fsf.org RET
> M-x eww RET gnu.org RET
> H    ;; Notice that there's one item in the history: the FSF page[1]
> q    ;; Close history window
> l    ;; Go back one in the history to the FSF page
> H    ;; Notice that there are two items in the history
> r    ;; Go forward one, back at the GNU page
> r    ;; Go forward again, now at the FSF page(?!)
> r    ;; Ditto, now at the GNU page
> r    ;; Repeat ad infinitum
> H    ;; Now there are many entries, alternating between GNU and FSF
> 
> Attached is a patch that fixes this. Now, 'eww-save-history' will update 
> the history entry in-place when viewing a historical page, and 
> 'eww-back-url' / 'eww-forward-url' take that into account. I also fixed 
> the predicates for when the back/forward menu items were enabled.
> 
> I think this is just a straightforward bug fix, so I didn't add a NEWS 
> entry. I could add one though if it seems worthwhile.

I'm not sure this is a bug fix, and I think this behavior change does
need a NEWS entry.

And have sure you are that no one will want the current behavior?

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

[Jim Porter]

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

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.

> And have sure you are that no one will want the current behavior?

Well, I suppose I can't prove that there's no one who likes the current 
behavior. I'd be pretty surprised though, since I've never seen a 
browser that manages back/forward navigation like this.

In *theory*, a person might be able to (ab)use this to track changes to 
a given page across refreshes, but that doesn't work consistently, since 
EWW doesn't always add a new history entry upon reloading. Given that 
you have to jump through some hoops to trigger this behavior (for 
example, this doesn't work if you're at a new page not in 'eww-history' 
yet), I doubt anyone has noticed that this is possible. On the off 
chance that someone wants a feature like that, I think we could do it in 
a better way, anyway.

If you're still concerned that someone might prefer the current 
behavior, I could announce the change to emacs-devel after (or before) 
it merges in order to give people an opportunity to speak up.

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

From 89d60d41e2001959fa73f229a7d98955d10d6c98 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..f9223a31fbd 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 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.
+
 ** 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

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

[Eli Zaretskii]

> Date: Mon, 19 Feb 2024 10:55:49 -0800
> Cc: 69232@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> If you're still concerned that someone might prefer the current 
> behavior, I could announce the change to emacs-devel after (or before) 
> it merges in order to give people an opportunity to speak up.

Maybe announce it now, so that in a week or two, when it's time to
install, we could learn whether this change would bother someone.

Thanks.

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

[Eli Zaretskii]

> 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?

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

[Jim Porter]

[-- 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

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

[Eli Zaretskii]

> Date: Thu, 22 Feb 2024 09:18:47 -0800
> Cc: 69232@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> 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?

It is IMO still not clear enough about the behavior change.  It looks
like you are describing what the old implementation did and the new
one will do, instead of describing the behavior as it the user will
see it.  Can you instead describe the change in terms of user-facing
behavior?

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

[Jim Porter]

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

On 2/22/2024 11:04 AM, Eli Zaretskii wrote:
> It is IMO still not clear enough about the behavior change.  It looks
> like you are describing what the old implementation did and the new
> one will do, instead of describing the behavior as it the user will
> see it.  Can you instead describe the change in terms of user-facing
> behavior?

What about this? It mentions the (IMO) buggy behavior and how the new 
implementation prevents that.

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

From 0f2528482527262ff59153595d7a3f10517153ca 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        |  8 ++++++++
 lisp/net/eww.el | 39 ++++++++++++++++++++++++++++-----------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4477116248e..947fb8cf0fc 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -982,6 +982,14 @@ 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 duplicate entry to the end of the history list each time.
+This made it impossible to navigate to the "end" of the history list.
+Now, navigating through history in EWW simply changes your position in
+the history list, allowing you to reach the end as expected.
+
 ** 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

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

[Eli Zaretskii]

> Date: Thu, 22 Feb 2024 12:10:36 -0800
> Cc: 69232@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 2/22/2024 11:04 AM, Eli Zaretskii wrote:
> > It is IMO still not clear enough about the behavior change.  It looks
> > like you are describing what the old implementation did and the new
> > one will do, instead of describing the behavior as it the user will
> > see it.  Can you instead describe the change in terms of user-facing
> > behavior?
> 
> What about this? It mentions the (IMO) buggy behavior and how the new 
> implementation prevents that.

Much better, thanks.

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> From 0f2528482527262ff59153595d7a3f10517153ca 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        |  8 ++++++++
>  lisp/net/eww.el | 39 ++++++++++++++++++++++++++++-----------
>  2 files changed, 36 insertions(+), 11 deletions(-)

One possible problem with this patch, I realize now, is that if you
navigate backward ('l') and then visit another link there, the new page
is added to the very end of history rather than the immediate next
position. This would be confusing if you, then, navigate back and find
that it's not the page from which you followed the link. Perhaps the
original code was a hack around this?

--
James

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

[Eli Zaretskii]

> Cc: 69232@debbugs.gnu.org
> Date: Sat, 24 Feb 2024 19:45:49 +0530
> From:  James Thomas via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> One possible problem with this patch, I realize now, is that if you
> navigate backward ('l') and then visit another link there, the new page
> is added to the very end of history rather than the immediate next
> position. This would be confusing if you, then, navigate back and find
> that it's not the page from which you followed the link. Perhaps the
> original code was a hack around this?

The only reasonable alternative is to throw away all the history after
'l', which I don't think is better.

What do other browsers do in this situation?

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

[Jim Porter]

On 2/24/2024 6:20 AM, Eli Zaretskii wrote:
>> Cc: 69232@debbugs.gnu.org
>> Date: Sat, 24 Feb 2024 19:45:49 +0530
>> From:  James Thomas via "Bug reports for GNU Emacs,
>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> One possible problem with this patch, I realize now, is that if you
>> navigate backward ('l') and then visit another link there, the new page
>> is added to the very end of history rather than the immediate next
>> position. This would be confusing if you, then, navigate back and find
>> that it's not the page from which you followed the link. Perhaps the
>> original code was a hack around this?

I intentionally chose not to address this in my patch (though perhaps 
that's not the right call).

> The only reasonable alternative is to throw away all the history after
> 'l', which I don't think is better.
> 
> What do other browsers do in this situation?

They throw away any history after the page you clicked the link on 
(which I think is what you mean by the above).

Another option might be, "If you're at a historical page and you click a 
link, add that page as a duplicate to the end of the history." That 
would partially restore the old behavior, but only in this case where 
things might otherwise be confusing.

We could also add a user option to select between these behaviors, since 
the former is the de facto standard for browsers.

(I think the real fix would be something like a history *tree*, but 
that's probably quite a bit more work.)

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii wrote:

> The only reasonable alternative is to throw away all the history after
> 'l', which I don't think is better.
>
> What do other browsers do in this situation?

Exactly that. Firefox, Chrome etc. for e.g.

--
James

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> Another option might be, "If you're at a historical page and you click
> a link, add that page as a duplicate to the end of the history." That
> would partially restore the old behavior, but only in this case where
> things might otherwise be confusing.

Just putting out a few ideas for refining this (in case it's opted for):

My personal preference would be for the entire history up to that point
be added (or be available), not just the source page of the link; so
because that might sometimes be too much, (adding to the end) only the
entire history up to the first occurrence of the source page.

--

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

[Eli Zaretskii]

> From: James Thomas <jimjoe@gmx.net>
> Cc: jporterbugs@gmail.com,  69232@debbugs.gnu.org
> Date: Sun, 25 Feb 2024 04:04:13 +0530
> 
> Eli Zaretskii wrote:
> 
> > The only reasonable alternative is to throw away all the history after
> > 'l', which I don't think is better.
> >
> > What do other browsers do in this situation?
> 
> Exactly that. Firefox, Chrome etc. for e.g.

So maybe we should offer that as optional behavior?

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

[Jim Porter]

On 2/24/2024 9:57 PM, Eli Zaretskii wrote:
>> From: James Thomas <jimjoe@gmx.net>
>> Cc: jporterbugs@gmail.com,  69232@debbugs.gnu.org
>> Date: Sun, 25 Feb 2024 04:04:13 +0530
>>
>> Eli Zaretskii wrote:
>>
>>> The only reasonable alternative is to throw away all the history after
>>> 'l', which I don't think is better.
>>>
>>> What do other browsers do in this situation?
>>
>> Exactly that. Firefox, Chrome etc. for e.g.
> 
> So maybe we should offer that as optional behavior?

If anything, I think this should be the default, with some other options 
provided for people who don't want to lose any history. That way the 
default behavior is what people know.

How about this as an option for preserving history though: if you're at 
a historical page and you navigate to a link, open that link in a *new* 
buffer, copying over the history leading up to that link. That way, you 
have two separate history timelines and nothing ever gets lost or munged.

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

[Eli Zaretskii]

> Date: Sun, 25 Feb 2024 11:40:39 -0800
> Cc: 69232@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 2/24/2024 9:57 PM, Eli Zaretskii wrote:
> >> From: James Thomas <jimjoe@gmx.net>
> >> Cc: jporterbugs@gmail.com,  69232@debbugs.gnu.org
> >> Date: Sun, 25 Feb 2024 04:04:13 +0530
> >>
> >> Eli Zaretskii wrote:
> >>
> >>> The only reasonable alternative is to throw away all the history after
> >>> 'l', which I don't think is better.
> >>>
> >>> What do other browsers do in this situation?
> >>
> >> Exactly that. Firefox, Chrome etc. for e.g.
> > 
> > So maybe we should offer that as optional behavior?
> 
> If anything, I think this should be the default, with some other options 
> provided for people who don't want to lose any history. That way the 
> default behavior is what people know.

I don't think I mind.

> How about this as an option for preserving history though: if you're at 
> a historical page and you navigate to a link, open that link in a *new* 
> buffer, copying over the history leading up to that link. That way, you 
> have two separate history timelines and nothing ever gets lost or munged.

Sounds too complicated, and two backward-incompatible changes instead
of just one.  My recommendation is not to over-engineer this.

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

[Jim Porter]

On 2/25/2024 11:49 AM, Eli Zaretskii wrote:
>> If anything, I think this should be the default, with some other options
>> provided for people who don't want to lose any history. That way the
>> default behavior is what people know.
> 
> I don't think I mind.

Thanks. Even if there were multiple options, this is probably what I'd 
choose, if only out of habit.

>> How about this as an option for preserving history though: if you're at
>> a historical page and you navigate to a link, open that link in a *new*
>> buffer, copying over the history leading up to that link. That way, you
>> have two separate history timelines and nothing ever gets lost or munged.
> 
> Sounds too complicated, and two backward-incompatible changes instead
> of just one.  My recommendation is not to over-engineer this.

I'm ok with implementing any combination of options here to make people 
happy (within reason; let's keep the number of possibilities to a few). 
If that means just making EWW work like other browsers, for better or 
worse, that's ok too. We can always revisit the decision to add more 
options later.

In any case, I'm going to look into writing some regression tests for 
this code so that we can be sure it works correctly.

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

[Jim Porter]

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

On 2/25/2024 2:41 PM, Jim Porter wrote:
> On 2/25/2024 11:49 AM, Eli Zaretskii wrote:
>>> If anything, I think this should be the default, with some other options
>>> provided for people who don't want to lose any history. That way the
>>> default behavior is what people know.
>>
>> I don't think I mind.
> 
> Thanks. Even if there were multiple options, this is probably what I'd 
> choose, if only out of habit.

Here's a patch that deletes "future history" in the case we'd previously 
discussed. I also added some regression tests for this. I think this all 
works correctly, but it's probably worth some manual testing over a few 
days just to be on the safe side.

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

From c97b30da6b16a553365c1ecc4d9d4cdd11395df4 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.  New argument CLEAR-FUTURE.
(eww-setup-buffer, eww-follow-link, eww-readable): Clear future history.
(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.

* test/lisp/net/eww-tests.el: New file.

* etc/NEWS: Announce this change.
---
 etc/NEWS                   |  10 ++++
 lisp/net/eww.el            |  53 ++++++++++++------
 test/lisp/net/eww-tests.el | 108 +++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+), 15 deletions(-)
 create mode 100644 test/lisp/net/eww-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index 6d444daf152..7983c4be25b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1004,6 +1004,16 @@ 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 duplicate entry to the end of the history list each time.
+This made it impossible to navigate to the "end" of the history list.
+Now, navigating through history in EWW simply changes your position in
+the history list, allowing you to reach the end as expected.  In
+addition, when navigating to a new page from a historical one, EWW
+deletes the history entries after the current page.
+
 ** go-ts-mode
 
 +++
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 5a25eef9e3c..904c778cb0a 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"
@@ -958,7 +961,7 @@ eww-display-pdf
 (defun eww-setup-buffer ()
   (when (or (plist-get eww-data :url)
             (plist-get eww-data :dom))
-    (eww-save-history))
+    (eww-save-history t))
   (let ((inhibit-read-only t))
     (remove-overlays)
     (erase-buffer))
@@ -1036,7 +1039,7 @@ eww-readable
 		(libxml-parse-html-region (point-min) (point-max))))
          (base (plist-get eww-data :url)))
     (eww-score-readability dom)
-    (eww-save-history)
+    (eww-save-history t)
     (eww-display-html nil nil
                       (list 'base (list (cons 'href base))
                             (eww-highest-readability dom))
@@ -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)
@@ -1958,7 +1965,7 @@ eww-follow-link
      ((and (setq target (url-target (url-generic-parse-url url)))
 	   (eww-same-page-p url (plist-get eww-data :url)))
       (let ((point (point)))
-	(eww-save-history)
+	(eww-save-history t)
 	(plist-put eww-data :url url)
         (goto-char (point-min))
         (if-let ((match (text-property-search-forward 'shr-target-id target #'member)))
@@ -2288,12 +2295,28 @@ eww-bookmark-mode
 
 ;;; History code
 
-(defun eww-save-history ()
+(defun eww-save-history (&optional clear-future)
+  "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.
+
+If CLEAR-FUTURE is non-nil, clear any future history elements from
+`eww-history'."
   (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)
+        (when (and clear-future (> eww-history-position 1))
+          (setq eww-history (nthcdr (1- eww-history-position) eww-history)
+                eww-history-position 1))
+        nil)
+    (setq eww-data (list :title ""))))
 
 (defvar eww-current-buffer)
 
diff --git a/test/lisp/net/eww-tests.el b/test/lisp/net/eww-tests.el
new file mode 100644
index 00000000000..9febaa33193
--- /dev/null
+++ b/test/lisp/net/eww-tests.el
@@ -0,0 +1,108 @@
+;;; eww-tests.el --- tests for eww.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'eww)
+
+(defmacro eww-test--with-mock-retrieve (&rest body)
+  "Evaluate BODY with a mock implementation of `eww-retrieve'.
+This avoids network requests during our tests.  Additionally, prepare a
+temporary EWW buffer for our tests."
+  (declare (indent 0))
+    `(cl-letf (((symbol-function 'eww-retrieve)
+                (lambda (url callback args)
+                  (with-temp-buffer
+                    ;; Write out an empty list of headers and the URL
+                    ;; as our body.
+                    (insert "\n" url)
+                    (apply callback nil args)))))
+       (with-temp-buffer
+         (eww-mode)
+         ,@body)))
+
+(defun eww-test--history-urls ()
+  (mapcar (lambda (elem) (plist-get elem :url)) eww-history))
+
+;;; Tests:
+
+(ert-deftest eww-test/history/new-page ()
+  "Test that when visiting a new page, the previous one goes into the history."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://one.invalid/")))
+    (eww "three.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://two.invalid/"
+                     "http://one.invalid/")))))
+
+(ert-deftest eww-test/history/back-forward ()
+  "Test that navigating through history just changes our history position.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (let ((url-history '("http://three.invalid/"
+                         "http://two.invalid/"
+                         "http://one.invalid/")))
+      ;; Go back one page.  This should add "three.invalid" to the
+      ;; history, making our position in the list 2.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go back again.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 3))
+      ;; At the beginning of the history, so trying to go back should
+      ;; signal an error.
+      (should-error (eww-back-url))
+      ;; Go forward once.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go forward again.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 1))
+      ;; At the end of the history, so trying to go forward should
+      ;; signal an error.
+      (should-error (eww-forward-url)))))
+
+(ert-deftest eww-test/history/clear-future ()
+  "Test that going to a new page from a historical one clears future history.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (eww-back-url)
+    (eww "four.invalid")
+    (should (equal (eww-test--history-urls) '("http://two.invalid/"
+                                              "http://one.invalid/")))
+    (should (= eww-history-position 0))))
+
+(provide 'eww-tests)
+;; eww-tests.el ends here
-- 
2.25.1

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

[Eli Zaretskii]

> Date: Wed, 28 Feb 2024 15:39:16 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 69232@debbugs.gnu.org, jimjoe@gmx.net
> 
> On 2/25/2024 2:41 PM, Jim Porter wrote:
> > On 2/25/2024 11:49 AM, Eli Zaretskii wrote:
> >>> If anything, I think this should be the default, with some other options
> >>> provided for people who don't want to lose any history. That way the
> >>> default behavior is what people know.
> >>
> >> I don't think I mind.
> > 
> > Thanks. Even if there were multiple options, this is probably what I'd 
> > choose, if only out of habit.
> 
> Here's a patch that deletes "future history" in the case we'd previously 
> discussed. I also added some regression tests for this. I think this all 
> works correctly, but it's probably worth some manual testing over a few 
> days just to be on the safe side.

Thanks, but I thought we were talking about some user option, since at
least some people said they don't like what other browsers do?

> +(defun eww-save-history (&optional clear-future)
> +  "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.
> +
> +If CLEAR-FUTURE is non-nil, clear any future history elements from
> +`eww-history'."

This should probably explain what it means by "future history
elements".

Thanks.

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

[Jim Porter]

On 2/28/2024 11:03 PM, Eli Zaretskii wrote:
> Thanks, but I thought we were talking about some user option, since at
> least some people said they don't like what other browsers do?

I'll wait to see if James has anything to say about this patch, but my 
understanding was that his problem was that the first version of my 
patch *didn't* work like other browsers, and he wanted something closer 
to that.

I don't mind adding an option though, once we have an idea of what 
options we'd want to support. One simple way might be to add some option 
like 'eww-history-replacement-function' (name suggestions welcome), 
which runs any time the user is at a historical page and navigates to a 
new one. This would default to the hypothetical function 
'eww-history-delete-future' and do what my latest patch does. Then users 
can write their own functions to modify the behavior.

It would also be nice to have an option like the Emacs 29 behavior, but 
with the bug in my original report still fixed. I'm not sure exactly the 
best implementation for this yet though...

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 2/28/2024 11:03 PM, Eli Zaretskii wrote:
>> Thanks, but I thought we were talking about some user option, since at
>> least some people said they don't like what other browsers do?
>
> I'll wait to see if James has anything to say about this patch, but my
> understanding was that his problem was that the first version of my
> patch *didn't* work like other browsers, and he wanted something
> closer to that.

The current patch is much better for me personally: 'l' and 'r' now do
what they're supposed to do. But my ideal (short of any advanced 'tree'
mechanism), as I originally stated, would've been to _insert_ (rather
than _replace_) the new history at the position in the current history
where it's created (but I see that there's no SOP for that in (info
"(elisp) Minibuffer History"), and that there could be performance
implications).

> I don't mind adding an option though, once we have an idea of what
> options we'd want to support. One simple way might be to add some
> option like 'eww-history-replacement-function' (name suggestions
> welcome), which runs any time the user is at a historical page and
> navigates to a new one. This would default to the hypothetical
> function 'eww-history-delete-future' and do what my latest patch does.
> Then users can write their own functions to modify the behavior.

Why not simply make 'eww-save-history' customizable?

> It would also be nice to have an option like the Emacs 29 behavior,
> but with the bug in my original report still fixed. I'm not sure
> exactly the best implementation for this yet though...

TBH I don't think anyone would have been (ab)using it effectively
because each 'l' or 'r' made things more complicated; but the advantage
that *all* of history was available with 'H'.

(I'm using this patch and will let you know if I see anything amiss)

Regards,
James

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

[Jim Porter]

On 2/29/2024 3:30 PM, James Thomas via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> The current patch is much better for me personally: 'l' and 'r' now do
> what they're supposed to do. But my ideal (short of any advanced 'tree'
> mechanism), as I originally stated, would've been to _insert_ (rather
> than _replace_) the new history at the position in the current history
> where it's created (but I see that there's no SOP for that in (info
> "(elisp) Minibuffer History"), and that there could be performance
> implications).

That shouldn't be too hard to do, at the very least with the appropriate 
hooks (i.e. you might need to write some Elisp depending on how many 
built-in options we want to support, but you won't have to use 
'advice-add').

> Why not simply make 'eww-save-history' customizable?

In a general sense, that's the idea. I don't want to make 
'eww-save-history' itself customizable though, since a) I want to let 
people define a history pruning/fixup function and b) I don't want that 
customizable function to have to be responsible for saving the current 
page's history; 'eww-save-history' can do that for us. In practice 
though, I think it'll all work out about the same, albeit easier to use.

> TBH I don't think anyone would have been (ab)using it effectively
> because each 'l' or 'r' made things more complicated; but the advantage
> that *all* of history was available with 'H'.

Yeah, I think the main thing we need here is some option to prevent loss 
of existing history.

> (I'm using this patch and will let you know if I see anything amiss)

I already found one issue with reloading messing up history, but that 
was an easy fix. Once I finish up the other parts of my v3 patch, I'll 
post it here.

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

[Jim Porter]

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

On 2/29/2024 5:00 PM, Jim Porter wrote:
> I already found one issue with reloading messing up history, but that 
> was an easy fix. Once I finish up the other parts of my v3 patch, I'll 
> post it here.

I've only lightly tested this so far, but this version adds an 
'eww-before-browse-function' option which lets you customize how EWW 
manipulates history before browsing to a new page. I've added functions 
for the default behavior ('eww-clear-future-history'), and for cloning 
all the "parent" entries ('eww-clone-previous-history')[1]. You can also 
set it to 'ignore', which will just preserve the old entries and add the 
new page to the end (which is the behavior v1 of my patch had).

I've also added more regression tests to make sure this all works as 
expected.

How does this look?

[1] See the docstring for a longer explanation of how this works.

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

From 6391bcf13fe0a26aba67ef3e77a51ec7b9ca6cc1 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-before-browse-function): New option.
(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--before-browse): New function...
(eww, eww-follow-link, eww-readable): ... call it.
(eww-render): Don't set 'eww-history-position' here...
(eww--before-browse): ... instead, set it here.
(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.
(eww-clear-future-history, eww-clone-previous-history): New functions.

* test/lisp/net/eww-tests.el: New file.

* etc/NEWS: Announce this change.
---
 etc/NEWS                   |  12 +++
 lisp/net/eww.el            |  84 ++++++++++++++---
 test/lisp/net/eww-tests.el | 179 +++++++++++++++++++++++++++++++++++++
 3 files changed, 263 insertions(+), 12 deletions(-)
 create mode 100644 test/lisp/net/eww-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index 6d444daf152..62f360bc112 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1004,6 +1004,18 @@ 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 duplicate entry to the end of the history list each time.
+This made it impossible to navigate to the "end" of the history list.
+Now, navigating through history in EWW simply changes your position in
+the history list, allowing you to reach the end as expected.  In
+addition, when navigating to a new page from a historical one, EWW
+deletes the history entries after the current page.  To change the
+behavior when navigating from historical pages, you can customize
+'eww-before-browse-function'.
+
 ** go-ts-mode
 
 +++
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 5a25eef9e3c..8c82f92af2a 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -182,6 +182,14 @@ eww-browse-url-new-window-is-tab
                  (const :tag "Open new tab when tab bar is enabled" tab-bar)
                  (const :tag "Never open URL in new tab" nil)))
 
+(defcustom eww-before-browse-function #'eww-clear-future-history
+  "A function to call before browsing to a new page.
+By default, this removes any entries in the history that come after the
+current page (see `eww-clear-future-history')."
+  :version "30.1"
+  :group 'eww
+  :type 'function)
+
 (defcustom eww-after-render-hook nil
   "A hook called after eww has finished rendering the buffer."
   :version "25.1"
@@ -312,7 +320,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"
@@ -402,6 +413,7 @@ eww
     (t
      (get-buffer-create "*eww*"))))
   (eww-setup-buffer)
+  (eww--before-browse)
   ;; Check whether the domain only uses "Highly Restricted" Unicode
   ;; IDNA characters.  If not, transform to punycode to indicate that
   ;; there may be funny business going on.
@@ -654,7 +666,6 @@ eww-render
 	    (with-current-buffer buffer
 	      (plist-put eww-data :url url)
 	      (eww--after-page-change)
-	      (setq eww-history-position 0)
 	      (and last-coding-system-used
 		   (set-buffer-file-coding-system last-coding-system-used))
               (unless shr-fill-text
@@ -905,6 +916,11 @@ eww-update-header-line-format
 		 `((?u . ,(or url ""))
 		   (?t . ,title))))))))
 
+(defun eww--before-browse ()
+  (funcall eww-before-browse-function)
+  (setq eww-history-position 0
+        eww-data (list :title "")))
+
 (defun eww--after-page-change ()
   (eww-update-header-line-format)
   (eww--rename-buffer))
@@ -1037,6 +1053,7 @@ eww-readable
          (base (plist-get eww-data :url)))
     (eww-score-readability dom)
     (eww-save-history)
+    (eww--before-browse)
     (eww-display-html nil nil
                       (list 'base (list (cons 'href base))
                             (eww-highest-readability dom))
@@ -1129,9 +1146,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 +1172,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 +1297,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)
@@ -1959,6 +1980,7 @@ eww-follow-link
 	   (eww-same-page-p url (plist-get eww-data :url)))
       (let ((point (point)))
 	(eww-save-history)
+        (eww--before-browse)
 	(plist-put eww-data :url url)
         (goto-char (point-min))
         (if-let ((match (text-property-search-forward 'shr-target-id target #'member)))
@@ -2289,11 +2311,49 @@ 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 "")))
+  (if (zerop eww-history-position)
+      (let ((history-delete-duplicates nil))
+        (add-to-history 'eww-history eww-data eww-history-limit t)
+        (setq eww-history-position 1)
+        t)
+    (setf (elt eww-history (1- eww-history-position)) eww-data)
+    nil))
+
+(defun eww-clear-future-history ()
+  "Remove any entries in `eww-history' after the currently-shown one.
+This is useful for `eww-before-browse-function' to make EWW's navigation
+to a new page from a historical one work like other web browsers: it
+will clear any \"future\" history elements before adding the new page to
+the end of the history."
+  (when (> eww-history-position 1)
+    (setq eww-history (nthcdr (1- eww-history-position) eww-history))))
+
+(defun eww-clone-previous-history ()
+  "Clone and prepend entries in `eww-history' up to the currently-shown one.
+These cloned entries get added to the beginning of `eww-history' so that
+it's possible to navigate back to the very first page for this EWW
+without deleting any history entries.
+
+For example, if `eww-history' looks like this (going from oldest to
+newest, with \"*\" marking the current page):
+
+  A B C* D E
+
+then calling this function updates `eww-history' to:
+
+  A B C D E A B C*
+
+This is useful for setting `eww-before-browse-function' (which see)."
+  (when (> eww-history-position 1)
+    (setq eww-history (append (nthcdr (1- eww-history-position) eww-history)
+                              eww-history))))
 
 (defvar eww-current-buffer)
 
diff --git a/test/lisp/net/eww-tests.el b/test/lisp/net/eww-tests.el
new file mode 100644
index 00000000000..8e056521fc7
--- /dev/null
+++ b/test/lisp/net/eww-tests.el
@@ -0,0 +1,179 @@
+;;; eww-tests.el --- tests for eww.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'eww)
+
+(defvar eww-test--response-function (lambda (url) (concat "\n" url))
+  "A function for returning a mock response for URL.
+The default just returns an empty list of headers URL as the body.")
+
+(defmacro eww-test--with-mock-retrieve (&rest body)
+  "Evaluate BODY with a mock implementation of `eww-retrieve'.
+This avoids network requests during our tests.  Additionally, prepare a
+temporary EWW buffer for our tests."
+  (declare (indent 1))
+    `(cl-letf (((symbol-function 'eww-retrieve)
+                (lambda (url callback args)
+                  (with-temp-buffer
+                    (insert (funcall eww-test--response-function url))
+                    (apply callback nil args)))))
+       (with-temp-buffer
+         (eww-mode)
+         ,@body)))
+
+(defun eww-test--history-urls ()
+  (mapcar (lambda (elem) (plist-get elem :url)) eww-history))
+
+;;; Tests:
+
+(ert-deftest eww-test/history/new-page ()
+  "Test that when visiting a new page, the previous one goes into the history."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://one.invalid/")))
+    (eww "three.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://two.invalid/"
+                     "http://one.invalid/")))))
+
+(ert-deftest eww-test/history/back-forward ()
+  "Test that navigating through history just changes our history position.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (let ((url-history '("http://three.invalid/"
+                         "http://two.invalid/"
+                         "http://one.invalid/")))
+      ;; Go back one page.  This should add "three.invalid" to the
+      ;; history, making our position in the list 2.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go back again.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 3))
+      ;; At the beginning of the history, so trying to go back should
+      ;; signal an error.
+      (should-error (eww-back-url))
+      ;; Go forward once.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go forward again.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 1))
+      ;; At the end of the history, so trying to go forward should
+      ;; signal an error.
+      (should-error (eww-forward-url)))))
+
+(ert-deftest eww-test/history/reload-in-place ()
+  "Test that reloading historical pages updates their history entry in-place.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (eww-back-url)
+    ;; Make sure our history has the original page text.
+    (should (equal (plist-get (nth 1 eww-history) :text)
+                   "http://two.invalid/"))
+    (should (= eww-history-position 2))
+    ;; Reload the page.
+    (let ((eww-test--response-function
+           (lambda (url) (concat "\nreloaded " url))))
+      (eww-reload)
+      (should (= eww-history-position 2)))
+    ;; Go to another page, and make sure the history is correct,
+    ;; including the reloaded page text.
+    (eww "four.invalid")
+    (should (equal (eww-test--history-urls) '("http://two.invalid/"
+                                              "http://one.invalid/")))
+    (should (equal (plist-get (nth 0 eww-history) :text)
+                   "reloaded http://two.invalid/"))
+    (should (= eww-history-position 0))))
+
+(ert-deftest eww-test/history/before-navigate/clear-future-history ()
+  "Test that going to a new page from a historical one clears future history.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (eww-back-url)
+    (eww "four.invalid")
+    (eww "five.invalid")
+    (should (equal (eww-test--history-urls) '("http://four.invalid/"
+                                              "http://two.invalid/"
+                                              "http://one.invalid/")))
+    (should (= eww-history-position 0))))
+
+(ert-deftest eww-test/history/before-navigate/ignore-history ()
+  "Test that going to a new page from a historical one preserves history.
+This sets `eww-before-browse-function' to `ignore' to preserve history.
+See bug#69232."
+  (let ((eww-before-browse-function #'ignore))
+    (eww-test--with-mock-retrieve
+      (eww "one.invalid")
+      (eww "two.invalid")
+      (eww "three.invalid")
+      (eww-back-url)
+      (eww "four.invalid")
+      (eww "five.invalid")
+      (should (equal (eww-test--history-urls) '("http://four.invalid/"
+                                                "http://three.invalid/"
+                                                "http://two.invalid/"
+                                                "http://one.invalid/")))
+      (should (= eww-history-position 0)))))
+
+(ert-deftest eww-test/history/before-navigate/clone-previous ()
+  "Test that going to a new page from a historical one clones prior history.
+This sets `eww-before-browse-function' to `eww-clone-previous-history'
+to clone the history.  See bug#69232."
+  (let ((eww-before-browse-function #'eww-clone-previous-history))
+    (eww-test--with-mock-retrieve
+      (eww "one.invalid")
+      (eww "two.invalid")
+      (eww "three.invalid")
+      (eww-back-url)
+      (eww "four.invalid")
+      (eww "five.invalid")
+      (should (equal (eww-test--history-urls)
+                     '(;; New page and cloned history.
+                       "http://four.invalid/"
+                       "http://two.invalid/"
+                       "http://one.invalid/"
+                       ;; Original history.
+                       "http://three.invalid/"
+                       "http://two.invalid/"
+                       "http://one.invalid/")))
+      (should (= eww-history-position 0)))))
+
+(provide 'eww-tests)
+;; eww-tests.el ends here
-- 
2.25.1

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

[Eli Zaretskii]

> Date: Thu, 29 Feb 2024 18:10:19 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 69232@debbugs.gnu.org
> 
> On 2/29/2024 5:00 PM, Jim Porter wrote:
> > I already found one issue with reloading messing up history, but that 
> > was an easy fix. Once I finish up the other parts of my v3 patch, I'll 
> > post it here.
> 
> I've only lightly tested this so far, but this version adds an 
> 'eww-before-browse-function' option which lets you customize how EWW 
> manipulates history before browsing to a new page. I've added functions 
> for the default behavior ('eww-clear-future-history'), and for cloning 
> all the "parent" entries ('eww-clone-previous-history')[1]. You can also 
> set it to 'ignore', which will just preserve the old entries and add the 
> new page to the end (which is the behavior v1 of my patch had).
> 
> I've also added more regression tests to make sure this all works as 
> expected.
> 
> How does this look?

Thanks.  I have a usability comment below.

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1004,6 +1004,18 @@ 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 duplicate entry to the end of the history list each time.
> +This made it impossible to navigate to the "end" of the history list.
> +Now, navigating through history in EWW simply changes your position in
> +the history list, allowing you to reach the end as expected.  In
> +addition, when navigating to a new page from a historical one, EWW

"historical" here should be in quotes (and perhaps explained in
parentheses), otherwise people might misinterpret or fail to
understand what it alludes to.

> +(defcustom eww-before-browse-function #'eww-clear-future-history
> +  "A function to call before browsing to a new page.
> +By default, this removes any entries in the history that come after the
> +current page (see `eww-clear-future-history')."

The doc string should describe the known possible values of the
option.

Now for the usability issue: It's okay to have this is a variable (to
allow future extensions, which might be unrelated to history), but
having a function for user option, and one that is more general than
being about history traversal, is not the best ides: it makes
customization harder for users who are not Lisp programmers.

Therefore, I suggest a history-specific defcustom, whose possible
values could be 'clear', 'clone', and 'add', and whose effect will be
to call the corresponding function via eww-before-browse-function.
The defcustom could also have a user-defined function value to allow
additional functions, of course.

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 2/29/2024 5:00 PM, Jim Porter wrote:
>> I already found one issue with reloading messing up history, but
>> that was an easy fix. Once I finish up the other parts of my v3
>> patch, I'll post it here.
>
> I've only lightly tested this so far, but this version adds an
> 'eww-before-browse-function' option which lets you customize how EWW
> manipulates history before browsing to a new page. I've added
> functions for the default behavior ('eww-clear-future-history'), and
> for cloning all the "parent" entries
> ('eww-clone-previous-history')[1]. You can also set it to 'ignore',
> which will just preserve the old entries and add the new page to the
> end (which is the behavior v1 of my patch had).
>
> I've also added more regression tests to make sure this all works as
> expected.
>
> How does this look?

I'm using it with the following:

--8<---------------cut here---------------start------------->8---

(defun eww-clone-previous-history-upto-first ()
  "Like `eww-clone-previous-history', but only clone up to the first
occurrence of the current page in the history."
  (when (> eww-history-position 1)
    (setq eww-history
	  (append
	   (cl-subseq
	    eww-history
	    (- 0 (cl-position-if
		  (lambda (elt) (equal (plist-get elt :url) (eww-current-url)))
		  eww-history :from-end)
	       1))
           eww-history))))

(setq eww-before-browse-function #'eww-clone-previous-history-upto-first)

--8<---------------cut here---------------end--------------->8---

...for getting the behaviour I'd referred to earlier:

> ... so because that might sometimes be too much, (adding to the end)
> only the entire history up to the first occurrence of the source page.

Will report back if I see any problems.

Regards,
James

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

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

James Thomas wrote:

> I'm using it with the following:

It had a small error; corrected version:

--8<---------------cut here---------------start------------->8---

(defun eww-clone-previous-history-upto-first ()
  "Like `eww-clone-previous-history', but only clone up to the first
occurrence of the current page in the history."
  (when (> eww-history-position 1)
    (setq eww-history
	  (append
	   (cl-subseq
	    eww-history
	    (cl-position-if
	     (lambda (elt) (equal (plist-get elt :url) (eww-current-url)))
	     eww-history :from-end))
           eww-history))))

(setq eww-before-browse-function #'eww-clone-previous-history-upto-first)

--8<---------------cut here---------------end--------------->8---

Regards,
James

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

[Jim Porter]

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

On 2/29/2024 11:26 PM, Eli Zaretskii wrote:
> Thanks.  I have a usability comment below.

Thanks for taking a look.

> "historical" here should be in quotes (and perhaps explained in
> parentheses), otherwise people might misinterpret or fail to
> understand what it alludes to.

Done. I've also added a short description about this to the EWW manual 
so that people in the future know they can tweak this behavior.

>> +(defcustom eww-before-browse-function #'eww-clear-future-history
>> +  "A function to call before browsing to a new page.
>> +By default, this removes any entries in the history that come after the
>> +current page (see `eww-clear-future-history')."
> 
> The doc string should describe the known possible values of the
> option.

Done.

> Now for the usability issue: It's okay to have this is a variable (to
> allow future extensions, which might be unrelated to history), but
> having a function for user option, and one that is more general than
> being about history traversal, is not the best ides: it makes
> customization harder for users who are not Lisp programmers.

After sleeping on this, I think I agree. I had debated whether to add 
something like 'eww-before-browse-hook', but then I reasoned that I 
really want exactly *one* of these history-modification functions to run 
before browsing, so I made it '...-function'. But that's not right 
either, since it would mess things up if someone wanted to use this as a 
hook (they'd need to write their own Lisp function to do everything).

There might be some use in the future for 'eww-before-browse-hook', but 
I can't think of any off the top of my head, so I've just changed this 
to 'eww-before-browse-history-function'. If we want a more general hook 
later, we can always add it when we know more.

> Therefore, I suggest a history-specific defcustom, whose possible
> values could be 'clear', 'clone', and 'add', and whose effect will be
> to call the corresponding function via eww-before-browse-function.
> The defcustom could also have a user-defined function value to allow
> additional functions, of course.

I've done something close to this, though I retained the previous form 
where the possible values are things like 'eww-delete-future-history'. 
That keeps the code to *use* this option simpler (no special-case symbol 
names), and the only difference with your suggestion is that users who 
manually edit their init.el will have a longer symbol name to type.

For Customize users, I added 'function-item' choices so they should just 
be able to click on the option they want without having to worry about 
the exact symbol name.

If you strongly prefer to have this accept non-function symbol names 
like 'clear', let me know and I can change it. I don't care too much, 
but this way seemed simpler and easier to maintain.

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

From 13b9709c76283967a0e273922641685f440683e3 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-before-browse-history-function): New option.
(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--before-browse): New function...
(eww, eww-follow-link, eww-readable): ... call it.
(eww-render): Don't set 'eww-history-position' here...
(eww--before-browse): ... instead, set it here.
(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.
(eww-delete-future-history, eww-clone-previous-history): New functions.

* test/lisp/net/eww-tests.el: New file.

* etc/NEWS: Announce this change.
---
 doc/misc/eww.texi          |   9 ++
 etc/NEWS                   |  13 +++
 lisp/net/eww.el            | 123 ++++++++++++++++++++++---
 test/lisp/net/eww-tests.el | 179 +++++++++++++++++++++++++++++++++++++
 4 files changed, 312 insertions(+), 12 deletions(-)
 create mode 100644 test/lisp/net/eww-tests.el

diff --git a/doc/misc/eww.texi b/doc/misc/eww.texi
index 5e69b11d347..d31fcf1802b 100644
--- a/doc/misc/eww.texi
+++ b/doc/misc/eww.texi
@@ -192,6 +192,15 @@ Basics
 buffer @file{*eww history*}.  The history is lost when EWW is quit.
 If you want to remember websites you can use bookmarks.
 
+@vindex eww-before-browse-history-function
+  By default, when browsing to a new page from a ``historical'' one
+(i.e.@: a page loaded by navigating back via @code{eww-back-url}), EWW
+will first delete any history entries newer than the current page.  This
+is the same behavior as most other web browsers.  You can change this by
+customizing @code{eww-before-browse-history-function} to another value.
+For example, setting it to @code{ignore} will preserve the existing
+history entries and simply prepend the new page to the history list.
+
 @vindex eww-history-limit
   Along with the URLs visited, EWW also remembers both the rendered
 page (as it appears in the buffer) and its source.  This can take a
diff --git a/etc/NEWS b/etc/NEWS
index 6d444daf152..7e80f83d231 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1004,6 +1004,19 @@ 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 duplicate entry to the end of the history list each time.
+This made it impossible to navigate to the "end" of the history list.
+Now, navigating through history in EWW simply changes your position in
+the history list, allowing you to reach the end as expected.  In
+addition, when browsing to a new page from a "historical" one (i.e. a
+page loaded by navigating back through history), EWW deletes the history
+entries newer than the current page.  To change the behavior when
+browsing from "historical" pages, you can customize
+'eww-before-browse-history-function'.
+
 ** go-ts-mode
 
 +++
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 5a25eef9e3c..6e31687f071 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -182,6 +182,33 @@ eww-browse-url-new-window-is-tab
                  (const :tag "Open new tab when tab bar is enabled" tab-bar)
                  (const :tag "Never open URL in new tab" nil)))
 
+(defcustom eww-before-browse-history-function #'eww-delete-future-history
+  "A function to call to update history before browsing to a new page.
+EWW provides the following values for this option:
+
+* `eww-delete-future-history': Delete any history entries after the
+currently-shown one.  This is the default behavior, and works the same
+as in most other web browsers.
+
+* `eww-clone-previous-history': Clone and prepend any history entries up
+  to the currently-shown one.  This is like `eww-delete-future-history',
+  except that it preserves the previous contents of the history list at
+  the end.
+
+* `ignore': Preserve the current history unchanged.  This will result in
+  the new page simply being prepended to the existing history list.
+
+You can also set this to any other function you wish."
+  :version "30.1"
+  :group 'eww
+  :type '(choice (function-item :tag "Delete future history"
+                                eww-delete-future-history)
+                 (function-item :tag "Clone previous history"
+                                eww-clone-previous-history)
+                 (function-item :tag "Preserve history"
+                                ignore)
+                 (function :tag "Custom function")))
+
 (defcustom eww-after-render-hook nil
   "A hook called after eww has finished rendering the buffer."
   :version "25.1"
@@ -312,7 +339,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"
@@ -402,6 +432,7 @@ eww
     (t
      (get-buffer-create "*eww*"))))
   (eww-setup-buffer)
+  (eww--before-browse)
   ;; Check whether the domain only uses "Highly Restricted" Unicode
   ;; IDNA characters.  If not, transform to punycode to indicate that
   ;; there may be funny business going on.
@@ -654,7 +685,6 @@ eww-render
 	    (with-current-buffer buffer
 	      (plist-put eww-data :url url)
 	      (eww--after-page-change)
-	      (setq eww-history-position 0)
 	      (and last-coding-system-used
 		   (set-buffer-file-coding-system last-coding-system-used))
               (unless shr-fill-text
@@ -905,6 +935,11 @@ eww-update-header-line-format
 		 `((?u . ,(or url ""))
 		   (?t . ,title))))))))
 
+(defun eww--before-browse ()
+  (funcall eww-before-browse-history-function)
+  (setq eww-history-position 0
+        eww-data (list :title "")))
+
 (defun eww--after-page-change ()
   (eww-update-header-line-format)
   (eww--rename-buffer))
@@ -1037,6 +1072,7 @@ eww-readable
          (base (plist-get eww-data :url)))
     (eww-score-readability dom)
     (eww-save-history)
+    (eww--before-browse)
     (eww-display-html nil nil
                       (list 'base (list (cons 'href base))
                             (eww-highest-readability dom))
@@ -1129,9 +1165,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 +1191,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 +1316,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)
@@ -1959,6 +1999,7 @@ eww-follow-link
 	   (eww-same-page-p url (plist-get eww-data :url)))
       (let ((point (point)))
 	(eww-save-history)
+        (eww--before-browse)
 	(plist-put eww-data :url url)
         (goto-char (point-min))
         (if-let ((match (text-property-search-forward 'shr-target-id target #'member)))
@@ -2289,11 +2330,69 @@ 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 "")))
+  (if (zerop eww-history-position)
+      (let ((history-delete-duplicates nil))
+        (add-to-history 'eww-history eww-data eww-history-limit t)
+        (setq eww-history-position 1)
+        t)
+    (setf (elt eww-history (1- eww-history-position)) eww-data)
+    nil))
+
+(defun eww-delete-future-history ()
+  "Remove any entries in `eww-history' after the currently-shown one.
+This is useful for `eww-before-browse-history-function' to make EWW's
+navigation to a new page from a historical one work like other web
+browsers: it will delete any \"future\" history elements before adding
+the new page to the end of the history.
+
+For example, if `eww-history' looks like this (going from newest to
+oldest, with \"*\" marking the current page):
+
+  E D C* B A
+
+then calling this function updates `eww-history' to:
+
+  C* B A"
+  (when (> eww-history-position 1)
+    (setq eww-history (nthcdr (1- eww-history-position) eww-history)
+          ;; We don't really need to set this since `eww--before-browse'
+          ;; sets it too, but this ensures that other callers can use
+          ;; this function and get the expected results.
+          eww-history-position 1)))
+
+(defun eww-clone-previous-history ()
+  "Clone and prepend entries in `eww-history' up to the currently-shown one.
+These cloned entries get added to the beginning of `eww-history' so that
+it's possible to navigate back to the very first page for this EWW
+without deleting any history entries.
+
+For example, if `eww-history' looks like this (going from newest to
+oldest, with \"*\" marking the current page):
+
+  E D C* B A
+
+then calling this function updates `eww-history' to:
+
+  C* B A E D C D A
+
+This is useful for setting `eww-before-browse-history-function' (which
+see)."
+  (when (> eww-history-position 1)
+    (setq eww-history (take eww-history-limit
+                            (append (nthcdr (1- eww-history-position)
+                                            eww-history)
+                                    eww-history))
+          ;; As with `eww-delete-future-history', we don't really need
+          ;; to set this since `eww--before-browse' sets it too, but
+          ;; let's be thorough.
+          eww-history-position 1)))
 
 (defvar eww-current-buffer)
 
diff --git a/test/lisp/net/eww-tests.el b/test/lisp/net/eww-tests.el
new file mode 100644
index 00000000000..ced84322e3a
--- /dev/null
+++ b/test/lisp/net/eww-tests.el
@@ -0,0 +1,179 @@
+;;; eww-tests.el --- tests for eww.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'eww)
+
+(defvar eww-test--response-function (lambda (url) (concat "\n" url))
+  "A function for returning a mock response for URL.
+The default just returns an empty list of headers URL as the body.")
+
+(defmacro eww-test--with-mock-retrieve (&rest body)
+  "Evaluate BODY with a mock implementation of `eww-retrieve'.
+This avoids network requests during our tests.  Additionally, prepare a
+temporary EWW buffer for our tests."
+  (declare (indent 1))
+    `(cl-letf (((symbol-function 'eww-retrieve)
+                (lambda (url callback args)
+                  (with-temp-buffer
+                    (insert (funcall eww-test--response-function url))
+                    (apply callback nil args)))))
+       (with-temp-buffer
+         (eww-mode)
+         ,@body)))
+
+(defun eww-test--history-urls ()
+  (mapcar (lambda (elem) (plist-get elem :url)) eww-history))
+
+;;; Tests:
+
+(ert-deftest eww-test/history/new-page ()
+  "Test that when visiting a new page, the previous one goes into the history."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://one.invalid/")))
+    (eww "three.invalid")
+    (should (equal (eww-test--history-urls)
+                   '("http://two.invalid/"
+                     "http://one.invalid/")))))
+
+(ert-deftest eww-test/history/back-forward ()
+  "Test that navigating through history just changes our history position.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (let ((url-history '("http://three.invalid/"
+                         "http://two.invalid/"
+                         "http://one.invalid/")))
+      ;; Go back one page.  This should add "three.invalid" to the
+      ;; history, making our position in the list 2.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go back again.
+      (eww-back-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 3))
+      ;; At the beginning of the history, so trying to go back should
+      ;; signal an error.
+      (should-error (eww-back-url))
+      ;; Go forward once.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 2))
+      ;; Go forward again.
+      (eww-forward-url)
+      (should (equal (eww-test--history-urls) url-history))
+      (should (= eww-history-position 1))
+      ;; At the end of the history, so trying to go forward should
+      ;; signal an error.
+      (should-error (eww-forward-url)))))
+
+(ert-deftest eww-test/history/reload-in-place ()
+  "Test that reloading historical pages updates their history entry in-place.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (eww-back-url)
+    ;; Make sure our history has the original page text.
+    (should (equal (plist-get (nth 1 eww-history) :text)
+                   "http://two.invalid/"))
+    (should (= eww-history-position 2))
+    ;; Reload the page.
+    (let ((eww-test--response-function
+           (lambda (url) (concat "\nreloaded " url))))
+      (eww-reload)
+      (should (= eww-history-position 2)))
+    ;; Go to another page, and make sure the history is correct,
+    ;; including the reloaded page text.
+    (eww "four.invalid")
+    (should (equal (eww-test--history-urls) '("http://two.invalid/"
+                                              "http://one.invalid/")))
+    (should (equal (plist-get (nth 0 eww-history) :text)
+                   "reloaded http://two.invalid/"))
+    (should (= eww-history-position 0))))
+
+(ert-deftest eww-test/history/before-navigate/delete-future-history ()
+  "Test that going to a new page from a historical one deletes future history.
+See bug#69232."
+  (eww-test--with-mock-retrieve
+    (eww "one.invalid")
+    (eww "two.invalid")
+    (eww "three.invalid")
+    (eww-back-url)
+    (eww "four.invalid")
+    (eww "five.invalid")
+    (should (equal (eww-test--history-urls) '("http://four.invalid/"
+                                              "http://two.invalid/"
+                                              "http://one.invalid/")))
+    (should (= eww-history-position 0))))
+
+(ert-deftest eww-test/history/before-navigate/ignore-history ()
+  "Test that going to a new page from a historical one preserves history.
+This sets `eww-before-browse-history-function' to `ignore' to preserve
+history.  See bug#69232."
+  (let ((eww-before-browse-history-function #'ignore))
+    (eww-test--with-mock-retrieve
+      (eww "one.invalid")
+      (eww "two.invalid")
+      (eww "three.invalid")
+      (eww-back-url)
+      (eww "four.invalid")
+      (eww "five.invalid")
+      (should (equal (eww-test--history-urls) '("http://four.invalid/"
+                                                "http://three.invalid/"
+                                                "http://two.invalid/"
+                                                "http://one.invalid/")))
+      (should (= eww-history-position 0)))))
+
+(ert-deftest eww-test/history/before-navigate/clone-previous ()
+  "Test that going to a new page from a historical one clones prior history.
+This sets `eww-before-browse-history-function' to
+`eww-clone-previous-history' to clone the history.  See bug#69232."
+  (let ((eww-before-browse-history-function #'eww-clone-previous-history))
+    (eww-test--with-mock-retrieve
+      (eww "one.invalid")
+      (eww "two.invalid")
+      (eww "three.invalid")
+      (eww-back-url)
+      (eww "four.invalid")
+      (eww "five.invalid")
+      (should (equal (eww-test--history-urls)
+                     '(;; New page and cloned history.
+                       "http://four.invalid/"
+                       "http://two.invalid/"
+                       "http://one.invalid/"
+                       ;; Original history.
+                       "http://three.invalid/"
+                       "http://two.invalid/"
+                       "http://one.invalid/")))
+      (should (= eww-history-position 0)))))
+
+(provide 'eww-tests)
+;; eww-tests.el ends here
-- 
2.25.1

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

[Eli Zaretskii]

> Date: Fri, 1 Mar 2024 12:13:08 -0800
> Cc: 69232@debbugs.gnu.org, jimjoe@gmx.net
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > Therefore, I suggest a history-specific defcustom, whose possible
> > values could be 'clear', 'clone', and 'add', and whose effect will be
> > to call the corresponding function via eww-before-browse-function.
> > The defcustom could also have a user-defined function value to allow
> > additional functions, of course.
> 
> I've done something close to this, though I retained the previous form 
> where the possible values are things like 'eww-delete-future-history'. 
> That keeps the code to *use* this option simpler (no special-case symbol 
> names), and the only difference with your suggestion is that users who 
> manually edit their init.el will have a longer symbol name to type.
> 
> For Customize users, I added 'function-item' choices so they should just 
> be able to click on the option they want without having to worry about 
> the exact symbol name.
> 
> If you strongly prefer to have this accept non-function symbol names 
> like 'clear', let me know and I can change it. I don't care too much, 
> but this way seemed simpler and easier to maintain.

I think what you did is functionally equivalent, and not harder to use
in practice.  So I'm okay with this version of the patch.  Thanks.

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

[Jim Porter]

On 3/1/2024 11:38 PM, Eli Zaretskii wrote:
> I think what you did is functionally equivalent, and not harder to use
> in practice.  So I'm okay with this version of the patch.  Thanks.

As it's been a couple of weeks since I posted about this on emacs-devel, 
and there haven't been any further comments, I've merged this to master 
as 59e470dd5de. If there are any followup issues about this though, just 
let me know.