Errors in redisplay in eww

Errors in redisplay in eww

[Eli Zaretskii]

When browsing this page:

  https://news.ycombinator.com/reply?id=10801669&goto=item?id=10801368

I see a lot of error messages in *Messages*:

  Invalid face reference: nil [329 times]
  Invalid face reference: nil [2000 times]

Also, Firefox displays that page with indentation, so that each
response is slightly indented relative to the message to which it
responds, but eww shows everything flat.  Is that a feature?

Thanks.

Re: Errors in redisplay in eww

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> When browsing this page:
>
>   https://news.ycombinator.com/reply?id=10801669&goto=item?id=10801368
>
> I see a lot of error messages in *Messages*:
>
>   Invalid face reference: nil [329 times]
>   Invalid face reference: nil [2000 times]

I don't get any such errors (running under Linux).  Could you see what
face it is that's nil?

> Also, Firefox displays that page with indentation, so that each
> response is slightly indented relative to the message to which it
> responds, but eww shows everything flat.  Is that a feature?

I haven't looked closely at the HTML, but I would guess that the
indentation depends on something CSS-ey.  And shr doesn't do (most of)
CSS.

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

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Tue, 29 Dec 2015 17:16:05 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > When browsing this page:
> >
> >   https://news.ycombinator.com/reply?id=10801669&goto=item?id=10801368
> >
> > I see a lot of error messages in *Messages*:
> >
> >   Invalid face reference: nil [329 times]
> >   Invalid face reference: nil [2000 times]
> 
> I don't get any such errors (running under Linux).  Could you see what
> face it is that's nil?

The face is (nil (:background "#ffffff")), and the problem happens
when we try to merge its car (which is nil, not a valid face).

Anything else I can help you with?  Just ask.

Re: Errors in redisplay in eww

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I don't get any such errors (running under Linux).  Could you see what
>> face it is that's nil?
>
> The face is (nil (:background "#ffffff")), and the problem happens
> when we try to merge its car (which is nil, not a valid face).

Huh.  Where does that face come from?  And why am I not seeing this?
Could it be something Windows-specific?

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

Re: Errors in redisplay in eww

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Lars Ingebrigtsen  Date: Tue, 29 Dec 2015 17:16:05 +0100
>>>>> Eli Zaretskii <eliz@gnu.org> writes:

 >>> When browsing this page:

 >>> https://news.ycombinator.com/reply?id=10801669&goto=item?id=10801368

 >>> I see a lot of error messages in *Messages*:

 >>> Invalid face reference: nil [329 times]
 >>> Invalid face reference: nil [2000 times]

 >> I don't get any such errors (running under Linux).  Could you see
 >> what face it is that's nil?

 > The face is (nil (:background "#ffffff")), and the problem happens
 > when we try to merge its car (which is nil, not a valid face).

	If I’m reading the code correctly, the 'face property value is
	set to either a font or the value returned by the
	shr-face-background function, /or/ the value passed to
	shr-add-font is prepended to the current face list.  Could you
	please try doing, say, trace-function-background on these two
	functions and see if the former ever returns anything suspicious
	(like the list above; and for what inputs), or if the latter is
	ever called with a nil TYPE argument?

-- 
FSF associate member #7257  http://am-1.org/~ivan/      … 3013 B6A0 230E 334A

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Wed, 30 Dec 2015 12:24:34 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I don't get any such errors (running under Linux).  Could you see what
> >> face it is that's nil?
> >
> > The face is (nil (:background "#ffffff")), and the problem happens
> > when we try to merge its car (which is nil, not a valid face).
> 
> Huh.  Where does that face come from?

I guess from the page source:

  <html><head><meta name="viewport" content="width=device-width, initial-scale=1.0"><title></title></head><body bgcolor="#ffffff" alink="#0000be">You have to be logged in to reply.

> And why am I not seeing this?

No clue.  Hope you can take it from this lead.

> Could it be something Windows-specific?

Hope you will be able to tell, or at least ask some questions that
will allow me to find that out.

Thanks.

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Wed, 30 Dec 2015 14:00:43 +0000
> 
>  > The face is (nil (:background "#ffffff")), and the problem happens
>  > when we try to merge its car (which is nil, not a valid face).
> 
> 	If I’m reading the code correctly, the 'face property value is
> 	set to either a font or the value returned by the
> 	shr-face-background function, /or/ the value passed to
> 	shr-add-font is prepended to the current face list.  Could you
> 	please try doing, say, trace-function-background on these two
> 	functions and see if the former ever returns anything suspicious
> 	(like the list above; and for what inputs), or if the latter is
> 	ever called with a nil TYPE argument?

The contents of the trace buffer is below.  Does this reveal anything
interesting?

Thanks.

  ======================================================================
  1 -> (shr-add-font 38 43 bold)
  1 <- shr-add-font: nil
  ======================================================================
  1 -> (shr-face-background variable-pitch)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background nil)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background variable-pitch)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background nil)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-add-font 126 147 shr-link)
  1 <- shr-add-font: nil
  ======================================================================
  1 -> (shr-add-font 149 163 bold)
  1 <- shr-add-font: nil
  ======================================================================
  1 -> (shr-face-background variable-pitch)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background nil)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background variable-pitch)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background nil)
  1 <- shr-face-background: nil
  ======================================================================
  1 -> (shr-face-background (variable-pitch (:background "#ffffff")))
  1 <- shr-face-background: (:background "#ffffff")
  ======================================================================
  1 -> (shr-face-background (variable-pitch bold (:background "#ffffff")))
  1 <- shr-face-background: (:background "#ffffff")
  ======================================================================
  1 -> (shr-face-background (variable-pitch shr-link (:background "#ffffff")))
  1 <- shr-face-background: (:background "#ffffff")
  ======================================================================
  1 -> (shr-face-background (variable-pitch bold (:background "#ffffff")))
  1 <- shr-face-background: (:background "#ffffff")

Re: Errors in redisplay in eww

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Hope you will be able to tell, or at least ask some questions that
> will allow me to find that out.

Is the `variable-pitch' face valid for you?

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

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 02 Jan 2016 06:54:56 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Hope you will be able to tell, or at least ask some questions that
> > will allow me to find that out.
> 
> Is the `variable-pitch' face valid for you?

Yes, both before and after visiting that URL.

Re: Errors in redisplay in eww

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Wed, 30 Dec 2015 14:00:43 +0000

 >>> The face is (nil (:background "#ffffff")), and the problem happens
 >>> when we try to merge its car (which is nil, not a valid face).

 >> If I’m reading the code correctly, the 'face property value is set
 >> to either a font or the value returned by the shr-face-background
 >> function, /or/ the value passed to shr-add-font is prepended to the
 >> current face list.  Could you please try doing, say,
 >> trace-function-background on these two functions and see if the
 >> former ever returns anything suspicious (like the list above; and
 >> for what inputs), or if the latter is ever called with a nil TYPE
 >> argument?

 > The contents of the trace buffer is below.  Does this reveal anything
 > interesting?

	I see nothing suspicious there, alas.  I have also tried
	rendering the HTML code of said Web page with a just built Emacs
	(c988877f), but see no nils in the resulting 'face lists.

	FWIW, the shr-face-background function uses consp where I guess
	face-list-p would be more appropriate.  Still, I see no way it
	could be related to the issue described.

  1796	(defun shr-face-background (face)
  1797	  (and (consp face)
  1798	       (let ((background nil))
  1799		 (dolist (elem face)

[…]

-- 
FSF associate member #7257  http://am-1.org/~ivan/      … 3013 B6A0 230E 334A

Re: Errors in redisplay in eww

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Hope you will be able to tell, or at least ask some questions that
>> > will allow me to find that out.
>> 
>> Is the `variable-pitch' face valid for you?
>
> Yes, both before and after visiting that URL.

shr adds the colours (mostly) by using `add-face-text-property'.  Could
there be a bug somewhere in that code?

(defun shr-colorize-region (start end fg &optional bg)
  (when (and shr-use-colors
             (or fg bg)
             (>= (display-color-cells) 88))
    (let ((new-colors (shr-color-check fg bg)))
      (when new-colors
	(when fg
	  (add-face-text-property start end
				  (list :foreground (cadr new-colors))
				  t))
	(when bg
	  (add-face-text-property start end
				  (list :background (car new-colors))
				  t)))
      new-colors)))


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

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sun, 03 Jan 2016 10:19:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Hope you will be able to tell, or at least ask some questions that
> >> > will allow me to find that out.
> >> 
> >> Is the `variable-pitch' face valid for you?
> >
> > Yes, both before and after visiting that URL.
> 
> shr adds the colours (mostly) by using `add-face-text-property'.  Could
> there be a bug somewhere in that code?

No, add-face-text-property and shr-colorize-region are not the
culprit, at least they don't seem to be.  The problem is here:

	      (dolist (line lines)
		(end-of-line)
		(let ((start (point)))
		  (insert
		   line
		   (propertize " "
			       'display `(space :align-to (,pixel-align))
			       'face (and (> (length line) 0)
					  (shr-face-background
					   (get-text-property
					    (1- (length line)) 'face line)))
			       'shr-table-indent shr-table-id)
		   shr-table-vertical-line)
		  (shr-colorize-region
		   start (1- (point)) (nth 5 column) (nth 6 column)))
		(forward-line 1))

shr-face-background returns nil when get-text-property returns just
variable-pitch, so we get 'face nil' in the properties, and the rest
is history.

Can you tell how come get-text-property returns something else in your
case?  Where do the colors in the line's face come from?  If they come
from shr-colorize-region, then I never see any fg color but nil there,
and bg is almost always nil, except when it's (:background "#ffffff").

HTH

Re: Errors in redisplay in eww

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> No, add-face-text-property and shr-colorize-region are not the
> culprit, at least they don't seem to be.  The problem is here:
>
> 	      (dolist (line lines)
> 		(end-of-line)
> 		(let ((start (point)))
> 		  (insert
> 		   line
> 		   (propertize " "
> 			       'display `(space :align-to (,pixel-align))
> 			       'face (and (> (length line) 0)
> 					  (shr-face-background
> 					   (get-text-property
> 					    (1- (length line)) 'face line)))
> 			       'shr-table-indent shr-table-id)
> 		   shr-table-vertical-line)
> 		  (shr-colorize-region
> 		   start (1- (point)) (nth 5 column) (nth 6 column)))
> 		(forward-line 1))
>
> shr-face-background returns nil when get-text-property returns just
> variable-pitch, so we get 'face nil' in the properties, and the rest
> is history.

I see.

> Can you tell how come get-text-property returns something else in your
> case?

No, it has to return the same as you're seeing.  Is it possible that
my Emacs just isn't warning about these nil faces?

The right fix here is probably not to propertize the `face' is
`shr-face-background' returns nil, I guess.

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

Re: Errors in redisplay in eww

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 09 Jan 2016 10:13:16 +0100
> 
> > Can you tell how come get-text-property returns something else in your
> > case?
> 
> No, it has to return the same as you're seeing.  Is it possible that
> my Emacs just isn't warning about these nil faces?

Did you look in *Messages*?  These warnings are silent when they
happen as part of redisplay (because showing them in the echo area
would re-enter redisplay and cause an infinite loop).

I don't think it's possible for this not to produce a warning, the
code that produces them is unconditional.

Thanks.

Re: Errors in redisplay in eww

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Did you look in *Messages*?  These warnings are silent when they
> happen as part of redisplay (because showing them in the echo area
> would re-enter redisplay and cause an infinite loop).

Oops.  No, I didn't.  They're there:

Invalid face reference: nil [16 times]

I'll fix the bug...

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

Re: Errors in redisplay in eww

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Did you look in *Messages*?  These warnings are silent when they
> happen as part of redisplay (because showing them in the echo area
> would re-enter redisplay and cause an infinite loop).
>
> I don't think it's possible for this not to produce a warning, the
> code that produces them is unconditional.

This has now been fixed in emacs-25.

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