bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Stephen Berman]

0. configure --without-toolkit-scroll-bars && make
1. emacs -Q
2. M-x customize-face RET scroll-bar RET, show all attributes, check the
   Foreground box , change its value to "green", and set for current
   session.
   Now the scroll bar is green.
3. Click the State button and select "Revert This Session's
   Customization".
=> The scroll bar is still green.
   Note that the Foreground box is unchecked.
4. Check the Foreground box.
   Note that its value is "black", though the scroll bar is green.

This pattern also obtains when starting Emacs without -Q, customizating
scroll-bar face, saving the change for future sessions, and then
reverting the customization.


In GNU Emacs 24.3.50.4 (x86_64-suse-linux-gnu, GTK+ Version 3.4.4)
 of 2013-01-17 on rosalinde
Bzr revision: 111542 michael.albinus@gmx.de-20130117090647-lb9mkbk6n8q142w5
Windowing system distributor `The X.Org Foundation', version 11.0.11203000
System Description:	openSUSE 12.2 (x86_64)

Configured using:
 `configure --without-toolkit-scroll-bars CFLAGS=-g3 -O0 --no-create
 --no-recursion'

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

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

Stephen Berman <stephen.berman@gmx.net> writes:

> 0. configure --without-toolkit-scroll-bars && make
> 1. emacs -Q
> 2. M-x customize-face RET scroll-bar RET, show all attributes, check the
>    Foreground box , change its value to "green", and set for current
>    session.
>    Now the scroll bar is green.
> 3. Click the State button and select "Revert This Session's
>    Customization".
> => The scroll bar is still green.
>    Note that the Foreground box is unchecked.
> 4. Check the Foreground box.
>    Note that its value is "black", though the scroll bar is green.

I can reproduce this in latest master.

However, the problem seems not to be in Customize, but in faces.el.  Or
might be possible to fix it in faces.el, at least.

To revert to a previous face, Customize calls face-spec-set, which calls
face-spec-recalc for the face (in this case, the scroll-bar face).

For this face, this function sets all attributes to 'unspecified (via
face-spec-reset-face), and then tries to apply either a customized or
theme spec, or the face-default-spec.  But the scroll-bar face has this
definition:
(defface scroll-bar '((t nil))
  "Basic face for the scroll bar colors under X."
  :version "21.1"
  :group 'frames
  :group 'basic-faces)

So the default-attrs are nil for the scroll-bar face, resulting in
nothing else happening, which leaves the scroll-bar green.  Now, face
attributes say this:
(face-attribute 'scroll-bar :foreground nil nil) ==> nil
(face-attribute 'scroll-bar :foreground nil 'default) ==> "black"

BUT, we have this in the frame parameters:
(frame-parameter (selected-frame) 'scroll-bar-foreground) ==> "green"

The frame-parameter doesn't get updated, because we just unspecified the
:foreground attribute, we never specified something new, so this code in
internal-set-lisp-face-attribute:
if (!NILP (param))
{
 if (EQ (frame, Qt))
   /* Update `default-frame-alist', which is used for new frames.  */
   {
     store_in_alist (&Vdefault_frame_alist, param, value);
   }
 else
   /* Update the current frame's parameters.  */
   {
     AUTO_FRAME_ARG (arg, param, value);
     Fmodify_frame_parameters (frame, arg);
   }
}
doesn't run.

AFAICT, the reason the bug doesn't happen for other faces handled like
this, say the cursor face, is because the cursor face is defined like
this:
(defface cursor
  '((((background light)) :background "black")
    (((background dark))  :background "white"))
  "Basic face for the cursor color under X.
Currently, only the `:background' attribute is meaningful; all
other attributes are ignored.  The cursor foreground color is
taken from the background color of the underlying text.

Note: Other faces cannot inherit from the cursor face."
  :version "21.1"
  :group 'cursor
  :group 'basic-faces)

Meaning we do have non-nil default-attrs, and the
internal-set-lisp-face-attribute does run in this case, effectively
updating the frame parameter cursor-color.

The cursor face used to have the same definition as the scroll-bar face,
but it was changed to a "non-trivial" spec here:

commit 4983ddeaa82ea0d34b7dc9a6733f870da38b1c2e
Author: Chong Yidong <cyd@stupidchicken.com>
Date:   Wed Oct 13 23:55:18 2010 -0400

    Define a cursor defface; minor face optimizations.

    * faces.el (face-spec-reset-face): Reset all attributes in one
    single call to set-face-attribute.
    (face-spec-match-p): Make it a defsubst.
    (frame-set-background-mode): New arg KEEP-FACE-SPECS.
    (x-create-frame-with-faces, tty-create-frame-with-faces)
    (tty-set-up-initial-frame-faces): Don't recompute face specs in
    frame-set-background-mode, since they are recomputed immediately
    afterwards in face-set-after-frame-default.
    (face-set-after-frame-default): Minor optimization.
    (cursor): Provide non-trivial defface spec.

    * custom.el (custom-theme-recalc-face): Simplify.

So I'm thinking that perhaps an OK solution, at least for the scroll-bar
face, would be to give it a similar definition to that of the cursor
face.

[-- Attachment #2: Type: text/html, Size: 4838 bytes --]

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Fri, 30 Oct 2020 10:20:20 -0300
> Cc: 13476@debbugs.gnu.org
> 
> So I'm thinking that perhaps an OK solution, at least for the scroll-bar
> face, would be to give it a similar definition to that of the cursor
> face.

Yes, please try that.  If it fixes this minor problem, and doesn't
produce any unintended effects, it is IMO a fine solution for this
issue.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Date: Fri, 30 Oct 2020 10:20:20 -0300
>> Cc: 13476@debbugs.gnu.org
>>
>> So I'm thinking that perhaps an OK solution, at least for the scroll-bar
>> face, would be to give it a similar definition to that of the cursor
>> face.
>
> Yes, please try that.  If it fixes this minor problem, and doesn't
> produce any unintended effects, it is IMO a fine solution for this
> issue.

I've tried the attached patch.  Now reverting the foreground color
works, and I haven't seen unintended effects so far.

[-- Attachment #1.2: Type: text/html, Size: 862 bytes --]

[-- Attachment #2: 0001-Give-the-scroll-bar-face-a-non-trivial-spec.patch --]
[-- Type: text/x-patch, Size: 925 bytes --]

From 18e7e3002f014a154ef5e307847cd690d0277c04 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 30 Oct 2020 11:13:34 -0300
Subject: [PATCH] Give the scroll-bar face a non-trivial spec

* lisp/faces.el (scroll-bar): Give it a non-trivial spec, so when
resetting it to its face-defface-spec, we effectively reset it.
(Bug#13476)
---
 lisp/faces.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 0ce9532270..728f8b0fe6 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2716,9 +2716,11 @@ fringe
   :group 'frames
   :group 'basic-faces)
 
-(defface scroll-bar '((t nil))
+(defface scroll-bar
+  '((((background light)) :foreground "black")
+    (((background dark))  :foreground "white"))
   "Basic face for the scroll bar colors under X."
-  :version "21.1"
+  :version "28.1"
   :group 'frames
   :group 'basic-faces)
 
-- 
2.29.0

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Stephen Berman]

On Fri, 30 Oct 2020 11:18:01 -0300 Mauro Aranda <maurooaranda@gmail.com> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Mauro Aranda <maurooaranda@gmail.com>
>>> Date: Fri, 30 Oct 2020 10:20:20 -0300
>>> Cc: 13476@debbugs.gnu.org
>>>
>>> So I'm thinking that perhaps an OK solution, at least for the scroll-bar
>>> face, would be to give it a similar definition to that of the cursor
>>> face.
>>
>> Yes, please try that.  If it fixes this minor problem, and doesn't
>> produce any unintended effects, it is IMO a fine solution for this
>> issue.
>
> I've tried the attached patch.  Now reverting the foreground color
> works, and I haven't seen unintended effects so far.

I confirm it works for me too.

Steve Berman

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Fri, 30 Oct 2020 11:18:01 -0300
> Cc: Stephen Berman <stephen.berman@gmx.net>, 13476@debbugs.gnu.org
> 
> I've tried the attached patch.  Now reverting the foreground color
> works, and I haven't seen unintended effects so far.

LGTM, thanks.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

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

tags 13476 fixed
close 13476 28.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Date: Fri, 30 Oct 2020 11:18:01 -0300
>> Cc: Stephen Berman <stephen.berman@gmx.net>, 13476@debbugs.gnu.org
>>
>> I've tried the attached patch.  Now reverting the foreground color
>> works, and I haven't seen unintended effects so far.
>
> LGTM, thanks.

Pushed to master.  Closing.

[-- Attachment #2: Type: text/html, Size: 715 bytes --]

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Stefan Monnier]

>> So I'm thinking that perhaps an OK solution, at least for the scroll-bar
>> face, would be to give it a similar definition to that of the cursor
>> face.
> Yes, please try that.  If it fixes this minor problem, and doesn't
> produce any unintended effects, it is IMO a fine solution for this
> issue.

Any chance we could drop support for the `scroll-bar-foreground`
frame parameter?  It seems this might be a path towards an actual
solution (rather than the current workaround)?


        Stefan

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

[Unarchive the bug before sending a message, so that the message reaches
the bug tracker]


Po Lu <luangruo@yahoo.com> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> I've tried the attached patch.  Now reverting the foreground color
>> works, and I haven't seen unintended effects so far.
>
> I'm afraid that patch doesn't make sense, and causes bad side-effects
> with Athena and Motif scroll bars.
>
> With toolkit scroll bars, the foreground and background colors must be
> unspecified by default, so that the toolkit default is used instead.

Ok, sorry about that.

> So I think the bug lies in Custom (it should reset the value to
> "unspecified") and not the declaration of the scroll-bar face.

Why do you think it is a bug in Custom?

Custom relies on faces.el to update a customized face and to revert to
the default/standard.  So, with the old spec for scroll-bar:

(defface scroll-bar '((t nil))
  "Basic face for the scroll bar colors under X.")

;; Make sure the spec is set.
(face-spec-set 'scroll-bar '((t (:foreground "green"))) nil)
;; Reset to the standard.
(face-spec-set 'scroll-bar nil 'reset)

The scroll-bar foreground color stays green in the selected frame.

There's no Custom code involved there.

And face-spec-reset-face is setting all attributes to unspecified, I
thought that was clear from
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13476#8

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

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

Mauro Aranda <maurooaranda@gmail.com> writes:

>> So I think the bug lies in Custom (it should reset the value to
>> "unspecified") and not the declaration of the scroll-bar face.
>
> Why do you think it is a bug in Custom?

I assumed it was, but it turned out to not be the case.  A better fix
was installed on master a few hours ago, please give it a try.  Thanks.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

Po Lu <luangruo@yahoo.com> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>>> So I think the bug lies in Custom (it should reset the value to
>>> "unspecified") and not the declaration of the scroll-bar face.
>>
>> Why do you think it is a bug in Custom?
>
> I assumed it was, but it turned out to not be the case.  A better fix
> was installed on master a few hours ago, please give it a try.  Thanks.

It would have been nice to see the patch posted here.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

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

Mauro Aranda <maurooaranda@gmail.com> writes:

> It would have been nice to see the patch posted here.

branch: master
commit 66899628f8a8c79ca8dfe32094f11a8320630fae
Author: Po Lu <luangruo@yahoo.com>
Commit: Po Lu <luangruo@yahoo.com>

    Better fix for bug#13476
    
    * lisp/faces.el (face-spec-recalc): Apply scroll bar foreground
    and background to the frame if changing the scroll-bar face.
    (scroll-bar): Restore previous declaration.  That way, the
    default colors are used for toolkit scroll bars, instead of
    black and white.
---
 lisp/faces.el | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 76da210280..4b582ac439 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1743,7 +1743,14 @@ The following sources are applied in this order:
         (and tail (face-spec-set-2 face frame
                                    (list :extend (cadr tail))))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
-    (face-spec-set-2 face frame face-attrs)))
+    (face-spec-set-2 face frame face-attrs)
+    (when (and (fboundp 'set-frame-parameter) ; This isn't available
+                                              ; during loadup.
+               (eq face 'scroll-bar))
+      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
+      ;; frame parameters.  (bug#13476)
+      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
+      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))
 
 (defun face-spec-set-2 (face frame face-attrs)
   "Set the face attributes of FACE on FRAME according to FACE-ATTRS.
@@ -2826,11 +2833,9 @@ used to display the prompt text."
   :group 'frames
   :group 'basic-faces)
 
-(defface scroll-bar
-  '((((background light)) :foreground "black")
-    (((background dark))  :foreground "white"))
+(defface scroll-bar '((t nil))
   "Basic face for the scroll bar colors under X."
-  :version "28.1"
+  :version "21.1"
   :group 'frames
   :group 'basic-faces)

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

Po Lu <luangruo@yahoo.com> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> It would have been nice to see the patch posted here.
>
> branch: master
> commit 66899628f8a8c79ca8dfe32094f11a8320630fae
> Author: Po Lu <luangruo@yahoo.com>
> Commit: Po Lu <luangruo@yahoo.com>
>
>     Better fix for bug#13476
>     
>     * lisp/faces.el (face-spec-recalc): Apply scroll bar foreground
>     and background to the frame if changing the scroll-bar face.
>     (scroll-bar): Restore previous declaration.  That way, the
>     default colors are used for toolkit scroll bars, instead of
>     black and white.
> ---
>  lisp/faces.el | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/faces.el b/lisp/faces.el
> index 76da210280..4b582ac439 100644
> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -1743,7 +1743,14 @@ The following sources are applied in this order:
>          (and tail (face-spec-set-2 face frame
>                                     (list :extend (cadr tail))))))
>      (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
> -    (face-spec-set-2 face frame face-attrs)))
> +    (face-spec-set-2 face frame face-attrs)
> +    (when (and (fboundp 'set-frame-parameter) ; This isn't available
> +                                              ; during loadup.
> +               (eq face 'scroll-bar))
> +      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
> +      ;; frame parameters.  (bug#13476)
> +      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
> +      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))
>  
>  (defun face-spec-set-2 (face frame face-attrs)
>    "Set the face attributes of FACE on FRAME according to FACE-ATTRS.
> @@ -2826,11 +2833,9 @@ used to display the prompt text."
>    :group 'frames
>    :group 'basic-faces)
>  
> -(defface scroll-bar
> -  '((((background light)) :foreground "black")
> -    (((background dark))  :foreground "white"))
> +(defface scroll-bar '((t nil))
>    "Basic face for the scroll bar colors under X."
> -  :version "28.1"
> +  :version "21.1"
>    :group 'frames
>    :group 'basic-faces)

Thank you.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Cc: eliz@gnu.org,  13476@debbugs.gnu.org,  stephen.berman@gmx.net
> Date: Mon, 28 Feb 2022 09:59:15 -0300
> 
> > --- a/lisp/faces.el
> > +++ b/lisp/faces.el
> > @@ -1743,7 +1743,14 @@ The following sources are applied in this order:
> >          (and tail (face-spec-set-2 face frame
> >                                     (list :extend (cadr tail))))))
> >      (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
> > -    (face-spec-set-2 face frame face-attrs)))
> > +    (face-spec-set-2 face frame face-attrs)
> > +    (when (and (fboundp 'set-frame-parameter) ; This isn't available
> > +                                              ; during loadup.
> > +               (eq face 'scroll-bar))
> > +      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
> > +      ;; frame parameters.  (bug#13476)
> > +      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
> > +      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))

Why do we need this special treatment of the scroll-bar face?

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Cc: eliz@gnu.org,  13476@debbugs.gnu.org,  stephen.berman@gmx.net
>> Date: Mon, 28 Feb 2022 09:59:15 -0300
>> 
>> > --- a/lisp/faces.el
>> > +++ b/lisp/faces.el
>> > @@ -1743,7 +1743,14 @@ The following sources are applied in this order:
>> >          (and tail (face-spec-set-2 face frame
>> >                                     (list :extend (cadr tail))))))
>> >      (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
>> > -    (face-spec-set-2 face frame face-attrs)))
>> > +    (face-spec-set-2 face frame face-attrs)
>> > +    (when (and (fboundp 'set-frame-parameter) ; This isn't available
>> > +                                              ; during loadup.
>> > +               (eq face 'scroll-bar))
>> > +      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
>> > +      ;; frame parameters.  (bug#13476)
>> > +      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
>> > +      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))
>
> Why do we need this special treatment of the scroll-bar face?

I haven't read the code yet so I can't really answer to your question.  What
I know is what I said on
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13476#8

Customizing and then resetting to the standard scroll-bar face failed
because the 'scroll-bar-foreground parameter wasn't updated after
resetting the attributes via face-spec-reset-face.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Cc: luangruo@yahoo.com, 13476@debbugs.gnu.org, stephen.berman@gmx.net
> Date: Mon, 28 Feb 2022 11:04:13 -0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Mauro Aranda <maurooaranda@gmail.com>
> >> Cc: eliz@gnu.org,  13476@debbugs.gnu.org,  stephen.berman@gmx.net
> >> Date: Mon, 28 Feb 2022 09:59:15 -0300
> >> 
> >> > --- a/lisp/faces.el
> >> > +++ b/lisp/faces.el
> >> > @@ -1743,7 +1743,14 @@ The following sources are applied in this order:
> >> >          (and tail (face-spec-set-2 face frame
> >> >                                     (list :extend (cadr tail))))))
> >> >      (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
> >> > -    (face-spec-set-2 face frame face-attrs)))
> >> > +    (face-spec-set-2 face frame face-attrs)
> >> > +    (when (and (fboundp 'set-frame-parameter) ; This isn't available
> >> > +                                              ; during loadup.
> >> > +               (eq face 'scroll-bar))
> >> > +      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
> >> > +      ;; frame parameters.  (bug#13476)
> >> > +      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
> >> > +      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))
> >
> > Why do we need this special treatment of the scroll-bar face?
> 
> I haven't read the code yet so I can't really answer to your question.  What
> I know is what I said on
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13476#8
> 
> Customizing and then resetting to the standard scroll-bar face failed
> because the 'scroll-bar-foreground parameter wasn't updated after
> resetting the attributes via face-spec-reset-face.

I don't understand why we need to set the frame parameter when the
customize the face to begin with, I guess.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Cc: luangruo@yahoo.com, 13476@debbugs.gnu.org, stephen.berman@gmx.net
>> Date: Mon, 28 Feb 2022 11:04:13 -0300
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Mauro Aranda <maurooaranda@gmail.com>
>> >> Cc: eliz@gnu.org,  13476@debbugs.gnu.org,  stephen.berman@gmx.net
>> >> Date: Mon, 28 Feb 2022 09:59:15 -0300
>> >> 
>> >> > --- a/lisp/faces.el
>> >> > +++ b/lisp/faces.el
>> >> > @@ -1743,7 +1743,14 @@ The following sources are applied in this order:
>> >> >          (and tail (face-spec-set-2 face frame
>> >> >                                     (list :extend (cadr tail))))))
>> >> >      (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
>> >> > -    (face-spec-set-2 face frame face-attrs)))
>> >> > +    (face-spec-set-2 face frame face-attrs)
>> >> > +    (when (and (fboundp 'set-frame-parameter) ; This isn't available
>> >> > +                                              ; during loadup.
>> >> > +               (eq face 'scroll-bar))
>> >> > +      ;; Set the `scroll-bar-foreground' and `scroll-bar-background'
>> >> > +      ;; frame parameters.  (bug#13476)
>> >> > +      (set-frame-parameter frame 'scroll-bar-foreground (face-foreground face))
>> >> > +      (set-frame-parameter frame 'scroll-bar-background (face-background face)))))
>> >
>> > Why do we need this special treatment of the scroll-bar face?
>> 
>> I haven't read the code yet so I can't really answer to your question.  What
>> I know is what I said on
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13476#8
>> 
>> Customizing and then resetting to the standard scroll-bar face failed
>> because the 'scroll-bar-foreground parameter wasn't updated after
>> resetting the attributes via face-spec-reset-face.
>
> I don't understand why we need to set the frame parameter when the
> customize the face to begin with, I guess.

In an Emacs prior to c307c9648d541338814fe541389ea8c7a1cf50a5 and
configured with: --without-toolkit-scroll-bars

M-x customize-face RET scroll-bar
Customize the foreground color to "green", and set for current session.
Click the State button to "Revert This Session's Customization".
The scroll bar stays green, even when the foreground color says it is
black.

Evaluate the following:
(face-attribute 'scroll-bar :foreground nil 'default) ; ==> "black"
(frame-parameter (selected-frame) 'scroll-bar-foreground) ; ==> "green"

So, AFAIU the reason the scroll bar stayed green even after the attempt
to go back to the standard was that the frame parameter didn't change
after evaluating
(face-spec-reset 'scroll-bar nil 'reset)

Giving the scroll-bar face a non-trivial spec worked because it caused
some code in internal-set-lisp-face-attribute to update the
frame parameter, but it looks like it caused bad side effects when using
some toolkits.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

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

Eli Zaretskii <eliz@gnu.org> writes:

> I don't understand why we need to set the frame parameter when the
> customize the face to begin with, I guess.

Because that's what you would expect to happen: customizing the
foreground or background of the scroll-bar face should affect the scroll
bar colors, which are frame parameters.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Mauro Aranda <maurooaranda@gmail.com>,  13476@debbugs.gnu.org,
>   stephen.berman@gmx.net
> Date: Tue, 01 Mar 2022 08:30:15 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't understand why we need to set the frame parameter when the
> > customize the face to begin with, I guess.
> 
> Because that's what you would expect to happen: customizing the
> foreground or background of the scroll-bar face should affect the scroll
> bar colors, which are frame parameters.

So this face is special in that we handle it via frame parameters?  In
that case, please add a comment to that effect where you made the
change.

bug#13476: 24.3.50; Reverting scroll-bar face customization has no effect

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

Eli Zaretskii <eliz@gnu.org> writes:

> So this face is special in that we handle it via frame parameters?  In
> that case, please add a comment to that effect where you made the
> change.

Thanks, will do.