Closed Bug 406782 Opened 14 years ago Closed 14 years ago

double rendering of some bullets on wikimo

Categories

(Core Graveyard :: GFX, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: vlad)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

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.
Version: unspecified → Trunk
Is this a recent regression?
It's been at least a few weeks, I saw it a while ago but forgot to file it.
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.
Blocks: 390912
Flags: blocking1.9?
Keywords: regression
Attached file minimal testcase
Minimized as far as I could get.  It seems to require an <a> inside a <h1> right before the <ul>.
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.
nsCSSRendering::PaintDecorationLine is calling NewPath, so that's probably not it.  I can't reproduce using -chrome with the testcase, which is weird.
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.
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.
Duplicate of this bug: 406656
+'ing with P3.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: nobody → vladimir
> 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.
Attached patch patchSplinter Review
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.
Attachment #295812 - Flags: review?(roc)
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?
Attachment #295812 - Flags: superreview+
Attachment #295812 - Flags: review?(roc)
Attachment #295812 - Flags: review+
Could be, it was NS_ERROR below so I just kept it.  I can change both things to NS_ASSERTION if you'd prefer.
Checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Probably deserves a reftest.
Flags: in-testsuite?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.