Package: emacs;
Reported by: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 27 Feb 2025 23:14:02 UTC
Severity: normal
Found in version 30.1.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Message #47 received at 76620 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Eli Zaretskii <eliz <at> gnu.org>, jonas <at> bernoul.li, 76620 <at> debbugs.gnu.org Subject: Re: bug#76620: 30.1.50; mouse-1 mode-line bindings are unusable when point is on a button Date: Mon, 08 Sep 2025 16:49:59 -0400
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> writes: >>> I'm not sure I understand the logic of this change, so please >>> elaborate, by showing the form(s) of POSITION for which the current >>> code doesn't work, and please explain how this patch fixes that. >> When you click on a part of the mode line which is after the end of >> mode-line-format, (at least on GNU/Linux) you get an event like this: >> >> (down-mouse-1 (#<window 3 on *Bugs*> mode-line (2379 . 2071) 21536370 >> nil nil (157 . 88) nil (8 . 19) (15 . 32))) >> >> posn-string is nil for this but it's still in the mode-line. My change >> checks for whether a position is in the mode-line before checking if >> posn-string is nil, so it catches such events. > > I suspect this answer (in one way or another) should find its way either > into a comment or into the commit message. > [ I tend to have that impression for most questions that occur during > a code review, tho, so maybe it's just me. ] I agree, I added it to the commit message. > I have a more demanding request: any chance we could concoct an ERT test > for that? I realize that it might be difficult, but we keep skipping > such "UI" tests because they're difficult, so I think we should try to > slowly get closer to having an actual test suite of the UI elements. > If you can identify one obstacle that makes it currently impossible, > maybe we can fix that obstacle, and thus make one step in that direction. A very reasonable request, I think. How about the test in the attached patch? It's not really an end-to-end test, but I think it's still useful. In fact, while writing this test, I noticed another bug in this code, which I've now also fixed. Good call :) The test would be improved if we had a nice way to generate click events from Lisp. Then I could write some code which does "click on the mode line" and test that that has the right behavior. But I have no idea how to do that, is there even a way?
[0001-Ignore-keymaps-at-point-for-positions-outside-the-bu.patch (text/x-patch, inline)]
From 40881a88b06f3f6bbda2f48b7be908ef49e02d74 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 28 Aug 2025 14:13:24 -0400
Subject: [PATCH] Ignore keymaps at point for positions outside the buffer
Correct a few edge cases where we used the keymaps at point when
looking up keymaps for an event position which is outside the
current buffer. Namely:
- Clicking on a part of the mode line which is after the end of
mode-line-format produces an event with non-nil posn-area but
nil posn-string.
- Even if posn-string doesn't have a local keymap, we should
still ignore the keymaps at point if posn-string is non-nil.
* src/keymap.c (Fcurrent_active_maps): Ignore keymaps at point
for more positions outside the buffer. (bug#76620)
---
src/keymap.c | 38 ++++++++++++++++----------------------
test/src/keymap-tests.el | 27 +++++++++++++++++++++++++++
2 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/src/keymap.c b/src/keymap.c
index 2c250578b00..295b209f06b 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1735,11 +1735,20 @@ DEFUN ("current-active-maps", Fcurrent_active_maps, Scurrent_active_maps,
}
}
- /* If on a mode line string with a local keymap,
- or for a click on a string, i.e. overlay string or a
- string displayed via the `display' property,
- consider `local-map' and `keymap' properties of
- that string. */
+ Lisp_Object pos_area = POSN_POSN (position);
+ if (EQ (pos_area, Qmode_line) || EQ (pos_area, Qheader_line))
+ {
+ /* For clicks on mode line or header line, ignore the maps
+ we found at POSITION, because properties at point are
+ not relevant in that case. */
+ local_map = Qnil;
+ keymap = Qnil;
+ }
+
+ /* If on a mode line string with a local keymap, or for a
+ click on a string, i.e. overlay string or a string
+ displayed via the `display' property, consider only the
+ `local-map' and `keymap' properties of that string. */
if (CONSP (string) && STRINGP (XCAR (string)))
{
@@ -1749,23 +1758,8 @@ DEFUN ("current-active-maps", Fcurrent_active_maps, Scurrent_active_maps,
&& XFIXNUM (pos) >= 0
&& XFIXNUM (pos) < SCHARS (string))
{
- Lisp_Object map = Fget_text_property (pos, Qlocal_map,
- string);
- Lisp_Object pos_area = POSN_POSN (position);
- /* For clicks on mode line or header line, override
- the maps we found at POSITION unconditionally, even
- if the corresponding properties of the mode- or
- header-line string are nil, because propertries at
- point are not relevant in that case. */
- if (!NILP (map)
- || EQ (pos_area, Qmode_line)
- || EQ (pos_area, Qheader_line))
- local_map = map;
- map = Fget_text_property (pos, Qkeymap, string);
- if (!NILP (map)
- || EQ (pos_area, Qmode_line)
- || EQ (pos_area, Qheader_line))
- keymap = map;
+ local_map = Fget_text_property (pos, Qlocal_map, string);
+ keymap = Fget_text_property (pos, Qkeymap, string);
}
}
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index c605c3eb09d..950c741a6dd 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -509,6 +509,33 @@ keymap-unset-test-remove-and-inheritance
;; From the parent this time/
(should (equal (keymap-lookup map "u") #'undo))))
+(defun keymap-test--maps-for-posn (area string)
+ (current-active-maps
+ nil
+ ;; FIXME: This test would be better if this was a real position
+ ;; created by a real click.
+ `(,(selected-window) ,area (1 . 1) 0 (,string . 0) nil (1 . 1) nil (1 . 1) (1 . 1))))
+
+(ert-deftest keymap-test-keymaps-for-non-buffer-positions ()
+ "`current-active-maps' with non-buffer positions. (bug#76620)"
+ (with-temp-buffer
+ (pop-to-buffer (current-buffer))
+ (let ((keymap (make-sparse-keymap "keymap-at-point")))
+ (insert (propertize "string" 'keymap keymap))
+ (goto-char (point-min))
+ (should (memq keymap (current-active-maps)))
+ (should-not (memq keymap (keymap-test--maps-for-posn 'mode-line nil)))
+ (should-not (memq keymap (keymap-test--maps-for-posn 'mode-line "s")))
+ (should-not (memq keymap (keymap-test--maps-for-posn nil "s")))
+ (should (memq keymap (keymap-test--maps-for-posn nil nil)))
+ (let* ((mode-line-keymap (make-sparse-keymap "keymap-in-mode-line"))
+ (s (propertize "string" 'keymap mode-line-keymap)))
+ ;; Respect `keymap' in the string clicked on.
+ (should-not (memq keymap (keymap-test--maps-for-posn nil s)))
+ (should-not (memq keymap (keymap-test--maps-for-posn 'mode-line s)))
+ (should (memq mode-line-keymap (keymap-test--maps-for-posn nil s)))
+ (should (memq mode-line-keymap (keymap-test--maps-for-posn 'mode-line s)))))))
+
(provide 'keymap-tests)
;;; keymap-tests.el ends here
--
2.43.7
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.