[RFC] Add :invisible face attribute

[RFC] Add :invisible face attribute

[Michal Nazarewicz]

Introduce :invisible face attribute which makes foreground to be the
same as background rendering the text invisible; or when :invert-video
is also in effect, background is the same as foreground.

Use it in Org mode for org-hide face eliminting the need for
org-find-invisible-foreground function.  This also simplifies
auto-dim-other-buffers NonGNU ELPA package removing the need to
configure a separate hide face for org-hide remap.

To observe the atribute in action, set ‘org-hide-leading-stars’ option,
open NEWS file and enable org-mode.  The initial stars in section
headings are rendered using the new attribute rather than Org mode
needing to explicitly match foregroun to background.

---
 lisp/cus-face.el      |  6 ++++
 lisp/faces.el         | 32 +++++++++++++++--
 lisp/org/org-faces.el |  4 +--
 lisp/org/org.el       | 18 ----------
 src/dispextern.h      |  1 +
 src/xfaces.c          | 80 +++++++++++++++++++++++++++++++++++++------
 6 files changed, 108 insertions(+), 33 deletions(-)

diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index 478092c30cb..b7000747393 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -251,6 +251,12 @@ custom-face-attributes
 	     (const :tag "Off" nil)
 	     (const :tag "On" t)))
 
+    (:invisible
+     (choice :tag "Invisible"
+	     :help-echo "Control whether text should be rendered in the same color as background."
+	     (const :tag "Off" nil)
+	     (const :tag "On" t)))
+
     (:foreground
      (color :tag "Foreground"
 	    :help-echo "Set foreground color (name or #RRGGBB hex spec)."))
diff --git a/lisp/faces.el b/lisp/faces.el
index 05df685c679..6451664d791 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -375,6 +375,7 @@ face-x-resources
     (:underline (".attributeUnderline" . "Face.AttributeUnderline"))
     (:inverse-video (".attributeInverse" . "Face.AttributeInverse"))
     (:extend (".attributeExtend" . "Face.AttributeExtend"))
+    (:invisible (".attributeInvisible" . "Face.AttributeInvisible"))
     (:stipple
      (".attributeStipple" . "Face.AttributeStipple")
      (".attributeBackgroundPixmap" . "Face.AttributeBackgroundPixmap"))
@@ -615,6 +616,15 @@ face-inverse-video-p
  (eq (face-attribute face :inverse-video frame inherit) t))
 
 
+(defun face-invisible-video-p (face &optional frame inherit)
+ "Return non-nil if FACE specifies a non-nil invisible.
+If the optional argument FRAME is given, report on face FACE in that frame.
+If FRAME is t, report on the defaults for face FACE (for new frames).
+If FRAME is omitted or nil, use the selected frame.
+Optional argument INHERIT is passed to `face-attribute'."
+ (eq (face-attribute face :invisible frame inherit) t))
+
+
 (defun face-bold-p (face &optional frame inherit)
   "Return non-nil if the font of FACE is bold on FRAME.
 If the optional argument FRAME is given, report on face FACE in that frame.
@@ -822,6 +832,12 @@ set-face-attribute
 VALUE specifies whether characters in FACE should be displayed in
 inverse video.  VALUE must be one of t or nil.
 
+`:invisible'
+
+VALUE specifies whether characters in FACE should be displayed in
+foreground color the same as background color rendering them
+invisible.  VALUE must be one of t or nil.
+
 `:stipple'
 
 If VALUE is a string, it must be the name of a file of pixmap data.
@@ -1038,6 +1054,17 @@ set-face-inverse-video
 (define-obsolete-function-alias 'set-face-inverse-video-p
                                 'set-face-inverse-video "24.4")
 
+(defun set-face-invisible (face invisible-p &optional frame)
+  "Specify whether face FACE is in invisible.
+INVISIBLE-P non-nil means FACE displays explicitly in invisible.
+INVISIBLE-P nil means FACE explicitly is not in invisible.
+FRAME nil or not specified means change face on all frames.
+Use `set-face-attribute' to \"unspecify\" the invisible attribute."
+  (interactive
+   (let ((list (read-face-and-attribute :invisible)))
+     (list (car list) (if (cadr list) t))))
+  (set-face-attribute face frame :invisible invisible-p))
+
 (defun set-face-bold (face bold-p &optional frame)
   "Specify whether face FACE is bold.
 BOLD-P non-nil means FACE should explicitly display bold.
@@ -1215,7 +1242,7 @@ face-valid-attribute-values
 	   (:slant
             (mapcar (lambda (x) (cons (symbol-name (aref x 1)) (aref x 1)))
 		    font-slant-table))
-	   ((or :inverse-video :extend)
+	   ((or :inverse-video :extend :invisible)
             (mapcar (lambda (x) (cons (symbol-name x) x))
 		    (internal-lisp-face-attribute-values attribute)))
            ((or :underline :overline :strike-through :box)
@@ -1265,6 +1292,7 @@ face-attribute-name-alist
     (:strike-through . "strike-through")
     (:box . "box")
     (:inverse-video . "inverse-video display")
+    (:invisible . "invisible text")
     (:foreground . "foreground color")
     (:background . "background color")
     (:stipple . "background stipple")
@@ -1668,7 +1696,7 @@ face-spec-reset-face
 	     (append
 	      '(:underline nil :overline nil :strike-through nil
 		:box nil :inverse-video nil :stipple nil :inherit nil
-                :extend nil)
+                :extend nil :invisible nil)
 	      ;; `display-graphic-p' is unavailable when running
 	      ;; temacs, prior to loading frame.el.
 	      (when (fboundp 'display-graphic-p)
diff --git a/lisp/org/org-faces.el b/lisp/org/org-faces.el
index 21b23b641ca..ad6af62526d 100644
--- a/lisp/org/org-faces.el
+++ b/lisp/org/org-faces.el
@@ -40,9 +40,7 @@ org-default
   "Face used for default text."
   :group 'org-faces)
 
-(defface org-hide
-  '((((background light)) (:foreground "white"))
-    (((background dark)) (:foreground "black")))
+(defface org-hide '((t :invisible t))
   "Face used to hide leading stars in headlines.
 The foreground color of this face should be equal to the background
 color of the frame."
diff --git a/lisp/org/org.el b/lisp/org/org.el
index 4166738c162..782c2818c82 100644
--- a/lisp/org/org.el
+++ b/lisp/org/org.el
@@ -5100,10 +5100,6 @@ org-mode
   ;; Activate `org-table-header-line-mode'
   (when org-table-header-line-p
     (org-table-header-line-mode 1))
-  ;; Try to set `org-hide' face correctly.
-  (let ((foreground (org-find-invisible-foreground)))
-    (when foreground
-      (set-face-foreground 'org-hide foreground)))
   ;; Set face extension as requested.
   (org--set-faces-extend '(org-block-begin-line org-block-end-line)
                          org-fontify-whole-block-delimiter-line)
@@ -5138,20 +5134,6 @@ org-mode-transpose-word-syntax-table
   (abbrev-table-put org-mode-abbrev-table
 		    :parents (list text-mode-abbrev-table)))
 
-(defun org-find-invisible-foreground ()
-  (let ((candidates (remove
-		     "unspecified-bg"
-		     (nconc
-		      (list (face-background 'default)
-			    (face-background 'org-default))
-		      (mapcar
-		       (lambda (alist)
-			 (when (boundp alist)
-			   (cdr (assq 'background-color (symbol-value alist)))))
-		       '(default-frame-alist initial-frame-alist window-system-default-frame-alist))
-		      (list (face-foreground 'org-hide))))))
-    (car (remove nil candidates))))
-
 (defun org-current-time (&optional rounding-minutes past)
   "Current time, possibly rounded to ROUNDING-MINUTES.
 When ROUNDING-MINUTES is not an integer, fall back on the car of
diff --git a/src/dispextern.h b/src/dispextern.h
index 9df6eaf623a..2adb2686188 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1700,6 +1700,7 @@ #define FONT_TOO_HIGH(ft)						\
   LFACE_FONTSET_INDEX,
   LFACE_DISTANT_FOREGROUND_INDEX,
   LFACE_EXTEND_INDEX,
+  LFACE_INVISIBLE_INDEX,
   LFACE_VECTOR_SIZE
 };
 
diff --git a/src/xfaces.c b/src/xfaces.c
index f6264802fa4..1b89bd4eb04 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -1378,6 +1378,7 @@ load_face_colors (struct frame *f, struct face *face,
 {
   Lisp_Object fg, bg, dfg;
   Emacs_Color xfg, xbg;
+  bool invisible;
 
   bg = attrs[LFACE_BACKGROUND_INDEX];
   fg = attrs[LFACE_FOREGROUND_INDEX];
@@ -1391,6 +1392,11 @@ load_face_colors (struct frame *f, struct face *face,
       bg = tmp;
     }
 
+  /* Set foreground to background if invisible.  */
+  invisible = EQ (attrs[LFACE_INVISIBLE_INDEX], Qt);
+  if (invisible)
+    fg = bg;
+
   /* Check for support for foreground, not for background because
      face_color_supported_p is smart enough to know that grays are
      "supported" as background because we are supposed to use stipple
@@ -1405,14 +1411,17 @@ load_face_colors (struct frame *f, struct face *face,
   face->background = load_color2 (f, face, bg, LFACE_BACKGROUND_INDEX, &xbg);
   face->foreground = load_color2 (f, face, fg, LFACE_FOREGROUND_INDEX, &xfg);
 
-  dfg = attrs[LFACE_DISTANT_FOREGROUND_INDEX];
-  if (!NILP (dfg) && !UNSPECIFIEDP (dfg)
-      && color_distance (&xbg, &xfg) < face_near_same_color_threshold)
+  if (!invisible)
     {
-      if (EQ (attrs[LFACE_INVERSE_INDEX], Qt))
-        face->background = load_color (f, face, dfg, LFACE_BACKGROUND_INDEX);
-      else
-        face->foreground = load_color (f, face, dfg, LFACE_FOREGROUND_INDEX);
+      dfg = attrs[LFACE_DISTANT_FOREGROUND_INDEX];
+      if (!NILP (dfg) && !UNSPECIFIEDP (dfg)
+          && color_distance (&xbg, &xfg) < face_near_same_color_threshold)
+        {
+          if (EQ (attrs[LFACE_INVERSE_INDEX], Qt))
+            face->background = load_color (f, face, dfg, LFACE_BACKGROUND_INDEX);
+          else
+            face->foreground = load_color (f, face, dfg, LFACE_FOREGROUND_INDEX);
+        }
     }
 }
 
@@ -1803,6 +1812,7 @@ #define LFACE_FONTSET(LFACE)	    AREF (LFACE, LFACE_FONTSET_INDEX)
 #define LFACE_EXTEND(LFACE)	    AREF (LFACE, LFACE_EXTEND_INDEX)
 #define LFACE_DISTANT_FOREGROUND(LFACE) \
   AREF (LFACE, LFACE_DISTANT_FOREGROUND_INDEX)
+#define LFACE_INVISIBLE(LFACE)	    AREF (LFACE, LFACE_INVISIBLE_INDEX)
 
 /* True if LFACE is a Lisp face.  A Lisp face is a vector of size
    LFACE_VECTOR_SIZE which has the symbol `face' in slot 0.  */
@@ -1912,6 +1922,10 @@ check_lface_attrs (Lisp_Object attrs[LFACE_VECTOR_SIZE])
 	   || RESET_P (attrs[LFACE_FONTSET_INDEX])
 	   || NILP (attrs[LFACE_FONTSET_INDEX]));
 #endif
+  eassert (UNSPECIFIEDP (attrs[LFACE_INVISIBLE_INDEX])
+	   || IGNORE_DEFFACE_P (attrs[LFACE_INVISIBLE_INDEX])
+	   || RESET_P (attrs[LFACE_INVISIBLE_INDEX])
+	   || SYMBOLP (attrs[LFACE_INVISIBLE_INDEX]));
 }
 
 
@@ -2911,6 +2925,13 @@ merge_face_ref (struct window *w,
 		  else
 		    err = true;
 		}
+	      else if (EQ (keyword, QCinvisible))
+		{
+		  if (EQ (value, Qt) || NILP (value))
+		    to[LFACE_INVISIBLE_INDEX] = value;
+		  else
+		    err = true;
+		}
 	      else
 		err = true;
 
@@ -3484,6 +3505,19 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
       old_value = LFACE_EXTEND (lface);
       ASET (lface, LFACE_EXTEND_INDEX, value);
     }
+  else if (EQ (attr, QCinvisible))
+    {
+      if (!UNSPECIFIEDP (value)
+	  && !IGNORE_DEFFACE_P (value)
+	  && !RESET_P (value))
+	{
+	  CHECK_SYMBOL (value);
+	  if (!EQ (value, Qt) && !NILP (value))
+	    signal_error ("Invalid invisible face attribute value", value);
+	}
+      old_value = LFACE_INVISIBLE (lface);
+      ASET (lface, LFACE_INVISIBLE_INDEX, value);
+    }
   else if (EQ (attr, QCforeground))
     {
       HANDLE_INVALID_NIL_VALUE (QCforeground, face);
@@ -3977,7 +4011,8 @@ DEFUN ("internal-set-lisp-face-attribute-from-resource",
   else if (EQ (attr, QCweight) || EQ (attr, QCslant) || EQ (attr, QCwidth))
     value = intern (SSDATA (value));
   else if (EQ (attr, QCinverse_video)
-           || EQ (attr, QCextend))
+           || EQ (attr, QCextend)
+           || EQ (attr, QCinvisible))
     value = face_boolean_x_resource_value (value, true);
   else if (EQ (attr, QCunderline)
 	   || EQ (attr, QCoverline)
@@ -4203,6 +4238,8 @@ DEFUN ("internal-get-lisp-face-attribute", Finternal_get_lisp_face_attribute,
     value = LFACE_INHERIT (lface);
   else if (EQ (keyword, QCextend))
     value = LFACE_EXTEND (lface);
+  else if (EQ (keyword, QCinvisible))
+    value = LFACE_INVISIBLE (lface);
   else if (EQ (keyword, QCfont))
     value = LFACE_FONT (lface);
   else if (EQ (keyword, QCfontset))
@@ -4231,7 +4268,8 @@ DEFUN ("internal-lisp-face-attribute-values",
   if (EQ (attr, QCunderline) || EQ (attr, QCoverline)
       || EQ (attr, QCstrike_through)
       || EQ (attr, QCinverse_video)
-      || EQ (attr, QCextend))
+      || EQ (attr, QCextend)
+      || EQ (attr, QCinvisible))
     result = list2 (Qt, Qnil);
 
   return result;
@@ -5360,7 +5398,10 @@ gui_supports_face_attributes_p (struct frame *f,
 				def_attrs[LFACE_STRIKE_THROUGH_INDEX]))
       || (!UNSPECIFIEDP (lattrs[LFACE_BOX_INDEX])
 	  && face_attr_equal_p (lattrs[LFACE_BOX_INDEX],
-				def_attrs[LFACE_BOX_INDEX])))
+				def_attrs[LFACE_BOX_INDEX]))
+      || (!UNSPECIFIEDP (lattrs[LFACE_INVISIBLE_INDEX])
+	  && face_attr_equal_p (lattrs[LFACE_INVISIBLE_INDEX],
+				def_attrs[LFACE_INVISIBLE_INDEX])))
     return false;
 
   /* Check font-related attributes, as those are the most commonly
@@ -5549,6 +5590,17 @@ tty_supports_face_attributes_p (struct frame *f,
 	test_caps |= TTY_CAP_STRIKE_THROUGH;
     }
 
+  /* invisible video */
+  val = attrs[LFACE_INVERSE_INDEX];
+  if (!UNSPECIFIEDP (val))
+    {
+      if (face_attr_equal_p (val, def_attrs[LFACE_INVERSE_INDEX]))
+	return false;		/* same as default */
+      /* Always supported since it just copies bg to fg.  XXX: Is this
+	 true?  I actually have no idea what this function is supposed
+	 to do. */
+    }
+
   /* Color testing.  */
 
   /* Check if foreground color is close enough.  */
@@ -5968,6 +6020,9 @@ realize_default_face (struct frame *f)
   if (UNSPECIFIEDP (LFACE_INVERSE (lface)))
     ASET (lface, LFACE_INVERSE_INDEX, Qnil);
 
+  if (UNSPECIFIEDP (LFACE_INVISIBLE (lface)))
+    ASET (lface, LFACE_INVISIBLE_INDEX, Qnil);
+
   if (UNSPECIFIEDP (LFACE_FOREGROUND (lface)))
     {
       /* This function is called so early that colors are not yet
@@ -6716,6 +6771,9 @@ realize_tty_face (struct face_cache *cache,
       face->background = tem;
     }
 
+  if (!NILP (attrs[LFACE_INVISIBLE_INDEX]))
+    face->foreground = face->background;
+
   if (tty_suppress_bold_inverse_default_colors_p
       && face->tty_bold_p
       && face->background == FACE_TTY_DEFAULT_FG_COLOR
@@ -7342,6 +7400,7 @@ init_xfaces (void)
   face_attr_sym[LFACE_FONTSET_INDEX] = QCfontset;
   face_attr_sym[LFACE_DISTANT_FOREGROUND_INDEX] = QCdistant_foreground;
   face_attr_sym[LFACE_EXTEND_INDEX] = QCextend;
+  face_attr_sym[LFACE_INVISIBLE_INDEX] = QCinvisible;
 }
 
 void
@@ -7381,6 +7440,7 @@ syms_of_xfaces (void)
   DEFSYM (QCbox, ":box");
   DEFSYM (QCinherit, ":inherit");
   DEFSYM (QCextend, ":extend");
+  DEFSYM (QCinvisible, ":invisible");
 
   /* Symbols used for Lisp face attribute values.  */
   DEFSYM (QCcolor, ":color");
-- 
2.45.2

Re: [RFC] Add :invisible face attribute

[Eli Zaretskii]

> From: Michal Nazarewicz <mina86@mina86.com>
> Date: Wed, 18 Dec 2024 17:08:12 +0100
> 
> Introduce :invisible face attribute which makes foreground to be the
> same as background rendering the text invisible; or when :invert-video
> is also in effect, background is the same as foreground.
> 
> Use it in Org mode for org-hide face eliminting the need for
> org-find-invisible-foreground function.  This also simplifies
> auto-dim-other-buffers NonGNU ELPA package removing the need to
> configure a separate hide face for org-hide remap.
> 
> To observe the atribute in action, set ‘org-hide-leading-stars’ option,
> open NEWS file and enable org-mode.  The initial stars in section
> headings are rendered using the new attribute rather than Org mode
> needing to explicitly match foregroun to background.

Thanks.

A new face attribute adds quite a bit of complexity, so we should
really believe this is a good idea.  Given that this can already be
achieved without a new attribute, I'm not sure.  Is this really a
frequent need/situation?

But let's see what others think about this.

> @@ -2911,6 +2925,13 @@ merge_face_ref (struct window *w,
>  		  else
>  		    err = true;
>  		}
> +	      else if (EQ (keyword, QCinvisible))
> +		{
> +		  if (EQ (value, Qt) || NILP (value))
> +		    to[LFACE_INVISIBLE_INDEX] = value;
> +		  else
> +		    err = true;
> +		}

Does this mean that the result of merging two faces with different
values for :invisible will depend on the order of the merge?  That is,
does the nil value override the t value?  Or am I missing something?

Btw, I think "invisible" is not the best name for this, because that's
not really what will happen on display.  It's more like "illegible" or
something to that effect.

Re: [RFC] Add :invisible face attribute

[Michal Nazarewicz]

On Wed, Dec 18 2024, Eli Zaretskii wrote:
> A new face attribute adds quite a bit of complexity, so we should
> really believe this is a good idea.  Given that this can already be
> achieved without a new attribute, I'm not sure.  Is this really a
> frequent need/situation?

Yes, agreed.  org-hide is the only face I’m aware of and this only came
up as I was working on auto-dim-other-buffers.

To quickly recap, auto-dim-other-buffers adds a face remap to default
face which changes its background.  The problem is that if default
background is changed, text rendered with org-hide becomes visible.  So
now I also need to remap org-hide.

This by itself I could live with, but it creates friction for users who
need to not only customise auto-dim-other-buffers-face (which specifies
the remap background) but also auto-dim-other-buffers-hide-face.

There are actually two minor bugs in Org mode that this patch fixes.
First is when default face is changed after org-mode is initialised:

    (setq org-hide-leading-stars t)
    (org-mode)
    (insert "\n*** Foo\n")        ; the first two stars are invisible
    (set-face-background 'default "red")  ; the starts become visible

Second is when running with frames on multiple displays, e.g. starting
org-mode on terminal frame with black background makes org-hide use
black foreground.  But that’s visible on X11 frame which uses white
background.

This is why I’ve not gone with Org’s way of initialising the hide face
when the mode is first enabled.

>> @@ -2911,6 +2925,13 @@ merge_face_ref (struct window *w,
>>  		  else
>>  		    err = true;
>>  		}
>> +	      else if (EQ (keyword, QCinvisible))
>> +		{
>> +		  if (EQ (value, Qt) || NILP (value))
>> +		    to[LFACE_INVISIBLE_INDEX] = value;
>> +		  else
>> +		    err = true;
>> +		}

> Does this mean that the result of merging two faces with different
> values for :invisible will depend on the order of the merge?  That is,
> does the nil value override the t value?  Or am I missing something?

The behaviour is the same as for other boolean attributes, e.g. extend
or inverse-video, so yes, if the attribute is specified multiple times
one overrides the other.

There’s a related difference in how org-hide works now vs with this
patch.  Currently, selecting text which uses org-hide makes the text
visible; with this patch it remains invisible unless region face is
changed to include :invisible nil.

> Btw, I think "invisible" is not the best name for this, because that's
> not really what will happen on display.  It's more like "illegible" or
> something to that effect.

Not sure I’m a fan of illegible but I’m happy to go with whatever name.

By the way, two other ideas I had were:
a) add another value for :inverse-video (e.g. ‘:inverse-video
   'foreground’ makes foreground same as background) or
b) add a pseudo colour so one could write ‘:foreground "background"’.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»