[BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)]

[BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)]

[StrawberryTea]

Hello. Currently, if you have a heading background, and you have a date,
radio link, or special keyword, the background is overridden. I have
attached a patch that fixes this issue by using the `prepend' property
instead of the `t' property. Here is also a minimal example to reproduce
the issue:

(defvar bootstrap-version) ; bootstrapping straight.el
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
                                    'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))
(straight-use-package 'org)
(require 'org)
(load-theme 'leuven t)
(dolist (face '(org-level-1 org-level-2 org-level-3 org-level-4 org-level-5
                org-level-6 org-level-7 org-level-8))
  (set-face-attribute face nil :extend t))
(setq org-fontify-whole-heading-line t)
(scratch-buffer)
(org-mode)
(insert "<<<radio>>>\n* [2020-09-11 Fri] Headline 1\n
* COMMENT Headline 2")

If you would like to demo how the patch looks in emacs -Q, you can add
this defun to the above code:

(defun org-set-font-lock-defaults ()
  "Set font lock defaults for the current buffer."
  (let ((org-font-lock-extra-keywords
     (list
      ;; Call the hook
      '(org-font-lock-hook)
      ;; Headlines
      `(,(if org-fontify-whole-heading-line
         "^\\(\\**\\)\\(\\* \\)\\(.*\n?\\)"
           "^\\(\\**\\)\\(\\* \\)\\(.*\\)")
        (1 (org-get-level-face 1))
        (2 (org-get-level-face 2))
        (3 (org-get-level-face 3)))
      ;; Table lines
      '("^[ \t]*\\(\\(|\\|\\+-[-+]\\).*\\S-\\)\n?"
            (0 'org-table-row t)
        (1 'org-table t))
      ;; Table internals
      '("^[ \t]*|\\(?:.*?|\\)? *\\(:?=[^|\n]*\\)" (1 'org-formula t))
      '("^[ \t]*| *\\([#*]\\) *|" (1 'org-formula t))
      '("^[ \t]*|\\( *\\([$!_^/]\\) *|.*\\)|" (1 'org-formula t))
      '("| *\\(<[lrc]?[0-9]*>\\)" (1 'org-formula t))
      ;; Properties
      (list org-property-re
        '(1 'org-special-keyword t)
        '(3 'org-property-value t))
      ;; Drawers
      '(org-fontify-drawers)
      ;; Link related fontification.
      '(org-activate-links)
      (when (memq 'tag org-highlight-links) '(org-activate-tags (1 'org-tag prepend)))
      (when (memq 'radio org-highlight-links) '(org-activate-target-links (1 'org-link prepend)))
      (when (memq 'date org-highlight-links) '(org-activate-dates (0 'org-date prepend)))
      (when (memq 'footnote org-highlight-links) '(org-activate-footnote-links))
          ;; Targets.
          (list org-radio-target-regexp '(0 'org-target t))
      (list org-target-regexp '(0 'org-target t))
      ;; Diary sexps.
      '("^&?%%(.*\\|<%%([^>\n]*?>" (0 'org-sexp-date t))
      ;; Macro
      '(org-fontify-macros)
      ;; TODO keyword
      (list (format org-heading-keyword-regexp-format
            org-todo-regexp)
        '(2 (org-get-todo-face 2) prepend))
      ;; TODO
      (when org-fontify-todo-headline
        (list (format org-heading-keyword-regexp-format
              (concat
               "\\(?:"
               (mapconcat 'regexp-quote org-not-done-keywords "\\|")
               "\\)"))
          '(2 'org-headline-todo prepend)))
      ;; DONE
      (when org-fontify-done-headline
        (list (format org-heading-keyword-regexp-format
              (concat
               "\\(?:"
               (mapconcat 'regexp-quote org-done-keywords "\\|")
               "\\)"))
          '(2 'org-headline-done prepend)))
      ;; Priorities
      '(org-font-lock-add-priority-faces)
      ;; Tags
      '(org-font-lock-add-tag-faces)
      ;; Tags groups
      (when (and org-group-tags org-tag-groups-alist)
        (list (concat org-outline-regexp-bol ".+\\(:"
              (regexp-opt (mapcar 'car org-tag-groups-alist))
              ":\\).*$")
          '(1 'org-tag-group prepend)))
      ;; Special keywords
      (list (concat "\\<" org-deadline-string) '(0 'org-special-keyword t))
      (list (concat "\\<" org-scheduled-string) '(0 'org-special-keyword t))
      (list (concat "\\<" org-closed-string) '(0 'org-special-keyword t))
      (list (concat "\\<" org-clock-string) '(0 'org-special-keyword t))
      ;; Emphasis
      (when org-fontify-emphasized-text '(org-do-emphasis-faces))
      ;; Checkboxes
      `(,org-list-full-item-re 3 'org-checkbox prepend lax)
      (when (cdr (assq 'checkbox org-list-automatic-rules))
        '("\\[\\([0-9]*%\\)\\]\\|\\[\\([0-9]*\\)/\\([0-9]*\\)\\]"
          (0 (org-get-checkbox-statistics-face) prepend)))
      ;; Description list items
          '("\\(?:^[ \t]*[-+]\\|^[ \t]+[*]\\)[ \t]+\\(.*?[ \t]+::\\)\\([ \t]+\\|$\\)"
        1 'org-list-dt prepend)
          ;; Inline export snippets
          '("\\(@@\\)\\([a-z-]+:\\).*?\\(@@\\)"
            (1 'font-lock-comment-face t)
            (2 'org-tag t)
            (3 'font-lock-comment-face t))
      ;; ARCHIVEd headings
      (list (concat
         org-outline-regexp-bol
         "\\(.*:" org-archive-tag ":.*\\)")
        '(1 'org-archived prepend))
      ;; Specials
      '(org-do-latex-and-related)
      '(org-fontify-entities)
      '(org-raise-scripts)
      ;; Code
      '(org-activate-code (1 'org-code t))
      ;; COMMENT
      (list (format
         "^\\*+\\(?: +%s\\)?\\(?: +\\[#[A-Z0-9]\\]\\)? +\\(?9:%s\\)\\(?: \\|$\\)"
         org-todo-regexp
         org-comment-string)
        '(9 'org-special-keyword prepend))
      ;; Blocks and meta lines
      '(org-fontify-meta-lines-and-blocks)
          '(org-fontify-inline-src-blocks)
          ;; Citations.  When an activate processor is specified, if
          ;; specified, try loading it beforehand.
          (progn
            (unless (null org-cite-activate-processor)
              (org-cite-try-load-processor org-cite-activate-processor))
            '(org-cite-activate))
          '(org-activate-folds))))
    (setq org-font-lock-extra-keywords (delq nil org-font-lock-extra-keywords))
    (run-hooks 'org-font-lock-set-keywords-hook)
    ;; Now set the full font-lock-keywords
    (setq-local org-font-lock-keywords org-font-lock-extra-keywords)
    (setq-local font-lock-defaults
        '(org-font-lock-keywords t nil nil backward-paragraph))
    (setq-local font-lock-extend-after-change-region-function
        #'org-fontify-extend-region)
    (kill-local-variable 'font-lock-keywords)
    nil))

The patch is as follows:

diff --git a/lisp/org.el b/lisp/org.el
index 33d9050..e9b4eac 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -5923,8 +5923,8 @@ needs to be inserted at a specific position in the font-lock sequence.")
          ;; Link related fontification.
          '(org-activate-links)
          (when (memq 'tag org-highlight-links) '(org-activate-tags (1 'org-tag prepend)))
-         (when (memq 'radio org-highlight-links) '(org-activate-target-links (1 'org-link t)))
-         (when (memq 'date org-highlight-links) '(org-activate-dates (0 'org-date t)))
+         (when (memq 'radio org-highlight-links) '(org-activate-target-links (1 'org-link prepend)))
+         (when (memq 'date org-highlight-links) '(org-activate-dates (0 'org-date prepend)))
          (when (memq 'footnote org-highlight-links) '(org-activate-footnote-links))
           ;; Targets.
           (list org-radio-target-regexp '(0 'org-target t))
@@ -5999,7 +5999,7 @@ needs to be inserted at a specific position in the font-lock sequence.")
                 "^\\*+\\(?: +%s\\)?\\(?: +\\[#[A-Z0-9]\\]\\)? +\\(?9:%s\\)\\(?: \\|$\\)"
                 org-todo-regexp
                 org-comment-string)
-               '(9 'org-special-keyword t))
+               '(9 'org-special-keyword prepend))
          ;; Blocks and meta lines
          '(org-fontify-meta-lines-and-blocks)
           '(org-fontify-inline-src-blocks)

Emacs  : GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.18.0)
 of 2024-03-03
Package: Org mode version 9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)

Sincerely,
StrawberryTea

[DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Ihor Radchenko]

StrawberryTea <look@strawberrytea.xyz> writes:

> Hello. Currently, if you have a heading background, and you have a date,
> radio link, or special keyword, the background is overridden. I have
> attached a patch that fixes this issue by using the `prepend' property
> instead of the `t' property. Here is also a minimal example to reproduce
> the issue:

This is not an exhaustive list. We have a number of markup objects that
are fontified overriding the containing element/paragraph/heading face:

- target links
- timestamps
- footnotes
- radio targets
- macros
- inline export snippets

I am not 100% sure we should unconditionally prepend their faces to
the containing headline/table/etc.

CCing Protesilaos. This discussion might be of interest for him.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Protesilaos Stavrou]

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

Thank you, Ihor!

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Mon,  4 Mar 2024 10:32:38 +0000
>
> StrawberryTea <look@strawberrytea.xyz> writes:
>
>> Hello. Currently, if you have a heading background, and you have a date,
>> radio link, or special keyword, the background is overridden. I have
>> attached a patch that fixes this issue by using the `prepend' property
>> instead of the `t' property. Here is also a minimal example to reproduce
>> the issue:
>
> This is not an exhaustive list. We have a number of markup objects that
> are fontified overriding the containing element/paragraph/heading face:
>
> - target links
> - timestamps
> - footnotes
> - radio targets
> - macros
> - inline export snippets
>
> I am not 100% sure we should unconditionally prepend their faces to
> the containing headline/table/etc.
>
> CCing Protesilaos. This discussion might be of interest for him.

I think it makes sense to not override the background because otherwise
the heading can look visually "broken". See attached screenshot for an
example: it is called "demo-org-heading-with-background.png". In that
screenshot I include an overline for the heading, to show how this is
not limited to the background colour but extends to all face attributes.
To this end, I also attach "demo-org-heading-with-more-height.png" which
shows how the date is not scaling to match the heading's increased text
height.

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com

[-- Attachment #2: demo-org-heading-with-background.png --]
[-- Type: image/png, Size: 8020 bytes --]

[-- Attachment #3: demo-org-heading-with-more-height.png --]
[-- Type: image/png, Size: 7402 bytes --]

Re: [DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Ihor Radchenko]

Protesilaos Stavrou <info@protesilaos.com> writes:

>> This is not an exhaustive list. We have a number of markup objects that
>> are fontified overriding the containing element/paragraph/heading face:
>>
>> - target links
>> - timestamps
>> - footnotes
>> - radio targets
>> - macros
>> - inline export snippets
>>
>> I am not 100% sure we should unconditionally prepend their faces to
>> the containing headline/table/etc.
>>
>> CCing Protesilaos. This discussion might be of interest for him.
>
> I think it makes sense to not override the background because otherwise
> the heading can look visually "broken". See attached screenshot for an
> example: it is called "demo-org-heading-with-background.png". In that
> screenshot I include an overline for the heading, to show how this is
> not limited to the background colour but extends to all face attributes.
> To this end, I also attach "demo-org-heading-with-more-height.png" which
> shows how the date is not scaling to match the heading's increased text
> height.

I agree that override (what we do now) is clearly not good.
The question, however, is whether we should prioritize object face over
the containing element face or vice versa:

Should link properties take precedence over headline or should headline
properties take precedence?

For example, if link explicitly sets face height to be smaller, should
we honor it? Or should we honor the headline face height, if set?
Similar for background. In your example, the white background override
does not really look nice when heading has its own background.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Protesilaos Stavrou]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Wed, 13 Mar 2024 14:27:18 +0000

> [... 16 lines elided]

>>> CCing Protesilaos. This discussion might be of interest for him.
>>
>> I think it makes sense to not override the background because otherwise
>> the heading can look visually "broken". See attached screenshot for an
>> example: it is called "demo-org-heading-with-background.png". In that
>> screenshot I include an overline for the heading, to show how this is
>> not limited to the background colour but extends to all face attributes.
>> To this end, I also attach "demo-org-heading-with-more-height.png" which
>> shows how the date is not scaling to match the heading's increased text
>> height.
>
> I agree that override (what we do now) is clearly not good.
> The question, however, is whether we should prioritize object face over
> the containing element face or vice versa:
>
> Should link properties take precedence over headline or should headline
> properties take precedence?
>
> For example, if link explicitly sets face height to be smaller, should
> we honor it? Or should we honor the headline face height, if set?
> Similar for background. In your example, the white background override
> does not really look nice when heading has its own background.

This can get tricky, I know.

The idea is to make this behave how all faces relate to the 'default'
face with regard to their 'undefined' attributes. Namely, if a face
defines an attribute, then it overrides the 'default' face, otherwise it
falls back to it for the value of the given attribute.

In the case of the headlines then, every other element should attain the
attributes of the heading unless it explicitly overrides them. So a link
with an 'undefined' height will be as high as the heading, but a link
with a height of, say, 1.0 will keep that height even if the heading is
at a 2.0 height (or whatever).

Same idea for all face attributes.

Now the tricky part is to figure out which combinations can work this
way. Headings and source blocks are two obvious ones. Maybe there are
more, but I cannot think of one right now.

-- 
Protesilaos Stavrou
https://protesilaos.com

Re: [DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Ihor Radchenko]

Protesilaos Stavrou <info@protesilaos.com> writes:

> Now the tricky part is to figure out which combinations can work this
> way. Headings and source blocks are two obvious ones. Maybe there are
> more, but I cannot think of one right now.

This is relevant to any greater element that can contain markup inside
and where we apply the fontification to both markup and contents:

- headlines
- tables
- descriptive lists
- commented headlines

For headlines and tables, it /feels/ reasonable to apply the container
face first and then prepend the markup faces.
However, for descriptive lists and commented headlines, it is not so
clear. For commented headlines that should fade out compared to ordinary
headlines, prepending markup faces may lead to commented headlines
standing out.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] Face priority in Org fontification

[StrawberryTea]

StrawberryTea <look@strawberrytea.xyz> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Protesilaos Stavrou <info@protesilaos.com> writes:
>>
>>> Now the tricky part is to figure out which combinations can work this
>>> way. Headings and source blocks are two obvious ones. Maybe there are
>>> more, but I cannot think of one right now.
>>
>> This is relevant to any greater element that can contain markup inside
>> and where we apply the fontification to both markup and contents:
>>
>> - headlines
>> - tables
>> - descriptive lists
>> - commented headlines
>>
>> For headlines and tables, it /feels/ reasonable to apply the container
>> face first and then prepend the markup faces.
>> However, for descriptive lists and commented headlines, it is not so
>> clear. For commented headlines that should fade out compared to ordinary
>> headlines, prepending markup faces may lead to commented headlines
>> standing out.
> I do not know what you mean by commented headlines. Do you mean
> headlines with the COMMENT keyword? I think it's better that the COMMENT
> keyword blend in with the headline so that the headline does not look
> broken. In the descriptive list case, it also makes sense for all the
> themes I've tried because the result of `append` is that the date font
> is bolded, which makes it look more aesthetically pleasing.
>
> Also, here is an updated version of the patch that covers all the
> inline markup you said earlier except for inline source blocks:
>
> diff --git a/lisp/ol.el b/lisp/ol.el
> index fa16b5920..b4253685f 100644
> --- a/lisp/ol.el
> +++ b/lisp/ol.el
> @@ -2223,8 +2223,8 @@ Also refresh fontification if needed."
>     ;; Some languages, e.g., Chinese, do not use spaces to
>     ;; separate words.  Also allow to surround radio targets with
>     ;; line-breakable characters.
> -	(before-re "\\(?:^\\|[^[:alnum:]]\\|\\c|\\)\\(")
> -	(after-re "\\)\\(?:$\\|[^[:alnum:]]\\|\\c|\\)")
> +	(before-re "\\(?:^\\|[^[:alnum:]<]\\|\\c|\\)\\(")
> +	(after-re "\\)\\(?:$\\|[^[:alnum:]>]\\|\\c|\\)")
>     (targets
>      (org-with-wide-buffer
>       (goto-char (point-min))
> diff --git a/lisp/org.el b/lisp/org.el
> index 33d90506b..ac2cd2bc4 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -5631,9 +5631,10 @@ by a #."
>          (match-string 1))
>     (let ((end (match-end 1))
>           (closing-start (match-beginning 1)))
> -	  (add-text-properties
> -	   begin end
> -	   '(font-lock-multiline t font-lock-fontified t face org-macro))
> +          (add-text-properties
> +           begin end
> +           '(font-lock-multiline t font-lock-fontified t))
> +          (add-face-text-property begin end 'org-macro)
>       (org-remove-flyspell-overlays-in begin end)
>       (when org-hide-macro-markers
>         (add-text-properties begin opening-end '(invisible t))
> @@ -5678,14 +5679,14 @@ by a #."
>         (search-forward (or label "fn:"))
>         (org-remove-flyspell-overlays-in beg (match-end 0))))
>     (add-text-properties beg end
> -			     (list 'mouse-face 'highlight
> -				   'keymap org-mouse-map
> -				   'help-echo
> -				   (if referencep "Footnote reference"
> -				     "Footnote definition")
> -				   'font-lock-fontified t
> -				   'font-lock-multiline t
> -				   'face 'org-footnote))))))
> +	                     (list 'mouse-face 'highlight
> +	                           'keymap org-mouse-map
> +	                           'help-echo
> +	                           (if referencep "Footnote reference"
> +		                     "Footnote definition")
> +	                           'font-lock-fontified t
> +	                           'font-lock-multiline t))
> +        (add-face-text-property beg end 'org-footnote)))))
>
>  (defun org-activate-dates (limit)
>    "Add text properties for dates."
> @@ -5923,12 +5924,12 @@ needs to be inserted at a specific position in the font-lock sequence.")
>       ;; Link related fontification.
>       '(org-activate-links)
>       (when (memq 'tag org-highlight-links) '(org-activate-tags (1 'org-tag prepend)))
> -	  (when (memq 'radio org-highlight-links) '(org-activate-target-links (1 'org-link t)))
> -	  (when (memq 'date org-highlight-links) '(org-activate-dates (0 'org-date t)))
> +	  (when (memq 'radio org-highlight-links) '(org-activate-target-links (1 'org-link prepend)))
> +	  (when (memq 'date org-highlight-links) '(org-activate-dates (0 'org-date prepend)))
>       (when (memq 'footnote org-highlight-links) '(org-activate-footnote-links))
>            ;; Targets.
> -          (list org-radio-target-regexp '(0 'org-target t))
> -	  (list org-target-regexp '(0 'org-target t))
> +          (list org-radio-target-regexp '(0 'org-target prepend))
> +	  (list org-target-regexp '(0 'org-target prepend))
>       ;; Diary sexps.
>       '("^&?%%(.*\\|<%%([^>\n]*?>" (0 'org-sexp-date t))
>       ;; Macro
>
> The only nuance I have noticed so far is that to prevent the radio link
> from also inheriting the `org-link' face for itself, I had to modify the
> target link regexp to exclude the `<' and `>' characters.
>
> StrawberryTea

Hi. I'm following up on this thread as a friendly ping since I don't
want this issue to be forgotten. I think the patch is ready for review.

StrawberryTea

Re: [DISCUSSION] Face priority in Org fontification (was: [BUG] org dates, radio links, and special keywords override heading backgrounds [9.7 (9.7-??-902dacb @ /home/st/.config/emacs/.local/straight/build-30.0.50/org/)])

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Protesilaos Stavrou <info@protesilaos.com> writes:
>
>> Now the tricky part is to figure out which combinations can work this
>> way. Headings and source blocks are two obvious ones. Maybe there are
>> more, but I cannot think of one right now.
>
> This is relevant to any greater element that can contain markup inside
> and where we apply the fontification to both markup and contents:
>
> - headlines
> - tables
> - descriptive lists
> - commented headlines
>
> For headlines and tables, it /feels/ reasonable to apply the container
> face first and then prepend the markup faces.
> However, for descriptive lists and commented headlines, it is not so
> clear. For commented headlines that should fade out compared to ordinary
> headlines, prepending markup faces may lead to commented headlines
> standing out.

I now made it so that markup consistently takes precedence over the
container face, except for commented headings.

Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=d3878cb6f

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>