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

[Jim Porter <jporterbugs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 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