bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

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

Hello!

I have a minor gripe with the way eww/shr render <code> elements within
other inline elements.  My main frustration stems from code inside
links[1], but this extends to other elements, e.g. <strong> and <em>:


[-- Attachment #2: foo.html --]
[-- Type: text/html, Size: 259 bytes --]

[-- Attachment #3: Type: text/plain, Size: 38 bytes --]


Here is how Emacs 26.3 rendered it:


[-- Attachment #4: emacs-26.png --]
[-- Type: image/png, Size: 45577 bytes --]

[-- Attachment #5: Type: text/plain, Size: 41 bytes --]


Here is how Emacs 27 and 28 render it:


[-- Attachment #6: emacs-27.png --]
[-- Type: image/png, Size: 58875 bytes --]

[-- Attachment #7: Type: text/plain, Size: 390 bytes --]


Both Emacs variants fail to do what I'd expect:

- Emacs 26 uses variable-pitch for <code> elements,
- Emacs 27+ uses the default face, which does ensure a monospace font,
  but also neutralizes any effects of enclosing elements, e.g.
    - no bold inside <strong>,
    - no italics inside <em>,
    - no blue color nor underline inside <a>.

For example, here is how Firefox 72 does it:


[-- Attachment #8: firefox-72.png --]
[-- Type: image/png, Size: 47052 bytes --]

[-- Attachment #9: Type: text/plain, Size: 125 bytes --]


Here is a first, simple patch that uses fixed-pitch instead of default
to ensure other face attributes are not overridden:


[-- Attachment #10: patch1.patch --]
[-- Type: text/x-diff, Size: 1135 bytes --]

From 36629660e9adf4cd500b081e8b45c76cb1d8cb4d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sat, 8 Feb 2020 00:08:13 +0100
Subject: [PATCH] Ensure faces of enclosing elements apply to <code> elements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The default face overrides the attributes of every enclosing element.
E.g. with <a href="…">foo <code>bar</code> baz</a>, since shr applies
'default' top of 'shr-link', "bar" has neither a blue foreground nor
an underline.

* lisp/net/shr.el (shr-tag-code): Use a face that only sets relevant
attributes.
---
 lisp/net/shr.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 241180d471..87106b0bd6 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1438,7 +1438,7 @@ shr-tag-u
   (shr-fontize-dom dom 'underline))
 
 (defun shr-tag-code (dom)
-  (let ((shr-current-font 'default))
+  (let ((shr-current-font 'fixed-pitch))
     (shr-generic dom)))
 
 (defun shr-tag-tt (dom)
-- 
2.20.1


[-- Attachment #11: Type: text/plain, Size: 152 bytes --]


Here is an alternative patch which allows customizing the face used for
<code>, in case the user wants e.g. fixed-pitch-serif instead of
fixed-pitch:


[-- Attachment #12: patch2.patch --]
[-- Type: text/x-diff, Size: 1368 bytes --]

From b648822ac4e401984f967fdc2d78e177697089ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sat, 8 Feb 2020 00:08:13 +0100
Subject: [PATCH] Introduce face for <code> elements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The default face overrides the attributes of every enclosing element.
E.g. with <a href="…">foo <code>bar</code> baz</a>, since shr applies
'default' top of 'shr-link', "bar" has neither a blue foreground nor
an underline.

* lisp/net/shr.el (shr-code): New face.
(shr-tag-code): Use it.
---
 lisp/net/shr.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 241180d471..1215fbddd0 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -142,6 +142,10 @@ shr-strike-through
   "Face for <s> elements."
   :version "24.1")
 
+(defface shr-code '((t :inherit fixed-pitch))
+  "Face for <code> elements."
+  :version "27.1")
+
 (defface shr-link
   '((t :inherit link))
   "Face for link elements."
@@ -1438,7 +1442,7 @@ shr-tag-u
   (shr-fontize-dom dom 'underline))
 
 (defun shr-tag-code (dom)
-  (let ((shr-current-font 'default))
+  (let ((shr-current-font 'shr-code))
     (shr-generic dom)))
 
 (defun shr-tag-tt (dom)
-- 
2.20.1


[-- Attachment #13: Type: text/plain, Size: 38 bytes --]


Both patches give the same results:


[-- Attachment #14: patch.png --]
[-- Type: image/png, Size: 58955 bytes --]

[-- Attachment #15: Type: text/plain, Size: 1017 bytes --]


If the second patch seems like a worthwhile addition, I can augment it
with some documentation.


Thank you for your time.


[1] Cf. for example This Week In Rust:
    https://this-week-in-rust.org/blog/2020/02/04/this-week-in-rust-324/#updates-from-rust-core


In GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-01-31 built on hirondell
Repository revision: d3ead375092e2690c1d1d6a5dd82e6e89cdf4f4c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Configured using:
 'configure --with-xwidgets --with-cairo'

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

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

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

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

Patches with changelogs amended with this bug number:


[-- Attachment #2: patch1.patch --]
[-- Type: text/x-diff, Size: 1113 bytes --]

From 36629660e9adf4cd500b081e8b45c76cb1d8cb4d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sat, 8 Feb 2020 00:08:13 +0100
Subject: [PATCH] Ensure faces of enclosing elements apply to <code> elements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The default face overrides the attributes of every enclosing element.
E.g. with <a href="…">foo <code>bar</code> baz</a>, since shr applies
'default' top of 'shr-link', "bar" has neither a blue foreground nor
an underline.

* lisp/net/shr.el (shr-tag-code): Use a face that only sets relevant
attributes.

(Bug#39504)
---
 lisp/net/shr.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 241180d471..87106b0bd6 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1438,7 +1438,7 @@ shr-tag-u
   (shr-fontize-dom dom 'underline))
 
 (defun shr-tag-code (dom)
-  (let ((shr-current-font 'default))
+  (let ((shr-current-font 'fixed-pitch))
     (shr-generic dom)))
 
 (defun shr-tag-tt (dom)
-- 
2.20.1


[-- Attachment #3: patch2.patch --]
[-- Type: text/x-diff, Size: 1335 bytes --]

From b648822ac4e401984f967fdc2d78e177697089ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sat, 8 Feb 2020 00:08:13 +0100
Subject: [PATCH] Introduce face for <code> elements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The default face overrides the attributes of every enclosing element.
E.g. with <a href="…">foo <code>bar</code> baz</a>, since shr applies
'default' top of 'shr-link', "bar" has neither a blue foreground nor
an underline.

* lisp/net/shr.el (shr-code): New face.
(shr-tag-code): Use it.

(Bug#39504)
---
 lisp/net/shr.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 241180d471..1215fbddd0 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -142,6 +142,10 @@ shr-strike-through
   "Face for <s> elements."
   :version "24.1")
 
+(defface shr-code '((t :inherit fixed-pitch))
+  "Face for <code> elements."
+  :version "27.1")
+
 (defface shr-link
   '((t :inherit link))
   "Face for link elements."
@@ -1438,7 +1442,7 @@ shr-tag-u
   (shr-fontize-dom dom 'underline))
 
 (defun shr-tag-code (dom)
-  (let ((shr-current-font 'default))
+  (let ((shr-current-font 'shr-code))
     (shr-generic dom)))
 
 (defun shr-tag-tt (dom)
-- 
2.20.1

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Here is a first, simple patch that uses fixed-pitch instead of default
> to ensure other face attributes are not overridden:

Thanks; applied to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Thanks; applied to Emacs 28.

Thanks Lars!

Just to be sure, are we fine with how things look on emacs-27?  I'm not
saying the situation there is unacceptable or anything; I'm just
wondering if we're comfortable with the behaviour change between 26 and
27 (cf. screenshots in the opening message).  It's a mostly cosmetic
issue anyway.


Speaking of cosmetic issues, how did you apply my patch?  AFAICT you
used

- my first patch's diff,
- my second patch's title,
- your own changelog entry.

Was there something unsatisfactory with my changelog entry?  I don't
care much either way, but I'm trying to tick as many boxes as I can to
take some load off the maintainers's shoulders; if you'd rather I just
dump a plain old diff, I might as well not bother with the changelog.

(Also, the second patch's title, "Introduce face for <code> elements",
refers to a new face introduced by the patch; the *first* patch does
*not* introduce a new face, which is why I gave it a different title.
Again, no biggy, it simply makes me question whether I should bother
with changelog entries.)


I actually thought committing a contributor's patch would be as simple
as running "git am" on the attached file, but I now notice that when I
download my patches using either Gnus's gnus-mime-save-part or
<https://lists.gnu.org/archive/html/bug-gnu-emacs>, they contain a stray
'>' character on the first line.

(I have no idea what causes this; C-u g on the article also shows this
stray '>' char, which makes me think that it might have been added while
sending?  It definitely wasn't there when I created the patches with
"git format-patch".)

The point being that "git am" chokes on this extra '>', unless given
"--patch-format=mbox".

(Downloading the attachments from <https://debbugs.gnu.org/39504> works
fine FWIW.)


I know you are all pretty busy, I apologize if this falls into
nitpicking territory.  I'm just slightly embarrassed about the final
commit title, and confused about whether the changelog entries I wrote
were correct.

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Just to be sure, are we fine with how things look on emacs-27?  I'm not
> saying the situation there is unacceptable or anything; I'm just
> wondering if we're comfortable with the behaviour change between 26 and
> 27 (cf. screenshots in the opening message).  It's a mostly cosmetic
> issue anyway.

I don't think it's worth backporting the fix to the release branch.

> Speaking of cosmetic issues, how did you apply my patch?

I did it by hand since there were two patches in the email and the `M-m'
command in debbugs-gnu (which does all this applying and marking with
bug numbers automatically) doesn't like that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Speaking of cosmetic issues, how did you apply my patch?
>
> I did it by hand since there were two patches in the email and the `M-m'
> command in debbugs-gnu (which does all this applying and marking with
> bug numbers automatically) doesn't like that.

Thanks!  I didn't know about debbugs-gnu's M-m.

FWIW, "| git am RET" with point on the attachment actually *does* TRT[1]
(so long as you're happy with the contributor's message).

(Sorry for my earlier ramblings; while "git am $file" chokes on the
leading '>', "git am < $file" digests it just fine.)


I've cobbled up a function to apply "git am" on the attachment at point
and let the committer optionally amend the message.  Does that look
useful?

(defun debbugs-gnu-am ()
  (gnus-mime-pipe-part "git am")
  (vc-checkin nil 'Git)
  (vc-git-log-edit-toggle-amend)
  ;; log-edit-done eventually errors out if vc-parent-buffer is not a
  ;; file-visiting buffer.
  (setq-local vc-parent-buffer (find-file-noselect "CONTRIBUTE")))

Granted,

- the vc-parent-buffer hack is ugly,

- this does not fill the *log-edit-files* buffer,

- this does none of the careful window placement debbugs-gnu-apply-patch
  takes care of.


[1] If default-directory is set to the root of the Emacs repository, but
    a cursory glance at debbugs-gnu-apply-patch gives me the feeling
    that it's also true there?

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Robert Pluim]

>>>>> On Fri, 21 Feb 2020 19:46:11 +0100, Kévin Le Gouguec <kevin.legouguec@gmail.com> said:

    Kévin> Lars Ingebrigtsen <larsi@gnus.org> writes:
    >>> Speaking of cosmetic issues, how did you apply my patch?
    >> 
    >> I did it by hand since there were two patches in the email and the `M-m'
    >> command in debbugs-gnu (which does all this applying and marking with
    >> bug numbers automatically) doesn't like that.

    Kévin> Thanks!  I didn't know about debbugs-gnu's M-m.

    Kévin> FWIW, "| git am RET" with point on the attachment actually *does* TRT[1]
    Kévin> (so long as you're happy with the contributor's message).

Unlike M-m :-)

    Kévin> [1] If default-directory is set to the root of the Emacs repository, but
    Kévin>     a cursory glance at debbugs-gnu-apply-patch gives me the feeling
    Kévin>     that it's also true there?

I tend to do "| cd ~/repos/emacs && git am"

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I've cobbled up a function to apply "git am" on the attachment at point
> and let the committer optionally amend the message.  Does that look
> useful?
>
> (defun debbugs-gnu-am ()
>   (gnus-mime-pipe-part "git am")
>   (vc-checkin nil 'Git)
>   (vc-git-log-edit-toggle-amend)
>   ;; log-edit-done eventually errors out if vc-parent-buffer is not a
>   ;; file-visiting buffer.
>   (setq-local vc-parent-buffer (find-file-noselect "CONTRIBUTE")))

Sure, looks nice, but I find that `M-m' works fine already.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> (defun debbugs-gnu-am ()
>>   (gnus-mime-pipe-part "git am")
>>   (vc-checkin nil 'Git)
>>   (vc-git-log-edit-toggle-amend)
>>   ;; log-edit-done eventually errors out if vc-parent-buffer is not a
>>   ;; file-visiting buffer.
>>   (setq-local vc-parent-buffer (find-file-noselect "CONTRIBUTE")))
>
> Sure, looks nice, but I find that `M-m' works fine already.

Fair enough[1].  Would it be more worthwhile to focus on extending M-m
to prompt when there are multiple attachments, then?


[1] Although to my untrained eyes debbugs-gnu-apply-patch (and
    debbugs-gnu-insert-changelog) seem quite complex for the specific
    case of a patch generated by git-format-patch(1).

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Fair enough[1].  Would it be more worthwhile to focus on extending M-m
> to prompt when there are multiple attachments, then?

Not for me, but if others would find it useful -- sure.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code> elements

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> Fair enough[1].  Would it be more worthwhile to focus on extending M-m
>> to prompt when there are multiple attachments, then?
>
> Not for me

OK, I guess I'll put the matter to rest then :)

My goal with this subthread was to find a technical solution to
accidental patch mangling by a maintainer; if you think there is no
issue to solve, then there is not much point working on that.

I know that this kind of blunder is pretty much a rounding error in your
overall workflow and I agree that in the grand scheme of things it's not
terribly important.

From the point of view of a drive-by contributor though (at least this
drive-by contributor), it's a bit disheartening to spend so much time
dotting the i's[1], only for the wrong commit title to show up in the
repository log, and for the commit message to be wholly rewritten.

And since this appears to stem from a technical limitation of our tools
(i.e. M-m failing to handle more than one patch per message) it is
somewhat frustrating that suggested technical solutions[2] are being
brushed aside.


I hope I'm not raising a storm in a teacup over this; as I said, it's
not terribly important on a macro scale.  Sorry for all the noise, and
thank you again for your time!


[1] Filling in changelog entries, possibly generating action stamps,
    adding bug numbers…

[2] Aimed entirely at improving the current tooling, instead of flat out
    ranting about how "this would never have happened with a GitSquare™
    Joint Request".