394109 - Inconsistent text-decoration positioning

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

The testcases in bug 365336 no longer work correctly.

Comment 1

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

I don't know when this broke, but I think it was quite recently.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsTextFrameThebes rounds |y + ascent| in |GetSnappedBaselineY|.

but nsCSSRenering does:

4440   gfxFloat y = NS_round(aPt.y + aAscent - offset);

Should we change it to |NS_round(aPt.y + aAscent) - NS_round(offset)|?

Comment 3

[Jesse Ruderman]

Related to bug 392425?

Comment 4

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

> Should we change it to |NS_round(aPt.y + aAscent) - NS_round(offset)|?

Yeah, I think so. Does that fix things?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not in my home, I'll create the patch tonight or tomorrow.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

right, this can fix.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patch has gfx part. gfxRect::Round is called from nsTextFrameThebes, for snapping to dev units. But the rounding is wrong if the values are negative.

Comment 9

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

+    textRect.y = presContext->RoundAppUnitsToNearestDevPixels(textRect.y);

What's this for in nsTextBoxFrame? If we need to do something here, we should probably be computing the baseline (y + ascent), rounding it, and then recomputing y by subtracting the ascent.

 nsTextFrame::PaintTextDecorations(gfxContext* aCtx, const gfxRect& aDirtyRect,
                                   const gfxPoint& aFramePt,
+                                  const gfxPoint& aTextBaselinePt,
                                   nsTextPaintStyle& aTextPaintStyle,

Why not just take aTextBaselinePt only?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

I changed only nsTextBoxFrame.

I think that I should not remove the |const gfxPoint& aFramePt| in nsTextFrameThebes. Because it needs x point of frame rect for drawing, but if it is removed, for RTL case, we need to recalculate the x point from baselinePt.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.