bug#67404: 30.0.50; Issue with :box face attribute

bug#67404: 30.0.50; Issue with :box face attribute

[David Ponce]

Hello,

The below face with specified :box attribute can be defined and further
used without problem:

(defface test-face
   '((t
      (:box (:style released-button :line-width (3 . 0)))
      ))
   "Test face.")

(insert (propertize "XXXXX" 'face 'test-face))

However, it is not possible to apply any change to this face with
`customize-face', for example to change :line-width (3 . 0) to
:line-width (2 . 0), or to add a :foreground color, etc.  This fails
with the message "set-face-attribute: Invalid face box: :line-width,
<value>, :style, <value>", and the :box attribute is discarded.

AFAICS, :box attribute with any zero :line-width value works as
expected: the box line with zero width is not displayed.  But zero is
not considered as a valid value by `set-face-attribute' (in fact, the
internal function `internal-set-lisp-face-attribute').

For consistency, a zero :line-width value either should be accepted by
`internal-set-lisp-face-attribute', or never accepted at all.

Thanks


In GNU Emacs 30.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
  3.24.38, cairo version 1.18.0) of 2023-11-21
Repository revision: 3328c327254b5846782990f6171175bea216eba9
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 39 (KDE Plasma)

Configured using:
  'configure --with-x-toolkit=gtk3 --with-cairo-xcb
  --with-native-compilation=no
  PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

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

bug#67404: 30.0.50; Issue with :box face attribute

[Eli Zaretskii]

> Date: Thu, 23 Nov 2023 16:23:15 +0100
> From: David Ponce <da_vid@orange.fr>
> 
> Hello,
> 
> The below face with specified :box attribute can be defined and further
> used without problem:
> 
> (defface test-face
>    '((t
>       (:box (:style released-button :line-width (3 . 0)))
>       ))
>    "Test face.")
> 
> (insert (propertize "XXXXX" 'face 'test-face))
> 
> However, it is not possible to apply any change to this face with
> `customize-face', for example to change :line-width (3 . 0) to
> :line-width (2 . 0), or to add a :foreground color, etc.  This fails
> with the message "set-face-attribute: Invalid face box: :line-width,
> <value>, :style, <value>", and the :box attribute is discarded.
> 
> AFAICS, :box attribute with any zero :line-width value works as
> expected: the box line with zero width is not displayed.  But zero is
> not considered as a valid value by `set-face-attribute' (in fact, the
> internal function `internal-set-lisp-face-attribute').
> 
> For consistency, a zero :line-width value either should be accepted by
> `internal-set-lisp-face-attribute', or never accepted at all.

Mauro, any comments about these issues?

bug#67404: 30.0.50; Issue with :box face attribute

[Mauro Aranda]

On 26/11/23 07:27, Eli Zaretskii wrote:

 >> Date: Thu, 23 Nov 2023 16:23:15 +0100
 >> From: David Ponce <da_vid@orange.fr>
 >>
 >> Hello,
 >>
 >> The below face with specified :box attribute can be defined and further
 >> used without problem:
 >>
 >> (defface test-face
 >>    '((t
 >>       (:box (:style released-button :line-width (3 . 0)))
 >>       ))
 >>    "Test face.")
 >>
 >> (insert (propertize "XXXXX" 'face 'test-face))
 >>
 >> However, it is not possible to apply any change to this face with
 >> `customize-face', for example to change :line-width (3 . 0) to
 >> :line-width (2 . 0), or to add a :foreground color, etc. This fails
 >> with the message "set-face-attribute: Invalid face box: :line-width,
 >> <value>, :style, <value>", and the :box attribute is discarded.
 >>
 >> AFAICS, :box attribute with any zero :line-width value works as
 >> expected: the box line with zero width is not displayed. But zero is
 >> not considered as a valid value by `set-face-attribute' (in fact, the
 >> internal function `internal-set-lisp-face-attribute').
 >>
 >> For consistency, a zero :line-width value either should be accepted by
 >> `internal-set-lisp-face-attribute', or never accepted at all.
 >
 > Mauro, any comments about these issues?

Hi Eli,

Not really, no.  The widget used for line-width just assumes 0 is a
valid value.  I don't know if that's the case or not, it is not
explicitly documented in the ELisp manual and I don't really know the
xfaces.c code.

For some reason, the code that rejects a value of 0 in
Finternal_set_lisp_face_attribute doesn't complain (maybe it does not
get to run?) when using defface, but it does complain when Customize
tries to set it, by calling face-spec-set.

Sorry I can't be of any more help right now.

bug#67404: 30.0.50; Issue with :box face attribute

[Eli Zaretskii]

> Date: Thu, 30 Nov 2023 07:46:02 -0300
> Cc: 67404@debbugs.gnu.org
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> On 26/11/23 07:27, Eli Zaretskii wrote:
> 
>  >> Date: Thu, 23 Nov 2023 16:23:15 +0100
>  >> From: David Ponce <da_vid@orange.fr>
>  >>
>  >> Hello,
>  >>
>  >> The below face with specified :box attribute can be defined and further
>  >> used without problem:
>  >>
>  >> (defface test-face
>  >>    '((t
>  >>       (:box (:style released-button :line-width (3 . 0)))
>  >>       ))
>  >>    "Test face.")
>  >>
>  >> (insert (propertize "XXXXX" 'face 'test-face))
>  >>
>  >> However, it is not possible to apply any change to this face with
>  >> `customize-face', for example to change :line-width (3 . 0) to
>  >> :line-width (2 . 0), or to add a :foreground color, etc. This fails
>  >> with the message "set-face-attribute: Invalid face box: :line-width,
>  >> <value>, :style, <value>", and the :box attribute is discarded.
>  >>
>  >> AFAICS, :box attribute with any zero :line-width value works as
>  >> expected: the box line with zero width is not displayed. But zero is
>  >> not considered as a valid value by `set-face-attribute' (in fact, the
>  >> internal function `internal-set-lisp-face-attribute').
>  >>
>  >> For consistency, a zero :line-width value either should be accepted by
>  >> `internal-set-lisp-face-attribute', or never accepted at all.
>  >
>  > Mauro, any comments about these issues?
> 
> Hi Eli,
> 
> Not really, no.  The widget used for line-width just assumes 0 is a
> valid value.  I don't know if that's the case or not, it is not
> explicitly documented in the ELisp manual and I don't really know the
> xfaces.c code.
> 
> For some reason, the code that rejects a value of 0 in
> Finternal_set_lisp_face_attribute doesn't complain (maybe it does not
> get to run?) when using defface, but it does complain when Customize
> tries to set it, by calling face-spec-set.

Thanks for the hints.  This is a (very old) bug in
Finternal_set_lisp_face_attribute.  The value zero for line-width is
not valid, but because of the bug it (and invalid values of other box
attributes) could be accepted, but only if the invalid attribute is
the last attribute of :box.  Try this equivalent definition:

(defface test-face
   '((t
      (:box (:line-width (3 . 0) :style released-button))
      ))
   "Test face.")

which just swaps the :box attributes, and you will see that defface
signals an error.  Customizing the :line-width attribute just happens
to move that attribute to the front of the list, and then you get the
error.

I've now fixed this on the master branch, and now zero width values
will always be rejected.

bug#67404: 30.0.50; Issue with :box face attribute

[David Ponce]

On 30/11/2023 16:20, Eli Zaretskii wrote:
>> Date: Thu, 30 Nov 2023 07:46:02 -0300
>> Cc: 67404@debbugs.gnu.org
>> From: Mauro Aranda <maurooaranda@gmail.com>
>>
>> On 26/11/23 07:27, Eli Zaretskii wrote:
>>
>>   >> Date: Thu, 23 Nov 2023 16:23:15 +0100
>>   >> From: David Ponce <da_vid@orange.fr>
>>   >>
>>   >> Hello,
>>   >>
>>   >> The below face with specified :box attribute can be defined and further
>>   >> used without problem:
>>   >>
>>   >> (defface test-face
>>   >>    '((t
>>   >>       (:box (:style released-button :line-width (3 . 0)))
>>   >>       ))
>>   >>    "Test face.")
>>   >>
>>   >> (insert (propertize "XXXXX" 'face 'test-face))
>>   >>
>>   >> However, it is not possible to apply any change to this face with
>>   >> `customize-face', for example to change :line-width (3 . 0) to
>>   >> :line-width (2 . 0), or to add a :foreground color, etc. This fails
>>   >> with the message "set-face-attribute: Invalid face box: :line-width,
>>   >> <value>, :style, <value>", and the :box attribute is discarded.
>>   >>
>>   >> AFAICS, :box attribute with any zero :line-width value works as
>>   >> expected: the box line with zero width is not displayed. But zero is
>>   >> not considered as a valid value by `set-face-attribute' (in fact, the
>>   >> internal function `internal-set-lisp-face-attribute').
>>   >>
>>   >> For consistency, a zero :line-width value either should be accepted by
>>   >> `internal-set-lisp-face-attribute', or never accepted at all.
>>   >
>>   > Mauro, any comments about these issues?
>>
>> Hi Eli,
>>
>> Not really, no.  The widget used for line-width just assumes 0 is a
>> valid value.  I don't know if that's the case or not, it is not
>> explicitly documented in the ELisp manual and I don't really know the
>> xfaces.c code.
>>
>> For some reason, the code that rejects a value of 0 in
>> Finternal_set_lisp_face_attribute doesn't complain (maybe it does not
>> get to run?) when using defface, but it does complain when Customize
>> tries to set it, by calling face-spec-set.
> 
> Thanks for the hints.  This is a (very old) bug in
> Finternal_set_lisp_face_attribute.  The value zero for line-width is
> not valid, but because of the bug it (and invalid values of other box
> attributes) could be accepted, but only if the invalid attribute is
> the last attribute of :box.  Try this equivalent definition:
> 
> (defface test-face
>     '((t
>        (:box (:line-width (3 . 0) :style released-button))
>        ))
>     "Test face.")
> 
> which just swaps the :box attributes, and you will see that defface
> signals an error.  Customizing the :line-width attribute just happens
> to move that attribute to the front of the list, and then you get the
> error.
> 
> I've now fixed this on the master branch, and now zero width values
> will always be rejected.

Hi Eli,

I confirm that with your fix on master branch, the behavior is now
consistent between defface and customize-face: both signal an error
on value zero for line-width :-)

However, please note that it remains possible to use a value zero for
line-width on an anonymous face.  For example, in the *scratch* buffer
you still can eval the below expression without error:
  
(insert
   (propertize "xxxx" 'font-lock-face
               '(:box (:style released-button :line-width (3 . 0)))))

Thanks!

bug#67404: 30.0.50; Issue with :box face attribute

[Eli Zaretskii]

> Date: Thu, 30 Nov 2023 17:20:44 +0100
> Cc: 67404@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> I confirm that with your fix on master branch, the behavior is now
> consistent between defface and customize-face: both signal an error
> on value zero for line-width :-)

Thanks for testing, I'm therefore closing this bug.

> However, please note that it remains possible to use a value zero for
> line-width on an anonymous face.  For example, in the *scratch* buffer
> you still can eval the below expression without error:
>   
> (insert
>    (propertize "xxxx" 'font-lock-face
>                '(:box (:style released-button :line-width (3 . 0)))))

This seems to be a general issue with anonymous faces: their
attributes are validated much more leniently than those of defface.
For example, try:

  (insert
     (propertize "xxxx" 'font-lock-face
		 '(:box (:style foobar :line-width 'bogus))))
or

  (insert
     (propertize "xxxx" 'font-lock-face
		 '(:underline (foo bar :style bogus :position))))

You get a face that effectively ignores the invalid attributes.  I'm
not sure we need to change that.