Closed Bug 417967 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: masayuki, Assigned: roc)

References

()

Details

Attachments

(6 files, 4 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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...
Attachment #303764 - Flags: superreview?(roc)
Attachment #303764 - Flags: review?(vladimir)
Comment on attachment 303764 [details] [diff] [review]
Patch v1.0

er, this patch is incorrect.
Attachment #303764 - Flags: superreview?(roc)
Attachment #303764 - Flags: review?(vladimir)
Attachment #303764 - Flags: review-
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #303764 - Attachment is obsolete: true
Attachment #303767 - Flags: superreview?(roc)
Attachment #303767 - Flags: review?(vladimir)
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.
Attachment #303767 - Flags: superreview?(roc)
Attachment #303767 - Flags: review?(vladimir)
Attachment #303767 - Flags: review-
Attached patch Patch rv1.2 (obsolete) — Splinter Review
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.
Attachment #303767 - Attachment is obsolete: true
Attachment #303795 - Flags: superreview?(roc)
Attachment #303795 - Flags: review?(vladimir)
Attached patch Patch rv1.3 (obsolete) — Splinter Review
This patch also fixes the same bug in print preview.
Attachment #303795 - Attachment is obsolete: true
Attachment #303800 - Flags: superreview?(roc)
Attachment #303800 - Flags: review?(vladimir)
Attachment #303795 - Flags: superreview?(roc)
Attachment #303795 - Flags: review?(vladimir)
Flags: blocking1.9?
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.
(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.
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.
especially, the print preview rendering is too ugly.
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.
If this is really specific to subframes, shouldn't we be pixel-snapping the interior of the frame instead?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
I'll take a look at this and see if I can do something like that.
Assignee: masayuki → roc
Status: ASSIGNED → NEW
Attached file 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.
Attached file 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.
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).
Attached patch fixSplinter Review
This is a little bit gnarly but the comments explain it, I hope.
Attachment #303800 - Attachment is obsolete: true
Attachment #304957 - Flags: superreview?(bzbarsky)
Attachment #304957 - Flags: review?(bzbarsky)
Attachment #303800 - Flags: superreview?(roc)
Attachment #303800 - Flags: review?(vladimir)
Whiteboard: [needs review]
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.
Attachment #304957 - Flags: superreview?(bzbarsky)
Attachment #304957 - Flags: superreview+
Attachment #304957 - Flags: review?(bzbarsky)
Attachment #304957 - Flags: review+
The patch also fixes some case of bug 408913. But there are some buggy cases.
(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?
Whiteboard: [needs review] → [needs landing]
(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.)
checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
-> v.

thank you.
Status: RESOLVED → VERIFIED
Yes, I think so. It needs a new bug with a testcase
I cannot create a testcase.
please file a bug, if you/someone can.
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.
(In reply to comment #25)
> Created an attachment (id=306176) [details]
> screenshot
> 

filed, no testcase.
bug 420413
You need to log in before you can comment on or make changes to this bug.