bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

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

I have tried to find a workaround around bug#42521 to render a 
completion popup with images.

The attached patch almost works, except in Emacs 27+ it leads to a
repeat of bug#38563. Meaning, the rendered popup inherits certain
properties (most importantly, :extend) from the character under the end
of the overlay. Leading to similar visual effects, screenshots attached.

I would hope to solve it like we solved it in bug#38563, but the 'face'
overlay property doesn't seem to have any effect here.

The attached patch for company.el should apply cleanly on top of its
current code in ELPA.

The reproduction scenario is almost the same, only in this case the 
problem occurs when the *end* of the overlay falls on a position with
the non-nil 'extend' property (and the overlay is 10 lines long).
See the screenshots.

Please let me know if you need any further clarifications.

I do believe it's a regression, considering the same code works okay in
Emacs 26.3, in exact same situations (whitespace-mode 'tail' face or the
log-edit line under the end of the popup overlay).

In GNU Emacs 28.0.50 (build 25, x86_64-pc-linux-gnu, GTK+ Version 
3.24.20, cairo version 1.16.0)
  of 2020-07-20 built on potemkin
Repository revision: 4c08c2f45b9bb0265f6d7c3529011dee1b18e843
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Ubuntu 20.04.1 LTS

[-- Attachment #2: company.el.diff --]
[-- Type: text/x-patch, Size: 1555 bytes --]

diff --git a/company.el b/company.el
index 592c6d1..d47e79f 100644
--- a/company.el
+++ b/company.el
@@ -2742,6 +2742,9 @@ If SHOW-VERSION is non-nil, show the version in the echo area."
     (let ((str (concat (when nl " \n")
                        (mapconcat 'identity (nreverse new) "\n")
                        "\n")))
+      ;; Use add-face-text-property in Emacs 24.4
+      ;; https://debbugs.gnu.org/38563
+      (font-lock-append-text-property 0 (length str) 'face 'default str)
       (when nl (put-text-property 0 1 'cursor t str))
       str)))
 
@@ -2950,14 +2953,13 @@ Returns a negative number if the tooltip should be displayed above point."
       (overlay-put ov 'priority 111)
       ;; No (extra) prefix for the first line.
       (overlay-put ov 'line-prefix "")
-      ;; `display' is better
-      ;; (http://debbugs.gnu.org/18285, http://debbugs.gnu.org/20847),
-      ;; but it doesn't work on 0-length overlays.
-      (if (< (overlay-start ov) (overlay-end ov))
-          (overlay-put ov 'display disp)
-        (overlay-put ov 'after-string disp)
-        (overlay-put ov 'invisible t))
-      (overlay-put ov 'face 'default)
+      (overlay-put ov 'after-string disp)
+      ;; `display' is better than `invisible':
+      ;; https://debbugs.gnu.org/18285
+      ;; https://debbugs.gnu.org/20847
+      ;; https://debbugs.gnu.org/42521
+      (overlay-put ov 'display "")
+      (overlay-put ov 'face 'default) ; Has no effect!
       (overlay-put ov 'window (selected-window)))))
 
 (defun company-pseudo-tooltip-guard ()

[-- Attachment #3: Screenshot from 2020-07-26 20-58-50.png --]
[-- Type: image/png, Size: 100959 bytes --]

[-- Attachment #4: Screenshot from 2020-07-26 20-59-25.png --]
[-- Type: image/png, Size: 78144 bytes --]

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 26 Jul 2020 21:23:32 +0300
> 
> The reproduction scenario is almost the same, only in this case the 
> problem occurs when the *end* of the overlay falls on a position with
> the non-nil 'extend' property (and the overlay is 10 lines long).
> See the screenshots.
> 
> Please let me know if you need any further clarifications.

I'd like a recipe for reproducing the problem and the description of
what exactly constitutes the problem, if possible.

Thanks.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

On 26.07.2020 22:02, Eli Zaretskii wrote:
> I'd like a recipe for reproducing the problem and the description of
> what exactly constitutes the problem, if possible.

The recipe is similar to 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#11.

The only difference is one needs to initiate completion 10 lines above 
the lines which `whitespace-mode` highlights with red.

So the scenario becomes:

1. Apply the patch to company.el.
2. Launch 'emacs -Q -L path/to/company -l company'.
3. Turn on company-mode and whitespace-mode.
4. In the scratch:
C-u 11 M-x newline
space space space
C-u 11 M-x previous-line
Type 'c', then M-x company-complete-common

The problem is twofold:

company-mode with the patch I attached to this report behaves worse in 
Emacs 27 than in Emacs 26.3: it renders a field of solid color to the 
right of the popup. The color is red in my config, but apparently yellow 
in the default configuration.

The solution I hoped would fix this, which we discussed in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#60, does not work 
when the popup is rendered with 'after-string' instead of 'display' 
overlay property (and a similar problem exists for 'before-string' as 
well). Hence the title of this bug report.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> Cc: 42552@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 26 Jul 2020 23:00:57 +0300
> 
> On 26.07.2020 22:02, Eli Zaretskii wrote:
> > I'd like a recipe for reproducing the problem and the description of
> > what exactly constitutes the problem, if possible.
> 
> The recipe is similar to 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#11.
> 
> The only difference is one needs to initiate completion 10 lines above 
> the lines which `whitespace-mode` highlights with red.

Thanks.

> 1. Apply the patch to company.el.
> 2. Launch 'emacs -Q -L path/to/company -l company'.
> 3. Turn on company-mode and whitespace-mode.
> 4. In the scratch:
> C-u 11 M-x newline
> space space space
> C-u 11 M-x previous-line
> Type 'c', then M-x company-complete-common

I installed a fix on master.  The fix is simple enough, but it is in
code that is used in all cases where faces are used in overlay
strings, and so I don't want to install this on emacs-27, since the
Emacs 27.1 release is imminent, and I don't want to delay it any
longer.  We could discuss later whether to backport to Emacs 27.2.

> The solution I hoped would fix this, which we discussed in 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#60, does not work 
> when the popup is rendered with 'after-string' instead of 'display' 
> overlay property (and a similar problem exists for 'before-string' as 
> well). Hence the title of this bug report.

Setting the 'face' property of an overlay with the intent of affecting
the display of an overlay string never worked in Emacs, and the
comments to the code even mention it (note the last sentence):

  /* Return the face ID at buffer position POS for displaying ASCII
     characters associated with overlay strings for overlay OVERLAY.

     Like face_at_buffer_position except for OVERLAY.  Currently it
     simply disregards the `face' properties of all overlays.  */

  int
  face_for_overlay_string (struct window *w, ptrdiff_t pos,

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

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

On 30.07.2020 17:04, Eli Zaretskii wrote:

>> 1. Apply the patch to company.el.
>> 2. Launch 'emacs -Q -L path/to/company -l company'.
>> 3. Turn on company-mode and whitespace-mode.
>> 4. In the scratch:
>> C-u 11 M-x newline
>> space space space
>> C-u 11 M-x previous-line
>> Type 'c', then M-x company-complete-common
> 
> I installed a fix on master.  The fix is simple enough, but it is in
> code that is used in all cases where faces are used in overlay
> strings, and so I don't want to install this on emacs-27, since the
> Emacs 27.1 release is imminent, and I don't want to delay it any
> longer.  We could discuss later whether to backport to Emacs 27.2.

Thank you. This sounds like a decent compromise: after we agree on a 
fix, it will have some time to mature on the master branch.

But the installed fix doesn't solve the other scenario, depicted on the 
second screenshot attached to this report: 
https://debbugs.gnu.org/cgi/bugreport.cgi?msg=5;att=3;filename=Screenshot+from+2020-07-26+20-59-25.png;bug=42552

Do you need a step-by-step repro for that?

The reason for that difference seems to be that it's a log-edit buffer, 
and the delimiter line is fontified using an anonymous face '(:height 
0.1 :inverse-video t :extend t), see the end of log-edit-font-lock-keywords.

Still, Emacs 26.3 doesn't exhibit this problem, and in that version the 
contents of that anonymous face was the same (except without :extend t, 
but back then, all faces did "extend"). (*)

Would it be too hard to have the same behavior in 28+?

The log-edit case isn't too important by itself, but the same thing can 
happen if, for example, the end of the completion overlay falls on a 
smerge region, for example. I'll attach a screenshot with this, too.

Furthermore, in Emacs 26.3 I can propertize the newlines in the overlay 
string with '(face region) and see the "extend" effect. Or keep them 
with 'default' face and see no "extend" effect on those lines.

So with some more work, if the behavior becomes more 26.3-compatible, I 
think I can implement even more accurate rendering, where only the lines 
which need "extending" are extended (in the popup's background).

>> The solution I hoped would fix this, which we discussed in
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#60, does not work
>> when the popup is rendered with 'after-string' instead of 'display'
>> overlay property (and a similar problem exists for 'before-string' as
>> well). Hence the title of this bug report.
> 
> Setting the 'face' property of an overlay with the intent of affecting
> the display of an overlay string never worked in Emacs, and the
> comments to the code even mention it (note the last sentence):
> 
>    /* Return the face ID at buffer position POS for displaying ASCII
>       characters associated with overlay strings for overlay OVERLAY.
> 
>       Like face_at_buffer_position except for OVERLAY.  Currently it
>       simply disregards the `face' properties of all overlays.  */
> 
>    int
>    face_for_overlay_string (struct window *w, ptrdiff_t pos,

But it works! That's how we closed bug#38563, didn't we?

To see it have an effect, launch Emacs built from the emacs-27 branch 
(to be 100% sure, but it seems this works with the current master too), 
then go through the scenario outlined in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38563#11.

Use the version of company currently in ELPA, with no extra patches.

You will see the bug fail to reproduce.

Now, delete (or comment out) the line

    (overlay-put ov 'face 'default)

from the function company-pseudo-tooltip-unhide, then re-evaluate it. 
Try the scenario again: the bug is back.

Not only does it help with the :extend property, it also helps to avoid 
inheriting other properties, such as :bold. But only if the overlay 
string is applied via 'display', not 'after-string'.

To be clear, I would prefer the fix of the first variety (making 
behavior more compatible with 26.3), rather than just being able to 
override the "underlying face" using the 'face' property. But either is 
better than nothing, and having both would be ideal.

[-- Attachment #2: Screenshot from 2020-08-03 01-31-51.png --]
[-- Type: image/png, Size: 73298 bytes --]

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> Cc: 42552@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 3 Aug 2020 02:37:37 +0300
> 
> But the installed fix doesn't solve the other scenario, depicted on the 
> second screenshot attached to this report: 
> https://debbugs.gnu.org/cgi/bugreport.cgi?msg=5;att=3;filename=Screenshot+from+2020-07-26+20-59-25.png;bug=42552
> 
> Do you need a step-by-step repro for that?
> 
> The reason for that difference seems to be that it's a log-edit buffer, 
> and the delimiter line is fontified using an anonymous face '(:height 
> 0.1 :inverse-video t :extend t), see the end of log-edit-font-lock-keywords.

When the underlying face has the :extend attribute set, we must obey
it.  So what you see in that case is the expected behavior.

> Still, Emacs 26.3 doesn't exhibit this problem, and in that version the 
> contents of that anonymous face was the same (except without :extend t, 
> but back then, all faces did "extend"). (*)

Emacs 26 and before simply extended the face of the last character of
the line.  Emacs 27 doesn't do that, it examines the faces in effect
anew, filtering out those which don't have the :extend attribute set,
so the result is different.  This is exactly the change in behavior
that was intended, not a bug.

> Would it be too hard to have the same behavior in 28+?

You can have this in Emacs > 26 if you make the face of the last
character have the :extend attribute set, so that its background color
overrides that of the one of the underlying buffer text.  E.g., you
could define a face that inherits from 'default', and if that doesn't
specify the background color, use the frame's background as fallback
to set that face's background.

I don't see how else to get the behavior you want when using overlay
strings.  The only alternative is to move the position where you place
the overlay outside the problematic face, but that is probably
undesirable for other reasons.

> Furthermore, in Emacs 26.3 I can propertize the newlines in the overlay 
> string with '(face region) and see the "extend" effect. Or keep them 
> with 'default' face and see no "extend" effect on those lines.

You are saying that this doesn't work in Emacs >= 27?

> >       Like face_at_buffer_position except for OVERLAY.  Currently it
> >       simply disregards the `face' properties of all overlays.  */
> > 
> >    int
> >    face_for_overlay_string (struct window *w, ptrdiff_t pos,
> 
> But it works! That's how we closed bug#38563, didn't we?

It works with display strings, yes.  You now want to switch to overlay
strings, and the rules of collecting faces are subtly different there.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

On 03.08.2020 18:09, Eli Zaretskii wrote:
>> Cc: 42552@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Mon, 3 Aug 2020 02:37:37 +0300
>>
>> But the installed fix doesn't solve the other scenario, depicted on the
>> second screenshot attached to this report:
>> https://debbugs.gnu.org/cgi/bugreport.cgi?msg=5;att=3;filename=Screenshot+from+2020-07-26+20-59-25.png;bug=42552
>>
>> Do you need a step-by-step repro for that?
>>
>> The reason for that difference seems to be that it's a log-edit buffer,
>> and the delimiter line is fontified using an anonymous face '(:height
>> 0.1 :inverse-video t :extend t), see the end of log-edit-font-lock-keywords.
> 
> When the underlying face has the :extend attribute set, we must obey
> it.  So what you see in that case is the expected behavior.

Should the "overlay string" obey the underlying face, though? It doesn't 
obey the 'face' property, like you explained. Seems inconsistent.

>> Still, Emacs 26.3 doesn't exhibit this problem, and in that version the
>> contents of that anonymous face was the same (except without :extend t,
>> but back then, all faces did "extend"). (*)
> 
> Emacs 26 and before simply extended the face of the last character of
> the line.  Emacs 27 doesn't do that, it examines the faces in effect
> anew, filtering out those which don't have the :extend attribute set,
> so the result is different.  This is exactly the change in behavior
> that was intended, not a bug.

But it should obey :extend set to nil, shouldn't it?

>> Would it be too hard to have the same behavior in 28+?
> 
> You can have this in Emacs > 26 if you make the face of the last
> character have the :extend attribute set, so that its background color
> overrides that of the one of the underlying buffer text.  E.g., you
> could define a face that inherits from 'default', and if that doesn't
> specify the background color,

 From what I can tell, 'default' specifies '#ffffff' background on my 
machine. It also sets :extend to nil.

In the diff attached to the report (which is part of the reproduction 
scenario), you can see the whole overlay string's 'face' property being 
appended with the value 'default'. It doesn't seem to help.

Even if I replace 'default' in there with a custom face that sets 
':extend' to nil, the result is the same.

> use the frame's background as fallback
> to set that face's background.

I suppose you mean the custom face must have :extend set to t, and have 
a :background value computed right before it is used (otherwise the user 
might customize the faces mid-session, leading to bad visuals).

That seems to work. It also has the significant benefit on working in 
Emacs 27, so if that's the approach you recommend in the end, the fix 
you already pushed to master might be unnecessary.

> I don't see how else to get the behavior you want when using overlay
> strings.  The only alternative is to move the position where you place
> the overlay outside the problematic face, but that is probably
> undesirable for other reasons.

It's just hard to do. And there might be no such position anywhere nearby.

>> Furthermore, in Emacs 26.3 I can propertize the newlines in the overlay
>> string with '(face region) and see the "extend" effect. Or keep them
>> with 'default' face and see no "extend" effect on those lines.
> 
> You are saying that this doesn't work in Emacs >= 27?

One works, the other doesn't.

>>>        Like face_at_buffer_position except for OVERLAY.  Currently it
>>>        simply disregards the `face' properties of all overlays.  */
>>>
>>>     int
>>>     face_for_overlay_string (struct window *w, ptrdiff_t pos,
>>
>> But it works! That's how we closed bug#38563, didn't we?
> 
> It works with display strings, yes.  You now want to switch to overlay
> strings, and the rules of collecting faces are subtly different there.

Hmm, so 'after-string' and 'before-string' are overlay strings, but 
'display' set on an overlay is not an overlay string. Okay then.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> Cc: 42552@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 5 Aug 2020 02:55:59 +0300
> 
> > When the underlying face has the :extend attribute set, we must obey
> > it.  So what you see in that case is the expected behavior.
> 
> Should the "overlay string" obey the underlying face, though? It doesn't 
> obey the 'face' property, like you explained. Seems inconsistent.

Emacs always worked this way, so changing it now is probably a big
deal.  AFAIU, the reason for this behavior is so that overlay strings
which specify no faces use the same face as the surrounding text.
Which sounds reasonable.

> > Emacs 26 and before simply extended the face of the last character of
> > the line.  Emacs 27 doesn't do that, it examines the faces in effect
> > anew, filtering out those which don't have the :extend attribute set,
> > so the result is different.  This is exactly the change in behavior
> > that was intended, not a bug.
> 
> But it should obey :extend set to nil, shouldn't it?

It does, but :extend nil doesn't override :extend t, it just says that
the face with a nil :extend attribute should not be considered when
computing the face for the empty space past EOL.

> Even if I replace 'default' in there with a custom face that sets 
> ':extend' to nil, the result is the same.

Right, as expected.  The nil value of :extend doesn't override the
non-nil value.

> I suppose you mean the custom face must have :extend set to t, and have 
> a :background value computed right before it is used (otherwise the user 
> might customize the faces mid-session, leading to bad visuals).

Yes, that's what I meant.

> That seems to work. It also has the significant benefit on working in 
> Emacs 27, so if that's the approach you recommend in the end, the fix 
> you already pushed to master might be unnecessary.

The fix I pushed might not be necessary for company's sake, but it is
necessary for other use cases, because without it we don't behave as
documented when :extend attribute is not present: we extend using the
wrong background color.

> >> Furthermore, in Emacs 26.3 I can propertize the newlines in the overlay
> >> string with '(face region) and see the "extend" effect. Or keep them
> >> with 'default' face and see no "extend" effect on those lines.
> > 
> > You are saying that this doesn't work in Emacs >= 27?
> 
> One works, the other doesn't.

Well, that's because you expected :extend nil to do something that it
isn't supposed to do.

> > It works with display strings, yes.  You now want to switch to overlay
> > strings, and the rules of collecting faces are subtly different there.
> 
> Hmm, so 'after-string' and 'before-string' are overlay strings, but 
> 'display' set on an overlay is not an overlay string. Okay then.

Yes, the latter is alluded to as "display string", since the string
comes from the 'display' property.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

On 05.08.2020 17:58, Eli Zaretskii wrote:

>> Should the "overlay string" obey the underlying face, though? It doesn't
>> obey the 'face' property, like you explained. Seems inconsistent.
> 
> Emacs always worked this way, so changing it now is probably a big
> deal.  AFAIU, the reason for this behavior is so that overlay strings
> which specify no faces use the same face as the surrounding text.
> Which sounds reasonable.

Do you imagine creating a better consistency the other way (by having 
the 'face' property affect overlay strings) would be as likely to create 
problems?

>>> Emacs 26 and before simply extended the face of the last character of
>>> the line.  Emacs 27 doesn't do that, it examines the faces in effect
>>> anew, filtering out those which don't have the :extend attribute set,
>>> so the result is different.  This is exactly the change in behavior
>>> that was intended, not a bug.
>>
>> But it should obey :extend set to nil, shouldn't it?
> 
> It does, but :extend nil doesn't override :extend t, it just says that
> the face with a nil :extend attribute should not be considered when
> computing the face for the empty space past EOL.

BTW, are there other attributes with a similar property? For example, I 
find that I have to add (:inverse-video nil) explicitly to the computed 
face which would be appended to the overlay string's 'face' properties.

Otherwise, the newlines are still "extended" with the "inverse video" 
effect. Try this for an example:

1. Eval:

(with-silent-modifications
   (insert (propertize "abc"
                       'font-lock-face
                       '((:background "green" :extend t)
                         default
                         ( :inverse-video t
                           :foreground "yellow"
                           :extend t)))))

2. C-j

The "extended" newline is yellow.

>> That seems to work. It also has the significant benefit on working in
>> Emacs 27, so if that's the approach you recommend in the end, the fix
>> you already pushed to master might be unnecessary.
> 
> The fix I pushed might not be necessary for company's sake, but it is
> necessary for other use cases, because without it we don't behave as
> documented when :extend attribute is not present: we extend using the
> wrong background color.

All right, thank you.

I'll try to implement this all as suggested, and will come back after.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> Cc: 42552@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 7 Aug 2020 02:16:12 +0300
> 
> On 05.08.2020 17:58, Eli Zaretskii wrote:
> 
> >> Should the "overlay string" obey the underlying face, though? It doesn't
> >> obey the 'face' property, like you explained. Seems inconsistent.
> > 
> > Emacs always worked this way, so changing it now is probably a big
> > deal.  AFAIU, the reason for this behavior is so that overlay strings
> > which specify no faces use the same face as the surrounding text.
> > Which sounds reasonable.
> 
> Do you imagine creating a better consistency the other way (by having 
> the 'face' property affect overlay strings) would be as likely to create 
> problems?

That's largely orthogonal, as most overlays don't specify 'face'
AFAIK.  But yes, this could also be a problem after so many years of
disregarding it.  If we really want something like that, perhaps a
separate new property (like 'overriding-face') would be a better way.

> >> But it should obey :extend set to nil, shouldn't it?
> > 
> > It does, but :extend nil doesn't override :extend t, it just says that
> > the face with a nil :extend attribute should not be considered when
> > computing the face for the empty space past EOL.
> 
> BTW, are there other attributes with a similar property?

Perhaps not, I haven't checked.

> For example, I find that I have to add (:inverse-video nil)
> explicitly to the computed face which would be appended to the
> overlay string's 'face' properties.
> 
> Otherwise, the newlines are still "extended" with the "inverse video" 
> effect. Try this for an example:
> 
> 1. Eval:
> 
> (with-silent-modifications
>    (insert (propertize "abc"
>                        'font-lock-face
>                        '((:background "green" :extend t)
>                          default
>                          ( :inverse-video t
>                            :foreground "yellow"
>                            :extend t)))))
> 
> 2. C-j
> 
> The "extended" newline is yellow.

That's expected due to face-merging, no?

> I'll try to implement this all as suggested, and will come back after.

Should we close this bug or keep it open?

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Dmitry Gutov]

On 07.08.2020 08:42, Eli Zaretskii wrote:

>> 1. Eval:
>>
>> (with-silent-modifications
>>     (insert (propertize "abc"
>>                         'font-lock-face
>>                         '((:background "green" :extend t)
>>                           default
>>                           ( :inverse-video t
>>                             :foreground "yellow"
>>                             :extend t)))))
>>
>> 2. C-j
>>
>> The "extended" newline is yellow.
> 
> That's expected due to face-merging, no?

I would have expected it to be green, at least. But if you say it's 
working correctly, it probably is.

>> I'll try to implement this all as suggested, and will come back after.
> 
> Should we close this bug or keep it open?

I've pushed the changes based on this discussion, so the matter looks 
closed.

Thanks.

bug#42552: 28.0.50; Overlay 'face' property doesn't set the "underlying face" for 'after-string'

[Eli Zaretskii]

> Cc: 42552-done@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 11 Aug 2020 01:27:59 +0300
> 
> On 07.08.2020 08:42, Eli Zaretskii wrote:
> 
> >> 1. Eval:
> >>
> >> (with-silent-modifications
> >>     (insert (propertize "abc"
> >>                         'font-lock-face
> >>                         '((:background "green" :extend t)
> >>                           default
> >>                           ( :inverse-video t
> >>                             :foreground "yellow"
> >>                             :extend t)))))
> >>
> >> 2. C-j
> >>
> >> The "extended" newline is yellow.
> > 
> > That's expected due to face-merging, no?
> 
> I would have expected it to be green, at least. But if you say it's 
> working correctly, it probably is.

No, you are right: there's a subtlety here and another bug.

The subtlety is that having 'default' there is not a no-op: it resets
the :inverse-video attribute, and then "yellow" no longer affects the
background of the merged face.  So to have the same happen during face
extension past EOL, you need to have all the components say ":extend t",
like this:

 (with-silent-modifications
     (insert (propertize "abc"
                         'font-lock-face
                         '((:background "green" :extend t)
                           ( :inherit default :extend t)
                           ( :inverse-video t
                             :foreground "yellow"
                             :extend t)))))

And the bug is that the above doesn't work in Emacs 27; I've just
installed a fix on master.

Thanks.