406782 - double rendering of some bullets on wikimo

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[(not currently active) Ted Mielczarek]

I've noticed this on a few pages on wikimo, the second level bullets appear to get double-rendered.  See the page in the URL, look at the bullets in front of "#  docs are ready and promoted on mdc " for example.  On both Win32 and Mac they look very dark, but if you scroll them off screen slowly then back on, they look normal.  However, if you scroll them quickly, using pageup/pagedown, they retain the dark look.  I'm not sure at all what's happening there.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Is this a recent regression?

Comment 2

[(not currently active) Ted Mielczarek]

It's been at least a few weeks, I saw it a while ago but forgot to file it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I see the same behavior on Linux, with the regression range being from 2007-08-06-02 to 2007-08-07-01:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-06+02&maxdate=2007-08-07+01&cvsroot=%2Fcvsroot

Comment 4

[Boris Zbarsky [:bzbarsky]]

Probably a regression from bug 390912.  A minimal testcase attached to the bug (so we don't have to worry about the page dying) would be nice.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: minimal testcase

Minimized as far as I could get.  It seems to require an <a> inside a <h1> right before the <ul>.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Just a guess: something's not calling NewPath(), perhaps the code that draws the underline -- so it's picking up the path that was used for the bullet.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

nsCSSRendering::PaintDecorationLine is calling NewPath, so that's probably not it.  I can't reproduce using -chrome with the testcase, which is weird.

Comment 8

[Boris Zbarsky [:bzbarsky]]

nsCSSRendering::PaintDecorationLine calls SetLineWidth, though.  I don't see it undoing that anywhere.

On the other hand, nsBulletFrame::PaintBullet just does a nsIRenderingContext::DrawEllipse call, which calls Ellipse() on the gfxContext, etc.  Nowhere does it set the line width.  Stepping into it, the cr->gstate->stroke_style.line_width is too big.

So we're just stroking the ellipse with the line-width from the underline.

Not sure whether PaintBullet (or DrawEllipse() or Ellipse()) should be setting the line width or whether PaintDecorationLine should be restoring the old line width.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Given the number of nsCSSRendering callers that set and don't seem to restore the line width, I would guess the former.  The only question is what the right API level is for it.

Comment 11

[Damon Sicore (:damons)]

+'ing with P3.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> nsCSSRendering::PaintDecorationLine calls SetLineWidth, though.  I don't see it
> undoing that anywhere.

Ah ha!

> Given the number of nsCSSRendering callers that set and don't seem to restore
> the line width, I would guess the former.  The only question is what the right
> API level is for it.

Nah, all the other nsCSSRendering callers call SetLineWidth in between Save()/Restore() -- PaintDecorationLine only does Save/Restore for dashed and dotted decorations.  I'll cook up a patch to save the current color and line width and reset them correctly in the case of solid lines.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch

Fix.  Moved the decoration value check at the top so I didn't have to deal with restoring the right thing in multiple places and/or using a goto.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 295812 [details] [diff] [review]
patch

+  if (aDecoration != NS_STYLE_TEXT_DECORATION_UNDERLINE &&
+      aDecoration != NS_STYLE_TEXT_DECORATION_OVERLINE &&
+      aDecoration != NS_STYLE_TEXT_DECORATION_LINE_THROUGH)
+  {
+    NS_ERROR("Invalid decoration value!");
+    return;
+  }

Shouldn't this just be NS_ASSERTION?

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Could be, it was NS_ERROR below so I just kept it.  I can change both things to NS_ASSERTION if you'd prefer.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in

Comment 17

[(not currently active) Ted Mielczarek]

Probably deserves a reftest.