365336 - text-decoration width should be rounded to the device pixels

Status: RESOLVED FIXED

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

Description

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

We fixed the bug 287624.

The text-decoration has similar problem. We should round the width of the text-decorations at rendering time. (If we implement the 'text-underline-width', 'text-line-through-width' and 'text-overline-width', we can be rounding in style system. But the spec may not be stable.)

I think that this bug doesn't happen on Windows. Because the width of the lines are always returns the rounded value on Windows. I don't know on Linux. I confirmed on Mac.

Comment 1

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

Attachment: screenshot on Mac

Comment 2

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

We currently get this information from font metrics -- and the font metrics may be giving subpixel widths on some platforms and not others.

Comment 3

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

dbaron:

thanks for the comment.

I think that we should implement the common function that draws the decoration lines in nsCSSRendering, and all frames should use this for drawing docoration lines. If so, we will happy for implementing the text-decoration styles in CSS3.

Comment 4

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

We get the information from font metrics, but we currently draw the decorations in cross-platform code in nsHTMLContainerFrame, nsTextFrame, etc.

Comment 5

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

Attachment: testcase for nsHTMLContainterFrame

Comment 6

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

Attachment: testcase for nsTextFrame

Comment 7

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

Attachment: Patch rv1.0

This patch supports 2 or more pixels dotted line that is used for spell checker. And it also supports dashed style. That will be used for the composition string of IME on Windows. And it also supports double style. But the double style doesn't works fine, see bug 365414.

The dotted style and dashed styles start the draw with same direction of the text, therefore, if the text is RTL, the right most dot/dash is not painted as shortly.

This patch is including the xul text box frame (nsTextBoxFrame), but I don't know how to test this frame...

Comment 8

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

roc:

Please check this, this changes few lines in the nsTextFrame.

Comment 9

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

All the text in e.g. menus is in nsTextBoxFrames.

Looks OK.

Comment 10

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

Attachment: testcase for xul

looks like the patch works fine on all cases.

Comment 11

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

Attachment: Patch rv1.1

This patch is Patch rv1.0 + patch for nsTextFrameThebes. But if I enable nsTextFrameThebes, Fx crashed at starting up. So, I didn't test for it.

Comment 12

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

I don't think we should round the horizontal coordinates (x and width). Fractional values there are not very visible.

+  if (aOffset > 0 || (aOffset == 0 && aAscent == 0)) {
+    // The offset is including the preferred size, we should remove it
+    y += preferredSize;
+  }

Why is this needed?

Comment 13

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

(In reply to comment #12)
> I don't think we should round the horizontal coordinates (x and width).
> Fractional values there are not very visible.

o.k.

> +  if (aOffset > 0 || (aOffset == 0 && aAscent == 0)) {
> +    // The offset is including the preferred size, we should remove it
> +    y += preferredSize;
> +  }
> 
> Why is this needed?

I think that if the decoration line is over line, we should align the bottom of the line to the offset position. (for the line should not overlap to the text.) And I think that if the decoration line is line through, we should align the middle of the line to offset position. (for the line should be on middle of the ascent of the text.)

If the actual height is same as preferred size which is specified by the font, the addition will be denied in the next switch statement. (But I should use |aDecoration| for the if statement, probably...)

Comment 14

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

ok, but I don't understand why we have to have both size and preferredsize.

Comment 15

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

(In reply to comment #14)
> ok, but I don't understand why we have to have both size and preferredsize.

Because the offsets for the over line and the line through are including the preferredsize. We have another way that is the callers remove the preferred size from the offset value. But the way is not better than current way. The way will create some new bugs if the caller forgets (or don't know) the rule.

Comment 16

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

Attachment: Patch rv1.2

updating to latest trunk. and this patch is not rounding the x offset and width.

Comment 19

[Ray Kiddy]

Attachment: test case

demonstration of 1 pixel separation between letters and underline in one paragraph and 0 pixel separation in other. remove first paragraph to see correct display.

Comment 21

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

Attachment: Patch rv2.0

Now, we can use thebes directly in nsCSSRendering.

Therefore, this patch can render better lines than rv1.x.

Roc, I have a question. In nsTextFrameThebes, some gfxFloat (or gfxPoint) params have nscoord (or nsPoint) values. Is it a mistake? Or is there a reason? It is unreadable...

Comment 22

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

Oops, sorry.

> +/**
> + * round the app unit to the nearest device pixel.
> + * 'l' is a length by app units. 'aupp' is the app units per the device pixel.
> + */
> +#define ROUND_TO_NEAREST_PIXELS(l,aupp) \
> +  (((l) == 0) ? 0 : \
> +    ((l) > 0) ? ((l) + ((aupp) / 2)) / (aupp) * (aupp) : \
> +                ((l) - ((aupp) / 2)) / (aupp) * (aupp))
> +

Now, this can be removed. Please ignore this.

Comment 23

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

(In reply to comment #19)
> demonstration of 1 pixel separation between letters and underline in one
> paragraph and 0 pixel separation in other. remove first paragraph to see
> correct display.

Ray:

This patch cannot fix the positioning bug perfectly. We need more work for it...

Comment 24

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

> +  // The y position should be lifted up to half size for the painted line being
> +  // aligned to device pixel.
> +  y -= size / 2;

fm... it may pull down. (i.g., '+=')

Comment 25

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

Attachment: Patch rv2.1

Sorry, this is correct. We should down the line half size.

Comment 26

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

And the rv2 patch also fixes the AAed underline for spell checker in new textframe.

Comment 27

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

What does PaintDecorationLine do that FillClippedRect doesn't do? FillClippedRect is snapping to device pixels when possible.

Comment 28

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

(In reply to comment #27)
> What does PaintDecorationLine do that FillClippedRect doesn't do?
> FillClippedRect is snapping to device pixels when possible.

If the line is solid, they are same result. Otherwise, FillClippedRect cannot render the other style lines. I hope that we integrate the all text-decoration rendering to one function for bug 59109 and bug 367614.

Comment 29

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

roc:

would you continue to review?

Comment 30

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

-  gfxFloat pix2app = mTextRun->GetAppUnitsPerDevUnit();
+  gfxFloat p2t = mTextRun->GetAppUnitsPerDevUnit();

Why are you changing the name? The existing name is much better; "twips" is not accurate at all.

+  nsPoint pt(NSToCoordRound(float(aFramePt.x)),
+             NSToCoordRound(float(aFramePt.y)));

Why do you need these float() casts, here and elsewhere?

+  nscoord width = NSToCoordRound(float(GetRect().width));
+  nscoord ascent = NSToCoordRound(float(mAscent));

Why are you doing these conversions? These are already nscoords

+    nsCSSRendering::PaintDecorationLine(aTextPaintStyle.PresContext(),
+      aCtx, overColor,
+      pt, width, size, 0, 0, size,
+      NS_STYLE_TEXT_DECORATION_OVERLINE, NS_STYLE_BORDER_STYLE_SOLID,

Wouldn't it be more correct to pass ascent and -ascent instead of 0 for the ascent and offset?

+   *     @param aSize             the height of the decoration line

How about calling it aHeight? Similar for aPreferredSize/aPreferredHeight.

I think the parameters to nsCSSRendering::PaintDecorationLine should be gfxFloats instead of nscoord. That would avoid lossy rounding and simplify code.

I don't think you need to Save() and Restore() here. nsTextFrameThebes doesn't. We don't require stroke state to be preserved.

+  gfxFloat y = NS_round((aPt.y + aAscent - aOffset) * t2p);

I think you need to round aOffset to device pixels before using it. Otherwise, if aOffset is not an integer number of pixels, the offset between the baseline and the decoration line may be drawn with a different number of pixels depending on the value of aPt.y + aAscent.

+  gfxFloat dashWidth = 0.0;
+  gfxFloat dash[2];

Make these local to the code blocks that use them.

+    case NS_STYLE_TEXT_DECORATION_OVERLINE:
+      // The offset is including the preferred size, we should remove it
+      y += preferredSize;
+      // the bottom of the decoration line should be aligned to the top of the
+      // text.
+      y -= totalHeight;
+      break;
+    case NS_STYLE_TEXT_DECORATION_LINE_THROUGH: {
+      // The offset is including the preferred size, we should remove it
+      y += preferredSize;
+      // the middle of the decoration line should be aligned to the middle of
+      // the original strike out offset.
+      gfxFloat halfsize;
+      if (preferredSize > totalHeight / 2.0)
+        halfsize = preferredSize;
+      else {
+        halfsize = NS_round(totalHeight / 2.0);
+      }
+      y -= halfsize;
+      break;

I think you should make these adjustments to aOffset instead of y, and do them before we round aOffset to pixels.

Comment 31

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

(In reply to comment #30)
> +  nsPoint pt(NSToCoordRound(float(aFramePt.x)),
> +             NSToCoordRound(float(aFramePt.y)));
> 
> Why do you need these float() casts, here and elsewhere?

non-casted converting from double (gfxFloat) to float makes warning on VC.

Comment 32

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

(In reply to comment #30)
> I don't think you need to Save() and Restore() here. nsTextFrameThebes doesn't.
> We don't require stroke state to be preserved.

The dotted/dashed style breaks current working, we need them...

Comment 33

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

Attachment: Patch rv3.0

Roc, by your suggestion, the offset problem is fixed! It's great.

Comment 34

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

(In reply to comment #31)
> (In reply to comment #30)
> > +  nsPoint pt(NSToCoordRound(float(aFramePt.x)),
> > +             NSToCoordRound(float(aFramePt.y)));
> > 
> > Why do you need these float() casts, here and elsewhere?
> 
> non-casted converting from double (gfxFloat) to float makes warning on VC.

Then I think you should add an overloaded version of NSToCoordRound that takes a double parameter.

> The dotted/dashed style breaks current working, we need them...

What do you mean? There are places where we assume that the current line style is solid? Where? Even if so, you can easily move Save() into the DOTTED/DASHED cases of the first switch, and move Restore() into the DOTTED/DASHED cases of the last switch.

Could you use names like offsetPixel instead of offset_pixel to be more consistent with the surrounding code?

+      if (aPreferredHeight > totalHeight / 2.0)
+        halfHeight = aPreferredHeight;
+      else {
+        halfHeight = totalHeight / 2.0;
+      }

use PR_MAX.

+      // The offset is including the preferred size, we should remove it

// The offset includes the preferred height, we should remove it

+  // The y position should be downed to half size for the painted line being
+  // aligned to device pixel.

// The y position should be set to the middle of the line.

Comment 35

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

Attachment: Patch rv3.1

(In reply to comment #34)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > +  nsPoint pt(NSToCoordRound(float(aFramePt.x)),
> > > +             NSToCoordRound(float(aFramePt.y)));
> > > 
> > > Why do you need these float() casts, here and elsewhere?
> > 
> > non-casted converting from double (gfxFloat) to float makes warning on VC.
> 
> Then I think you should add an overloaded version of NSToCoordRound that takes
> a double parameter.

yeah, but the code has been removed, should I add the new function in this patch?

Comment 36

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

Attachment: Patch rv3.2

Oops, sorry. I forgot some changes.

Comment 37

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

Attachment: Patch rv3.2.1

Sorry, this is better..

Comment 38

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

Comment on attachment 275010 [details] [diff] [review]
Patch rv3.2.1

ok great!

Comment 39

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

checked-in.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> +    gfxPoint pt((start + aPt.x) * a2p, (aLine->mBounds.y + aPt.y) * a2p);

Is there a reason this is not using AppUnitsToDevPixels?  If the problem is that this rounds, perhaps we just need an API that doesn't do that instead of rolling our own?  Or perhaps this code should use NSAppUnitsToFloatPixels directly with the value it got off the prescontext?

Thing is, as the code is written (doing |foo * (1.0 / bar)|) you actually get rounding errors compared to doing |foo / bar| directly.  We don't really want that.

Comment 41

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

yeah, I need non-rounded pixels out of nsCSSRendering. And now, gfxFloat is double, not float.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Sounds like we should have a followup bug on the API impedance mismatch, then.  This code should really be able to use NSAppUnitsToFloatPixels or equivalent instead of having to hand-roll it.

Comment 43

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

we should also have a followup patch to use division here instead of multiplication (or preferably the new API).