417967 - text-decoration lines (underline/strikeout/overline) are sometimes anti aliased in sub frames

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

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

Attachment: Patch v1.0

In some sub frames, the text-decoration lines are anti aliased.

We should snap the line rect to device pixels when we can do it.

I referenced gfxContext::Rectangle code for the attached patch. I'm not sure this is correct...

Comment 1

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

Comment on attachment 303764 [details] [diff] [review]
Patch v1.0

er, this patch is incorrect.

Comment 2

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

Attachment: Patch v1.1

We should not do the extra work if the rect is not changed, and if the context is already save, we must not re-save it.

Comment 3

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

Comment on attachment 303767 [details] [diff] [review]
Patch v1.1

mmm.... this approach is incorrect. Because the underlines are not always positioned to same offset from each baseline.

Comment 4

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

Attachment: Patch rv1.2

I think that this is correct. If we can snap to device pixels, we need to send the device pixels to GetTextDecorationRectInternal, otherwise, we can send the user pixels.

# Note that the bottom changing in the patch is removing the non-needed function call.

Comment 5

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

Attachment: Patch rv1.3

This patch also fixes the same bug in print preview.

Comment 6

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

This patch won't always work well because nsTextFrameThebes needs to snap the text baseline consistently with the underline or we get the wrong size gap between the text and the underline.

To be honest, I was planning to not fix this for 1.9. It's not that common and it's not that bad when it happens. When we do Compositor post-1.9, it won't happen anymore because we'll always paint into a widget whose appunit origin is pixel-aligned.

Comment 7

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

(In reply to comment #6)
> This patch won't always work well because nsTextFrameThebes needs to snap the
> text baseline consistently with the underline or we get the wrong size gap
> between the text and the underline.

I'm not sure what case you said. Actually, I can see the text is rendered in wrong position at scrolling, but it's rare case in my experiments. I believe that this patch works fine in most cases.

> To be honest, I was planning to not fix this for 1.9. It's not that common and
> it's not that bad when it happens. When we do Compositor post-1.9, it won't
> happen anymore because we'll always paint into a widget whose appunit origin is
> pixel-aligned.

Sounds great. But I hope that we fix this bug in most cases except that the current patch has a damage for tp value.

Comment 8

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

roc:

And note that maybe, you see the AAed underline with AAed text. However, CJK text of Win32 are rendered bitmap glyphs in most cases. So, the AAed underline is a striking difference.

Comment 9

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

Attachment: screenshot (AAed underline and bitmap glyphs)

Comment 10

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

Attachment: screenshot in printpreview

especially, the print preview rendering is too ugly.

Comment 11

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

This patch adds complexity and it's not fully correct because it makes the algorithm for rounding the position of the underline different from the algorithm nsTextFrameThebes uses to round the baseline of text. I don't know exactly what testcases this will break but it will definitely break something and then we'll have to waste time trying to figure out what caused the break, and we'll also run the risk that the break is serious. I really don't want to do this.

Comment 12

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

If this is really specific to subframes, shouldn't we be pixel-snapping the interior of the frame instead?

Comment 13

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

I'll take a look at this and see if I can do something like that.

Comment 14

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

Attachment: testcase

This testcase shows the problem if I zoom in one level. The text outside the IFRAME still has a good underline but the text inside the IFRAME has an antialiased underline.

Comment 15

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

Attachment: better testcase

If I put the testcase into standards mode (which I should have done in the first place) it shows the problem without zooming.

Comment 16

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

Basically nsDisplayList::Paint and its callers e.g. nsLayoutUtils::PaintFrame and PresShell::Paint want (0,0) in the incoming rendering context to map to something pixel-aligned; the display item implementations do rounding based on that assumption. This breaks down in nsViewManager::RenderViews (and its caller nsViewManager::Refresh) which translate the rendering context so that (0,0) is transformed to the root view's origin, which may be at a fractional offset relative to the origin of the widget.

I'll try just snapping the root view origin to device pixels through the gfxContext. This should fix up the pixel alignment in a way that's consistent with other draw snapping (e.g. border drawing).

Comment 17

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

Attachment: fix

This is a little bit gnarly but the comments explain it, I hope.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 304957 [details] [diff] [review]
fix

I assume that it's not an issue that we're translating the rc and the dirty rect by different amounts, because the dirty rect needs to line up with frame coords while the rc needs to line up with the widgets?

If that's OK, r+sr=bzbarsky.

Comment 19

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

The patch also fixes some case of bug 408913. But there are some buggy cases.

Comment 20

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

(In reply to comment #18)
> (From update of attachment 304957 [details] [diff] [review])
> I assume that it's not an issue that we're translating the rc and the dirty
> rect by different amounts, because the dirty rect needs to line up with frame
> coords while the rc needs to line up with the widgets?

That's the idea, yes. Thanks!

(In reply to comment #19)
> The patch also fixes some case of bug 408913. But there are some buggy cases.

You mean there are some buggy cases caused by this patch, or there are some existing buggy cases not fixed by this patch?

Comment 21

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

(In reply to comment #20)
> (In reply to comment #19)
> > The patch also fixes some case of bug 408913. But there are some buggy cases.
> 
> You mean there are some buggy cases caused by this patch, or there are some
> existing buggy cases not fixed by this patch?

Maybe, latter. It seems that the patch changes the conditions of the bug.
The patch fixes the bug when the testcases are with the current reproduceable conditions. However, some other cases become buggy. (Probably, it's not a regression. The patch only changes the conditions of the testcases, I think.)

Comment 22

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

seems likely yes.

Comment 23

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

checked in.

Comment 24

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

-> v.

thank you.

Comment 25

[pal-moz]

Attachment: screenshot

regression ?
http://forums.mozillazine.org/viewtopic.php?p=3273035#3273035

Comment 26

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

Yes, I think so. It needs a new bug with a testcase

Comment 27

[pal-moz]

I cannot create a testcase.
please file a bug, if you/someone can.

Comment 28

[Daniel.S]

This checkin is also responsible for the black vertical line that recently appeared on Acid3.

The regression range for Acid3's breakage was:
Working: 20080224_1731_firefox-3.0b4pre
Broken (black vertical lines): 20080224_1809_firefox-3.0b4pre
-> This Bug.

Comment 29

[pal-moz]

(In reply to comment #25)
> Created an attachment (id=306176) [details]
> screenshot
> 

filed, no testcase.
bug 420413