Subject: bug#69941: 30.0.50; Faulty fontification of radio button widgets

From: Mauro Aranda <[email-masked]>
Date: Sat, 8 Feb 2025 20:50:20 -0300

Stephen Berman <[email-masked]> writes:

 > With the attached file I can reproduce the problem.  Save the file and
 > then invoke Emacs from master (i.e., with your patch) as follows:
 >
 > emacs -Q -l srb-widget-test.el -f srb-widget-test
 >
 > In the displayed buffer "*Widget Test*" the three radio buttons "One",
 > "Two" and "Confirm" are active, which the default face visually
 > confirms.  (Their labels are fontified with face `widget-unselected'.)
 > Now do the following steps:
 >
 > 1. Hit TAB to move point to radio button "One".
 > 2. Type RET to activate the "Reset" button and move point to it.  Also,
 >    the radio buttons are now deactivated, which is indicated by their
 >    now being fontified with face `widget-inactive' (as are their
 >    labels).
 > 3. Type RET on the "Reset" button to reactivate the radio buttons.
 > 4. Hit TAB twice to move point to radio button "Two". Hitting RET
 >    results in the same state as in step 2.
 > 5. Repeat step 3 and then hit TAB three times (or S-TAB once) to move
 >    point to radio button "Confirm".  This pops up two indented radio
 >    buttons "Right" and "Wrong" under "Confirm", and point is now on
 >    "Right".  Note that the radio buttons "One", "Two" and "Confirm" are
 >    deactivated, while "Right" and "Wrong" are active.
 > 6. Type RET on radio button "Right".  This deletes radio buttons "Right"
 >    and "Wrong" and moves point to "Reset".  Radio buttons "One", "Two"
 >    and "Confirm" remain deactivated (confirmed by tabbing to each and
 >    typing RET, which dings and displays the message "Attempt to perform
 >    action on inactive widget"); however:
 > => Now radio button "One" is misfontified with the default face instead
 >    of face `widget-inactive'.
 >
 > If you repeat step 5 and then tab to radio button "Wrong" and hit RET,
 > the resulting state is the same as in step 6, i.e., radio button "One"
 > (and only it) is again misfontified.

OK, it's a similar problem, but this time with the :inactive overlay,
which has FRONT-ADVANCE t.

 > ;;; -*- lexical-binding: t; -*-
 >
 > (defvar srb--radio-widget)
 > (defvar srb--confirm-widget)
 > (defvar srb--reset-button)
 >
 > (defun srb--result (widget)
 >   (pcase (widget-value widget)
 >       ("One" (ignore))
 >       ("Two" (ignore))
 >       ("Confirm"
 >        (goto-char (widget-get srb--radio-widget :to))
 >        (setq srb--confirm-widget
 >          (widget-create
 >           'radio-button-choice
 >           :indent 4
 >           :notify (lambda (child &rest _)
 >             (srb--confirm child))
 >           '(item "Right")
 >           '(item "Wrong")))))
 >   (when srb--reset-button
 >     (widget-apply srb--reset-button :activate))
 >   (if (equal (widget-value srb--radio-widget) "Confirm")
 >       (widget-backward 2)
 >     (goto-char (widget-get srb--reset-button :from)))
 >   (widget-value-set srb--radio-widget "")
 >   (widget-apply srb--radio-widget :deactivate))

Here, before the :deactivate call the widget is already inactive, and
because of that the :deactivate call (which calls
widget-specify-inactive) does nothing at all, since it detects there's
already an :inactive overlay.  That :inactive overlay doesn't include
the 1st button anymore, since that one gets inserted right at the
overlay start.

So, modifying an inactive widget right at the start or at the end of the
overlay can produce this effect.

I don't think we can solve it easily like my patch did with the :from
and :to markers, though.  But here's an idea:

When a widget is already inactive (i.e., it has an :inactive property),
instead of doing nothing we make sure to move the overlay so that it
includes again all the widget, from the :from marker to the :to marker.

If we do that, we should document that if some code modifies an
:inactive widget, it has to call again :deactivate.

I attach a patch with that idea, together with documentation changes and
a new test.

From 8cdd802ae49c6b4915fc3b333043e1dd3879247e Mon Sep 17 00:00:00 2001
From: Mauro Aranda <[email-masked]>
Date: Sat, 8 Feb 2025 20:49:10 -0300
Subject: [PATCH] Fix bad fontification of inactive widgets

* lisp/wid-edit.el (widget-specify-inactive): When a widget is
already inactive, still move the overlay to the desired
positions.  Improve docstring.  (Bug#69941)

* doc/misc/widget.texi (default): Document the need to call the
:deactivate function if making modifications to an already
inactive widget.

* test/lisp/wid-edit-tests.el
(widget-test-modification-of-inactive-widget): New test
---
 doc/misc/widget.texi        |  3 +++
 lisp/wid-edit.el            | 11 +++++++++--
 test/lisp/wid-edit-tests.el | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/doc/misc/widget.texi b/doc/misc/widget.texi
index 68d53a42025..f84e81bce77 100644
--- a/doc/misc/widget.texi
+++ b/doc/misc/widget.texi
@@ -1333,6 +1333,9 @@ default
 Function that takes a widget and makes it inactive for user
 modifications.
 
+If you modify a widget that is not active, you should make sure the
+:deactivate function gets called again after the modifications.
+
 @vindex action@r{ keyword}
 @item :action
 Function that takes a widget and optionally an event, and handles a
diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index a1d4c4850ae..5301fd95a63 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -549,8 +549,15 @@ widget-inactive
   :group 'widget-faces)
 
 (defun widget-specify-inactive (widget from to)
-  "Make WIDGET inactive for user modifications."
-  (unless (widget-get widget :inactive)
+  "Make WIDGET inactive for user modifications.
+
+If WIDGET is already inactive, moves the :inactive overlay to the positions
+indicated by FROM and TO, either numbers or markers.
+
+If WIDGET is not inactive, creates an overlay that spans from FROM to TO,
+and saves that overlay under the :inactive property for WIDGET."
+  (if (widget-get widget :inactive)
+      (move-overlay (widget-get widget :inactive) from to)
     (let ((overlay (make-overlay from to nil t nil)))
       (overlay-put overlay 'face 'widget-inactive)
       ;; This is disabled, as it makes the mouse cursor change shape.
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index e34aa64f8d1..755bd12201f 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -481,4 +481,18 @@ widget-test-insertion-at-parent-markers-2
       (should (= ofrom2 (widget-get group2 :from)))
       (should (= oto2 (widget-get group2 :to))))))
 
+(ert-deftest widget-test-modification-of-inactive-widget ()
+  "Test that modifications to an inactive widget keep all of it inactive."
+  (with-temp-buffer
+    (let* ((radio (widget-create 'radio-button-choice
+                                 '(item "One") '(item "Two") '(item "Confirm")))
+           (from (widget-get radio :from))
+           (to (widget-get radio :to))
+           (ov (progn (widget-apply radio :deactivate)
+                      (widget-get radio :inactive))))
+      (widget-value-set radio "")
+      (widget-apply radio :deactivate)
+      (should (= (overlay-start ov) from))
+      (should (= (overlay-end ov) to)))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.34.1