bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Tina Russell]

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

:distant-foreground is a very useful concept for a face property: text
will be rendered with the :foreground color, unless it’s too close to
the current background color, in which case :distant-foreground kicks
in. Like, try this in Eshell or IELM:

(propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
"yellow" :background "yellow"))

You’ll get a solid band of yellow, of course. But, with
:distant-foreground…

(propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
"yellow" :background "yellow" :distant-foreground "black"))

Now it is a friendly greeting. (Naturally, you wouldn’t normally set
:background and :distant-foreground in the same face, this is just an
example.)

But, try this:

(propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
"yellow" :background "white" :distant-foreground "black"))

:distant-foreground doesn’t kick in—and you’re left with yellow-on-white
text that’s impossible to read, the exact scenario that
:distant-foreground was quite rightly designed to avoid.

I’m not the only one who’s noticed this; there’s a good StackExchange
thread from 2015 here:
https://emacs.stackexchange.com/questions/7982/ The author notes that
there should be a user option to set the amount of “distance” (between
foreground and background colors) that is required for
distant-foreground to kick in, and adds that a good way to measure
color distance in real-world circumstances is described here:
http://web.mst.edu/~rhall/web_design/color_readability.html A commenter
suggested that he file a bug, but I can’t find an Emacs bug report
related to this issue, so I’m filing this now.

I really hope this gets addressed, because :distant-foreground is a
really nice and clever feature. Thanks!

In GNU Emacs 26.1.90 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
 of 2019-01-02 built on pannychis
Repository revision: 08840f2f7bfc6144bd163dd85efe87d28541e425
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Ubuntu 18.04.1 LTS

Configured using:
 'configure --with-xaw3d --with-modules --with-xwidgets'

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

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Federico Tedin]

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

Tina Russell <tinakellyrussell@gmail.com> writes:

> :distant-foreground is a very useful concept for a face property: text
> will be rendered with the :foreground color, unless it’s too close to
> the current background color, in which case :distant-foreground kicks
> in. Like, try this in Eshell or IELM:
>
> (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> "yellow" :background "yellow"))
>
> You’ll get a solid band of yellow, of course. But, with
> :distant-foreground…
>
> (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> "yellow" :background "yellow" :distant-foreground "black"))
>
> Now it is a friendly greeting. (Naturally, you wouldn’t normally set
> :background and :distant-foreground in the same face, this is just an
> example.)
>
> But, try this:
>
> (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> "yellow" :background "white" :distant-foreground "black"))
>
> :distant-foreground doesn’t kick in—and you’re left with yellow-on-white
> text that’s impossible to read, the exact scenario that
> :distant-foreground was quite rightly designed to avoid.
>
> I’m not the only one who’s noticed this; there’s a good StackExchange
> thread from 2015 here:
> https://emacs.stackexchange.com/questions/7982/ The author notes that
> there should be a user option to set the amount of “distance” (between
> foreground and background colors) that is required for
> distant-foreground to kick in, and adds that a good way to measure
> color distance in real-world circumstances is described here:
> http://web.mst.edu/~rhall/web_design/color_readability.html A commenter
> suggested that he file a bug, but I can’t find an Emacs bug report
> related to this issue, so I’m filing this now.
>
> I really hope this gets addressed, because :distant-foreground is a
> really nice and clever feature. Thanks!
>
> In GNU Emacs 26.1.90 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
>  of 2019-01-02 built on pannychis
> Repository revision: 08840f2f7bfc6144bd163dd85efe87d28541e425
> Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
> System Description: Ubuntu 18.04.1 LTS
>
> Configured using:
>  'configure --with-xaw3d --with-modules --with-xwidgets'

I've created a patch that introduces a new variable
`face-near-same-color-threshold', with a default value of 30000 (as was
defined in NEAR_SAME_COLOR_THRESHOLD in xfaces.c). Changing this
variable's value will make certain color combinations be considered as
'same', which will lead to :distant-foreground being used. For example:

M-: (setq face-near-same-color-threshold 200000)
M-: (clear-font-cache)

Then, using your third example, the text will be displayed with a black
foregound.

Is this a reasonable fix, or should we consider implementing another way
of measuring distance between colors like Tina mentioned?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1636 bytes --]

diff --git a/src/xfaces.c b/src/xfaces.c
index cffa89e1f3..eeea165187 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -1157,8 +1157,6 @@ load_color (struct frame *f, struct face *face, Lisp_Object name,
 
 #ifdef HAVE_WINDOW_SYSTEM
 
-#define NEAR_SAME_COLOR_THRESHOLD 30000
-
 /* Load colors for face FACE which is used on frame F.  Colors are
    specified by slots LFACE_BACKGROUND_INDEX and LFACE_FOREGROUND_INDEX
    of ATTRS.  If the background color specified is not supported on F,
@@ -1198,8 +1196,9 @@ load_face_colors (struct frame *f, struct face *face,
   face->foreground = load_color2 (f, face, fg, LFACE_FOREGROUND_INDEX, &xfg);
 
   dfg = attrs[LFACE_DISTANT_FOREGROUND_INDEX];
+  int distance = color_distance (&xbg, &xfg);
   if (!NILP (dfg) && !UNSPECIFIEDP (dfg)
-      && color_distance (&xbg, &xfg) < NEAR_SAME_COLOR_THRESHOLD)
+      && distance < face_near_same_color_threshold)
     {
       if (EQ (attrs[LFACE_INVERSE_INDEX], Qt))
         face->background = load_color (f, face, dfg, LFACE_BACKGROUND_INDEX);
@@ -6768,6 +6767,12 @@ RESCALE-RATIO is a floating point number to specify how much larger
 a font of 10 point, we actually use a font of 10 * RESCALE-RATIO point.  */);
   Vface_font_rescale_alist = Qnil;
 
+  DEFVAR_INT ("face-near-same-color-threshold", face_near_same_color_threshold,
+	      doc: /* Number representing the minimum tolerated distance
+between face background and foreground colors before distant-foreground is
+used instead (when present). */);
+  face_near_same_color_threshold = 30000;
+
 #ifdef HAVE_WINDOW_SYSTEM
   defsubr (&Sbitmap_spec_p);
   defsubr (&Sx_list_fonts);

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Tina Russell]

Well, even if we change the backend for how color distance is
measured, we’ll probably still need a user option for “how much color
distance is required for two colors to be considered appropriately
distant,” so it seems like a good place to start in any event.

Thanks!

On Tue, Jan 15, 2019 at 5:08 PM Federico Tedin <federicotedin@gmail.com> wrote:
>
> Tina Russell <tinakellyrussell@gmail.com> writes:
>
> > :distant-foreground is a very useful concept for a face property: text
> > will be rendered with the :foreground color, unless it’s too close to
> > the current background color, in which case :distant-foreground kicks
> > in. Like, try this in Eshell or IELM:
> >
> > (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> > "yellow" :background "yellow"))
> >
> > You’ll get a solid band of yellow, of course. But, with
> > :distant-foreground…
> >
> > (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> > "yellow" :background "yellow" :distant-foreground "black"))
> >
> > Now it is a friendly greeting. (Naturally, you wouldn’t normally set
> > :background and :distant-foreground in the same face, this is just an
> > example.)
> >
> > But, try this:
> >
> > (propertize "Greetings, esteemed Emacs developers!" 'face '(:foreground
> > "yellow" :background "white" :distant-foreground "black"))
> >
> > :distant-foreground doesn’t kick in—and you’re left with yellow-on-white
> > text that’s impossible to read, the exact scenario that
> > :distant-foreground was quite rightly designed to avoid.
> >
> > I’m not the only one who’s noticed this; there’s a good StackExchange
> > thread from 2015 here:
> > https://emacs.stackexchange.com/questions/7982/ The author notes that
> > there should be a user option to set the amount of “distance” (between
> > foreground and background colors) that is required for
> > distant-foreground to kick in, and adds that a good way to measure
> > color distance in real-world circumstances is described here:
> > http://web.mst.edu/~rhall/web_design/color_readability.html A commenter
> > suggested that he file a bug, but I can’t find an Emacs bug report
> > related to this issue, so I’m filing this now.
> >
> > I really hope this gets addressed, because :distant-foreground is a
> > really nice and clever feature. Thanks!
> >
> > In GNU Emacs 26.1.90 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
> >  of 2019-01-02 built on pannychis
> > Repository revision: 08840f2f7bfc6144bd163dd85efe87d28541e425
> > Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
> > System Description: Ubuntu 18.04.1 LTS
> >
> > Configured using:
> >  'configure --with-xaw3d --with-modules --with-xwidgets'
>
> I've created a patch that introduces a new variable
> `face-near-same-color-threshold', with a default value of 30000 (as was
> defined in NEAR_SAME_COLOR_THRESHOLD in xfaces.c). Changing this
> variable's value will make certain color combinations be considered as
> 'same', which will lead to :distant-foreground being used. For example:
>
> M-: (setq face-near-same-color-threshold 200000)
> M-: (clear-font-cache)
>
> Then, using your third example, the text will be displayed with a black
> foregound.
>
> Is this a reasonable fix, or should we consider implementing another way
> of measuring distance between colors like Tina mentioned?
>

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Federico Tedin]

> Well, even if we change the backend for how color distance is
> measured, we’ll probably still need a user option for “how much color
> distance is required for two colors to be considered appropriately
> distant,” so it seems like a good place to start in any event.

I agree! Either option should be configurable. We should wait and see if
a maintainer has any thoughts on this as well.

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Basil L. Contovounesios]

Tina Russell <tinakellyrussell@gmail.com> writes:

> I’m not the only one who’s noticed this; there’s a good StackExchange
> thread from 2015 here:
> https://emacs.stackexchange.com/questions/7982/ The author notes that
> there should be a user option to set the amount of “distance” (between
> foreground and background colors) that is required for
> distant-foreground to kick in

It would be nice if any work in this area took into consideration
existing efforts by Julien Danjou (CCed) in lisp/net/shr-color.el
(see e.g. user options shr-color-visible-luminance-min and
shr-color-visible-distance-min and function shr-color-visible).

For example, the library may end up either duplicating existing core
logic, or prove sufficiently useful that it is moved to a less
SHR-specific location (I'm just speculating).

> , and adds that a good way to measure
> color distance in real-world circumstances is described here:
> http://web.mst.edu/~rhall/web_design/color_readability.html A commenter
> suggested that he file a bug, but I can’t find an Emacs bug report
> related to this issue, so I’m filing this now.

FWIW, there has been some discussion of colour distances in bug#25525
and bug#30295:

  https://debbugs.gnu.org/25525
  https://debbugs.gnu.org/30295

-- 
Basil

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Daniel Colascione]

On 1/21/19 5:58 PM, Basil L. Contovounesios wrote:
> Tina Russell <tinakellyrussell@gmail.com> writes:
> 
>> I’m not the only one who’s noticed this; there’s a good StackExchange
>> thread from 2015 here:
>> https://emacs.stackexchange.com/questions/7982/ The author notes that
>> there should be a user option to set the amount of “distance” (between
>> foreground and background colors) that is required for
>> distant-foreground to kick in
> 
> It would be nice if any work in this area took into consideration
> existing efforts by Julien Danjou (CCed) in lisp/net/shr-color.el
> (see e.g. user options shr-color-visible-luminance-min and
> shr-color-visible-distance-min and function shr-color-visible).
> 
> For example, the library may end up either duplicating existing core
> logic, or prove sufficiently useful that it is moved to a less
> SHR-specific location (I'm just speculating).
> 
>> , and adds that a good way to measure
>> color distance in real-world circumstances is described here:
>> http://web.mst.edu/~rhall/web_design/color_readability.html A commenter
>> suggested that he file a bug, but I can’t find an Emacs bug report
>> related to this issue, so I’m filing this now.
> 
> FWIW, there has been some discussion of colour distances in bug#25525
> and bug#30295:
> 
>    https://debbugs.gnu.org/25525
>    https://debbugs.gnu.org/30295
> 

I think I had a proposal a long time ago to just call into lisp during 
redisplay in order to compute the color composition.

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Tue, 15 Jan 2019 22:08:15 -0300
> 
> I've created a patch that introduces a new variable
> `face-near-same-color-threshold', with a default value of 30000 (as was
> defined in NEAR_SAME_COLOR_THRESHOLD in xfaces.c). Changing this
> variable's value will make certain color combinations be considered as
> 'same', which will lead to :distant-foreground being used. For example:

Sorry for the long delay, I've now pushed your change to the master
branch.

Please in the future provide a ChangeLog-style commit log message for
your changes, and also accompany them with the pertinent updates for
the documentation.  I did that for this change.

Thanks again for working on this.

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Federico Tedin]

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

Thanks Eli!

I have a question regarding commit messages and documentation: should I
include them in the first iteration of the patch I created, or should I
wait until there’s been some conversation first?  Until now I’ve chosen the
first option since I’m not always sure the patch I created is
correct/applicable/etc.

On Fri, 1 Mar 2019 at 05:40 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Federico Tedin <federicotedin@gmail.com>
> > Date: Tue, 15 Jan 2019 22:08:15 -0300
> >
> > I've created a patch that introduces a new variable
> > `face-near-same-color-threshold', with a default value of 30000 (as was
> > defined in NEAR_SAME_COLOR_THRESHOLD in xfaces.c). Changing this
> > variable's value will make certain color combinations be considered as
> > 'same', which will lead to :distant-foreground being used. For example:
>
> Sorry for the long delay, I've now pushed your change to the master
> branch.
>
> Please in the future provide a ChangeLog-style commit log message for
> your changes, and also accompany them with the pertinent updates for
> the documentation.  I did that for this change.
>
> Thanks again for working on this.
>

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

bug#34001: 26.1.90; :distant-foreground face property fails to work in most cases

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Fri, 1 Mar 2019 09:44:30 -0300
> Cc: 34001-done@debbugs.gnu.org, tinakellyrussell@gmail.com
> 
> I have a question regarding commit messages and documentation: should I include them in the first iteration
> of the patch I created, or should I wait until there’s been some conversation first?  Until now I’ve chosen the
> first option since I’m not always sure the patch I created is correct/applicable/etc.

Please include them with all the iterations, so that whoever ends up
pushing the changes will only need to look at a single email message.