bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

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

View the HTML portion of the attached email in MH-E and Gnus.  Both will
display "https://fsf.org" in red and put a caution emoji after it,
because the link doesn't actually point to https://fsf.org.  Good so
far.

In Gnus, if you mouse over the link, the actual target appears in the
echo area.  And if you mouse over the caution emoji, you'll see text in
the echo area that tells you that the link target doesn't match the link
text.  Also good so far.

In MH-E, if you mouse over the link or emoji, you get text in the echo
area that tells you how to follow the link.  There's no explanation
given for the red text or the caution emoji, which is a bug IMO.

I tracked this down to MH-E's use of goto-address when displaying a
message (mh-display-msg > mh-show-addr > goto-address).

I can think of a couple ways to deal with this, but I'm not sure what
the right way forward is.  The simplest approach would be for
mh-display-msg to stop using goto-address.  I'm not happy with that
approach because it removes functionality that MH-E has had for awhile.  

A slightly more sophisticated approach would be to not use goto-address
when using shr to render the message.  That loses some functionality
(like the auto-linkification of email addresses), and it seems kind of
kludgey, but I suppose it's not too terrible.

Another possibility would be to make goto-address smarter, so that it
doesn't stomp on whatever it was that shr did to get the mouse-over
text.

I'm happy to take a stab at a fix, but I could use some guidance on the
right direction to go in.

thanks,
mike

[-- Attachment #2: test case email --]
[-- Type: text/html, Size: 820 bytes --]

[-- Attachment #3: Type: text/plain, Size: 4425 bytes --]


PS. I've reproduced the problem with commit a14821d615; you can ignore
the "Repository revision" and "Repository branch" fields below.

In GNU Emacs 29.0.60 (build 4, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2022-12-29 built on alto
Repository revision: f26689637923ac9b834a209e1bab09c779be212f
Repository branch: emacs-29-mdk
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --prefix=/usr/new'

Configured features:
CAIRO FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LC_TIME: C
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: MH-Folder

Minor modes in effect:
  hl-line-mode: t
  shell-dirtrack-mode: t
  delete-selection-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  buffer-read-only: t
  column-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow mh-identity mh-letter emacsbug sort gnus-async gnus-bcklg
gnus-kill qp cl-extra help-mode gnus-ml disp-table url-http url-gw
url-cache url-auth url-handlers nnrss mm-url nndoc nndraft
network-stream nsm gnus-agent gnus-srvr nnvirtual nntp gnus-cache
textsec uni-scripts idna-mapping ucs-normalize uni-confusable
textsec-check mm-archive mail-extr mh-mime mule-util mh-thread mh-show
goto-addr gnus-cite mh-inc hl-line mh-tool-bar mh-acros mh-seq mh-xface
mh-utils mh-folder which-func imenu misearch multi-isearch crm thingatpt
cus-edit pp cus-start cus-load mdk-mail gnus-mh gnus-msg mh-comp mh-scan
mh-gnus gnus-dup nnmh gnus-score score-mode gnus-art mm-uu mml2015
mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku
url-file svg dom browse-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie generate-lisp-file url-domsuf
url-util url-parse auth-source cl-seq eieio eieio-core cl-macs json map
gv url-vars gnus-group gnus-undo gnus-start gnus-dbus dbus xml
gnus-cloud nnimap nnmail mail-source utf7 nnoo parse-time iso8601
gnus-spec gnus-int gnus-range gnus-win gnus nnheader range wid-edit mh-e
mh-buffers mh-loaddefs message sendmail mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr mailabbrev mail-utils gmm-utils mailheader warnings server
noutline outline icons cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs byte-opt bytecomp
byte-compile shell subr-x pcomplete comint ansi-osc ansi-color ring
xcscope advice delsel vc vc-dispatcher timeclock cl-loaddefs cl-lib
mdk-hacks rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads inotify dynamic-setting system-font-setting font-render-setting
cairo x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 350237 71147)
 (symbols 48 23107 9)
 (strings 32 88955 27698)
 (string-bytes 1 2387295)
 (vectors 16 66600)
 (vector-slots 8 1547532 240746)
 (floats 8 253 414)
 (intervals 56 767 0)
 (buffers 976 22))

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Eli Zaretskii]

> From: Mike Kupfer <kupfer@rawbw.com>
> Date: Thu, 29 Dec 2022 19:59:05 -0800
> 
> In MH-E, if you mouse over the link or emoji, you get text in the echo
> area that tells you how to follow the link.  There's no explanation
> given for the red text or the caution emoji, which is a bug IMO.
> 
> I tracked this down to MH-E's use of goto-address when displaying a
> message (mh-display-msg > mh-show-addr > goto-address).
> 
> I can think of a couple ways to deal with this, but I'm not sure what
> the right way forward is.  The simplest approach would be for
> mh-display-msg to stop using goto-address.  I'm not happy with that
> approach because it removes functionality that MH-E has had for awhile.  
> 
> A slightly more sophisticated approach would be to not use goto-address
> when using shr to render the message.  That loses some functionality
> (like the auto-linkification of email addresses), and it seems kind of
> kludgey, but I suppose it's not too terrible.
> 
> Another possibility would be to make goto-address smarter, so that it
> doesn't stomp on whatever it was that shr did to get the mouse-over
> text.
> 
> I'm happy to take a stab at a fix, but I could use some guidance on the
> right direction to go in.

I guess the latter, but it will have to be on master, since I'm quite
sure the changes will not be safe enough for the release branch.

Thanks.

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mike Kupfer <kupfer@rawbw.com>
>> Date: Thu, 29 Dec 2022 19:59:05 -0800
>>
>> In MH-E, if you mouse over the link or emoji, you get text in the echo
>> area that tells you how to follow the link.  There's no explanation
>> given for the red text or the caution emoji, which is a bug IMO.
>>
>> I tracked this down to MH-E's use of goto-address when displaying a
>> message (mh-display-msg > mh-show-addr > goto-address).
>>
>> I can think of a couple ways to deal with this, but I'm not sure what
>> the right way forward is.  The simplest approach would be for
>> mh-display-msg to stop using goto-address.  I'm not happy with that
>> approach because it removes functionality that MH-E has had for awhile.
>>
>> A slightly more sophisticated approach would be to not use goto-address
>> when using shr to render the message.  That loses some functionality
>> (like the auto-linkification of email addresses), and it seems kind of
>> kludgey, but I suppose it's not too terrible.
>>
>> Another possibility would be to make goto-address smarter, so that it
>> doesn't stomp on whatever it was that shr did to get the mouse-over
>> text.
>>
>> I'm happy to take a stab at a fix, but I could use some guidance on the
>> right direction to go in.
>
> I guess the latter, but it will have to be on master, since I'm quite
> sure the changes will not be safe enough for the release branch.
>
> Thanks.

Mike, did you make any progress here?  Thanks in advance.

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Stefan Kangas wrote:

> Mike, did you make any progress here?  Thanks in advance.

No, I'm afraid I haven't had time to look into this.

mike

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Stefan Kangas wrote:

> Mike, did you make any progress here?  Thanks in advance.

I finally have some progress to report.

The root cause of the fontification conflict between goto-address and
shr/textsec is that the textsec code uses text properties,
goto-address uses overlays, and overlays override text properties.

So one possible approach to fix this would be for the textsec code to
use overlays, and give the textsec overlay a higher priority than the
goto-address overlay.

I see a couple potential drawbacks with that approach:

1. shr originally used overlays; it was changed to use text properties
because of performance concerns (see
https://lists.gnu.org/r/emacs-diffs/2013-06/msg00215.html).  Of course,
that was over 10 years ago, and processors are faster now.  And maybe
shr could use overlays for the textsec stuff and text properties
everywhere else?

2. AFAICT, there's no system for coordinating overlay priorities across
different packages or subsystems.  I think that makes the priority
mechanism brittle, and for that reason I'm reluctant to use it.

So I lean towards having goto-address leave text alone (don't set an
overlay) if it finds text properties set for the text.

If you have any recommendations or other comments, please let me know.

thanks,
mike

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Eli Zaretskii]

> From: Mike Kupfer <kupfer@rawbw.com>
> cc: Eli Zaretskii <eliz@gnu.org>, 60423@debbugs.gnu.org
> Comments: In-reply-to Stefan Kangas <stefankangas@gmail.com>
>    message dated "Tue, 05 Sep 2023 16:31:18 -0700."
> Date: Tue, 08 Oct 2024 17:09:06 -0700
> 
> So I lean towards having goto-address leave text alone (don't set an
> overlay) if it finds text properties set for the text.

Not just any properties: only 'face' properties, right?

I think this is okay, if we don't have better tricks up our sleeves.
I added Stefan Monnier in the hope that he could have some
suggestions.

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Eli Zaretskii wrote:

> > From: Mike Kupfer <kupfer@rawbw.com>
[...]
> > So I lean towards having goto-address leave text alone (don't set an
> > overlay) if it finds text properties set for the text.
> 
> Not just any properties: only 'face' properties, right?

Hmm.  shr-tag-a sets these properties, with shr-urlify doing most of the
work:

- face ('shr-link')
- shr-url
- button
- category
- help-echo
- follow-link
- mouse-face

If the URL is suspicious, shr-tag-a also inserts a triangular warning
symbol with a 'help-echo' property.

So if I just care about conflicts between goto-address and shr, I guess
I could just check for the 'shr-link' face.  I'd prefer to have a more
general test, so I think I want to check for any of

- face
- help-echo
- mouse-face

and maybe

- button
- follow-link

as well.  What do you think?

thanks,
mike

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Eli Zaretskii]

> From: Mike Kupfer <kupfer@rawbw.com>
> cc: Stefan Monnier <monnier@iro.umontreal.ca>, stefankangas@gmail.com,
>         60423@debbugs.gnu.org
> Comments: In-reply-to Eli Zaretskii <eliz@gnu.org>
>    message dated "Wed, 09 Oct 2024 15:24:02 +0300."
> Date: Thu, 10 Oct 2024 16:46:16 -0700
> 
> Eli Zaretskii wrote:
> 
> > > From: Mike Kupfer <kupfer@rawbw.com>
> [...]
> > > So I lean towards having goto-address leave text alone (don't set an
> > > overlay) if it finds text properties set for the text.
> > 
> > Not just any properties: only 'face' properties, right?
> 
> Hmm.  shr-tag-a sets these properties, with shr-urlify doing most of the
> work:
> 
> - face ('shr-link')
> - shr-url
> - button
> - category
> - help-echo
> - follow-link
> - mouse-face
> 
> If the URL is suspicious, shr-tag-a also inserts a triangular warning
> symbol with a 'help-echo' property.
> 
> So if I just care about conflicts between goto-address and shr, I guess
> I could just check for the 'shr-link' face.  I'd prefer to have a more
> general test, so I think I want to check for any of
> 
> - face
> - help-echo
> - mouse-face
> 
> and maybe
> 
> - button
> - follow-link
> 
> as well.  What do you think?

AFAIU from your analysis of the problem, it happens because the same
text is covered by both a text property and an overlay with the same
property.  If that is indeed the reason, then the only conflict that I
could see in this situation is for the 'face' property: both shr-tag-a
and goto-address use it, one as a text property and the other as an
overlay property.  The other properties you mention aren't used by
goto-address, so they cannot conflict with what shr-tag-a.  Am I
missing something?

Testing unrelated properties might give us false positives, so I think
we should avoid that.

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

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

>> So I lean towards having goto-address leave text alone (don't set an
>> overlay) if it finds text properties set for the text.
>
> Not just any properties: only 'face' properties, right?
>
> I think this is okay, if we don't have better tricks up our sleeves.
> I added Stefan Monnier in the hope that he could have some
> suggestions.

In this case the problem is that two packages compete for the same URL.
I think it makes sense for goto-address to "leave text alone" in this
case, but the question remains of how to detect *this* situation.

The underlying text having a `face` property doesn't seem sufficient
(especially since multiple `face` properties get merged, so the
conflict is less severe).
Maybe it should check for the presence of `help-echo and (follow-link
or keymap)`?  And make sure the those properties cover exactly the same
chunk of text?


        Stefan

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Eli Zaretskii wrote:

> AFAIU from your analysis of the problem, it happens because the same
> text is covered by both a text property and an overlay with the same
> property.  If that is indeed the reason, then the only conflict that I
> could see in this situation is for the 'face' property: both shr-tag-a
> and goto-address use it, one as a text property and the other as an
> overlay property.  The other properties you mention aren't used by
> goto-address, so they cannot conflict with what shr-tag-a.  Am I
> missing something?

Yes, goto-address also sets the 'help-echo' and 'mouse-face' properties,
though the latter is only set for email addresses.  (Sorry, I didn't
call this out in my earlier email.)

> Testing unrelated properties might give us false positives, so I think
> we should avoid that.

Agreed about trying to avoid false positives.

The complete list of overlay properties that goto-address-fontify sets
is

- evaporate
- face
- follow-link
- goto-address
- help-echo
- keymap
- mouse-face

We agree about checking for 'face'.  I'm still holding out for checking
for 'help-echo' and 'mouse-face'.  After reviewing the documentation for
the other properties, I'd also like to check for 'keymap' and
'follow-link'.  I don't see a reason to check for 'evaporate'.
'goto-address' is not documented and appears to be used to mark the
overlay as belonging to goto-address, so no need to check for it.

mike

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Stefan Monnier wrote:

> In this case the problem is that two packages compete for the same URL.
> I think it makes sense for goto-address to "leave text alone" in this
> case, but the question remains of how to detect *this* situation.
> 
> The underlying text having a `face` property doesn't seem sufficient
> (especially since multiple `face` properties get merged, so the
> conflict is less severe).
> Maybe it should check for the presence of `help-echo and (follow-link
> or keymap)`?  And make sure the those properties cover exactly the same
> chunk of text?

As far as covering the same chunk of text, I'll need to play with this
some more to see what works.  shr-tag-a inserts a warning emoji with a
help-echo property, which goto-address somehow manages to clobber.  That
warning emoji is not something goto-address would normally be looking
for.

mike

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Eli Zaretskii]

> From: Mike Kupfer <kupfer@rawbw.com>
> cc: Eli Zaretskii <eliz@gnu.org>, 60423@debbugs.gnu.org,
>         stefankangas@gmail.com
> Comments: In-reply-to Stefan Monnier <monnier@iro.umontreal.ca>
>    message dated "Sat, 12 Oct 2024 19:13:49 -0400."
> Date: Wed, 16 Oct 2024 14:52:49 -0700
> 
> Stefan Monnier wrote:
> 
> > In this case the problem is that two packages compete for the same URL.
> > I think it makes sense for goto-address to "leave text alone" in this
> > case, but the question remains of how to detect *this* situation.
> > 
> > The underlying text having a `face` property doesn't seem sufficient
> > (especially since multiple `face` properties get merged, so the
> > conflict is less severe).
> > Maybe it should check for the presence of `help-echo and (follow-link
> > or keymap)`?  And make sure the those properties cover exactly the same
> > chunk of text?
> 
> As far as covering the same chunk of text, I'll need to play with this
> some more to see what works.  shr-tag-a inserts a warning emoji with a
> help-echo property, which goto-address somehow manages to clobber.  That
> warning emoji is not something goto-address would normally be looking
> for.

Ping!  How can we make some progress with this bug report?

bug#60423: 29.0.60; goto-address and shr/textsec don't play nicely together

[Mike Kupfer]

Eli Zaretskii wrote:

> > From: Mike Kupfer <kupfer@rawbw.com>
[...]
> > Stefan Monnier wrote:
[...]
> > > Maybe it should check for the presence of `help-echo and (follow-link
> > > or keymap)`?  And make sure the those properties cover exactly the same
> > > chunk of text?
> > 
> > As far as covering the same chunk of text, I'll need to play with this
> > some more to see what works.  shr-tag-a inserts a warning emoji with a
> > help-echo property, which goto-address somehow manages to clobber.  That
> > warning emoji is not something goto-address would normally be looking
> > for.
> 
> Ping!  How can we make some progress with this bug report?

I did figure out why goto-address is clobbering the help-echo property
on the warning emoji.  goto-address uses goto-address-url-regexp to
identify URLs.  shr puts the emoji immediately after the suspicious URL,
and apparently the regexp includes the emoji as part of the URL.

  https://badurl.com⚠️

I haven't completely reverse-engineered the regexp.  I see that it's
built from a list of URI schemes and thing-at-point-url-path-regexp.
Maybe thing-at-point-url-path-regexp needs to be pickier?  But I don't
understand how things should work in light of internationalized URLs.

I've thought about having goto-address bail out if there are any
conflicting properties in the range that it wants to overlay, but I
haven't had time to prototype it to see how well that works.

I suppose another possibility would be to move the warning emoji: put it
in front of the suspicious URL, rather than after the URL.

WDYT?

mike