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

From: Mauro Aranda <[email-masked]>
Date: Fri, 30 Oct 2020 10:20:20 -0300

Stephen Berman <[email-masked]> 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 <[email-masked]>
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.