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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39504 in the body.
You can then email your comments to 39504 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing elements apply
 to <code> elements
Date: Sat, 08 Feb 2020 01:06:11 +0100

[Message part 1 (text/plain, inline)]

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>:

[foo.html (text/html, inline)]

[Message part 3 (text/plain, inline)]

Here is how Emacs 26.3 rendered it:

[emacs-26.png (image/png, attachment)]

[Message part 5 (text/plain, inline)]

Here is how Emacs 27 and 28 render it:

[emacs-27.png (image/png, attachment)]

[Message part 7 (text/plain, inline)]

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:

[firefox-72.png (image/png, attachment)]

[Message part 9 (text/plain, inline)]

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

[patch1.patch (text/x-diff, attachment)]

[Message part 11 (text/plain, inline)]

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:

[patch2.patch (text/x-diff, attachment)]

[Message part 13 (text/plain, inline)]

Both patches give the same results:

[patch.png (image/png, attachment)]

[Message part 15 (text/plain, inline)]

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

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#39504: 27.0.60;
 [PATCH] eww/shr: Ensure faces of enclosing elements apply to <code>
 elements
Date: Sat, 08 Feb 2020 01:17:11 +0100

[Message part 1 (text/plain, inline)]

Patches with changelogs amended with this bug number:

[patch1.patch (text/x-diff, attachment)]

[patch2.patch (text/x-diff, attachment)]

Message #11 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Thu, 20 Feb 2020 14:31:33 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

Message #18 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Thu, 20 Feb 2020 23:19:32 +0100

Lars Ingebrigtsen <larsi <at> 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.

Message #21 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Fri, 21 Feb 2020 13:46:28 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

Message #24 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Fri, 21 Feb 2020 19:46:11 +0100

Lars Ingebrigtsen <larsi <at> 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?

Message #27 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Wed, 26 Feb 2020 15:06:55 +0100

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

    Kévin> Lars Ingebrigtsen <larsi <at> 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"

Message #30 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Sat, 14 Mar 2020 13:34:46 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

Message #33 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Sat, 14 Mar 2020 15:15:18 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec <at> 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).

Message #36 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Thu, 02 Apr 2020 13:00:41 +0200

Kévin Le Gouguec <kevin.legouguec <at> 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

Message #39 received at 39504 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39504 <at> debbugs.gnu.org
Subject: Re: bug#39504: 27.0.60; [PATCH] eww/shr: Ensure faces of enclosing
 elements apply to <code> elements
Date: Fri, 03 Apr 2020 10:25:16 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec <at> 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".

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.