746421 - nsTextBoxFrame doesn't include glyph overflow in its overflow rects

Status: RESOLVED FIXED

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

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Include the LOOSE_INK_EXTENTS

UL text frames only use the tight extents of the text for their size, they never include the loose extents in the overflow area.

This causes incorrect invalidation, and with DLBI patches, test failures.

Somewhat guessing with the attached patch, running it on tryserver now to see if it fixes the test failures.

Comment 2

[Matt Woodrow (:mattwoodrow)]

In addition to this, it appears that we are only using the LOOSE_INK_EXTENTS with normal html text frames when we recompute the overflow area. The initial reflow uses TIGHT_HINTED_OUTLINE_EXTENTS (except when IsFloatingFirstLetterChild() - not sure what that means).

See:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#7451

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#7916

I believe a similar bug also exists for nsMathMLChar, harder to figure out the measurement code there though.

Does anyone have time to help with this? It's blocking DLBI patches from landing.

Alternatively I can just extend the invalidation areas of text temporarily until we can fix this properly.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 615974 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS

Review of attachment 615974 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a plausible approach to me. I see a couple of failures on tryserver that still look like they're due to glyph antialiasing overflow, but maybe that's due to using the wrong(?) rect in the UnionRect operation? Or do we maybe need two pixels of "slop", not just one, at the left as well as the right edge?

Also, I suspect you should also incorporate the ascent/descent from the bounding metrics into the visual overflow area, as it's possible the ink might overflow the typographic bounds vertically as well as horizontally.

::: layout/xul/base/src/nsTextBoxFrame.cpp
@@ +980,5 @@
> +                                  gfxFont::LOOSE_INK_EXTENTS);
> +
> +
> +    textRect.x -= metrics.leftBearing;
> +    textRect.width = metrics.width;

What's textRect intended for? I don't see where this is being used...

@@ +982,5 @@
> +
> +    textRect.x -= metrics.leftBearing;
> +    textRect.width = metrics.width;
> +
> +    bounds.UnionRect(bounds, mTextDrawRect);

... unless... did you mean to use it here?

@@ +991,3 @@
>        // Our scrollable overflow is our bounds; our visual overflow may
>        // extend beyond that.
>        nsPoint origin(0,0);

Nothing to do with your patch, but this looks like a left-over from....something? Let's kill it while we're here.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Include the LOOSE_INK_EXTENTS v2

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 616433 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v2

Review of attachment 616433 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsFontMetrics.h
@@ +220,5 @@
>      nsBoundingMetrics GetBoundingMetrics(const PRUnichar *aString,
>                                           PRUint32 aLength,
> +                                         nsRenderingContext *aContext,
> +                                         gfxFont::BoundingBoxType aType = 
> +                                           gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS);

I wonder whether, instead of adding a parameter with a default value (to avoid having to update all existing callers, obviously), it'd be clearer to have a separate method with a more explicit name such as GetInkBoundsForVisualOverflow? Everything else about nsFontMetrics is dealing with "true" metrics that come from the font/glyph data, without considering artifacts of rasterization and antialiasing, so this is seems like a sufficiently different concept that perhaps it deserves a distinct API.

The two public methods could be inline wrappers around a shared private implementation, so the end result would be essentially the same code, but it might make things clearer at call sites. WDYT?

::: layout/xul/base/src/nsTextBoxFrame.cpp
@@ +984,5 @@
> +    textRect.width = metrics.width;
> +    // In DrawText() we always draw with the baseline at MaxAscent() (relative to mTextDrawRect), 
> +    // so any ascent on the text is guaranteed to be within the rect already. We need to then adjust
> +    // our overflow height to include this plus the descent of the text.
> +    NS_ASSERTION(fontMet->MaxAscent() >= metrics.ascent, "More than maximum ascent?");

I'm not sure this is necessarily a good assumption. Currently we only add "padding" outside the true outline of the glyph to allow for antialiasing pixels on the left and right edges, but it's possible that - especially with much tighter invalidation/drawing - we may need to do the same at top/bottom, in which case the loose (or tight) ink extents might exceed the font's nominal maxascent/descent.

In practice the "antialiasing bleed" problem is much more of an issue horizontally than vertically, but it can occur in all directions (depending on the font rasterization mode, etc).

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I wonder whether, instead of adding a parameter with a default value (to
> avoid having to update all existing callers, obviously), it'd be clearer to
> have a separate method with a more explicit name such as
> GetInkBoundsForVisualOverflow? Everything else about nsFontMetrics is
> dealing with "true" metrics that come from the font/glyph data, without
> considering artifacts of rasterization and antialiasing, so this is seems
> like a sufficiently different concept that perhaps it deserves a distinct
> API.
> 
> The two public methods could be inline wrappers around a shared private
> implementation, so the end result would be essentially the same code, but it
> might make things clearer at call sites. WDYT?

Seems reasonable to me, will do.

 
> I'm not sure this is necessarily a good assumption. Currently we only add
> "padding" outside the true outline of the glyph to allow for antialiasing
> pixels on the left and right edges, but it's possible that - especially with
> much tighter invalidation/drawing - we may need to do the same at
> top/bottom, in which case the loose (or tight) ink extents might exceed the
> font's nominal maxascent/descent.
> 
> In practice the "antialiasing bleed" problem is much more of an issue
> horizontally than vertically, but it can occur in all directions (depending
> on the font rasterization mode, etc).

Right, I (wrongly) assumed that the assertion I added was a valid assumption. I've adjusted the code to take the actual ascent into account.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Include the LOOSE_INK_EXTENTS v3

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 616510 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v3

Review of attachment 616510 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine - assuming it actually helps with the issues you were seeing, let's do it.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 616510 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v3

Review of attachment 616510 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsFontMetrics.cpp
@@ +393,5 @@
> +nsFontMetrics::GetBoundingMetrics(const PRUnichar *aString, PRUint32 aLength,
> +                                  nsRenderingContext *aContext)
> +{
> +  return GetTextBoundingMetrics(this, aString, aLength, aContext, gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS);
> +  

Hmm, looks like there's a redundant blank line here.

Maybe worth putting this and GetInkBounds... into the the .h file as inline methods (and make GetTextBoundingMetrics a private helper declared in the class)? I'll leave it up to you, I don't suppose it's a major issue either way.

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35dfe0d44d83

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/35dfe0d44d83

Comment 12

[IU]

Is it at all possible that this is the cause of bug 749658?

Comment 13

[[e-mail]]

Comment on attachment 616510 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v3

>+    nsBoundingMetrics metrics = 
>+      fontMet->GetInkBoundsForVisualOverflow(mTitle.get(), mTitle.Length(),
>+                                             aBoxLayoutState.GetRenderingContext());
I don't claim to understand this code but my guess is that bug 749658 was caused because this should have used mCroppedTitle instead of mTitle.

Comment 14

[[e-mail]]

...although I'm still seeing "ASSERTION: Wrong bounds" when dragging tabs...

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 616510 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v3

Review of attachment 616510 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/nsTextBoxFrame.cpp
@@ +985,5 @@
> +    textRect.y += fontMet->MaxAscent() - metrics.ascent;
> +    textRect.height = metrics.ascent + metrics.descent;
> +
> +    bounds.UnionRect(bounds, textRect);
> +    nsOverflowAreas overflow(bounds, bounds);

I don't think it's right to store |bounds| for both visual *and* scrollable overflow, is it? It's appropriate for visual overflow, but we shouldn't have changed the scrollable overflow here.

Comment 16

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

Yes. Please fix!

Comment 17

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

Oh, I see that patch now :-)

Comment 18

[[e-mail]]

(In reply to comment #14)
> ...although I'm still seeing "ASSERTION: Wrong bounds" when dragging tabs...
Must be something else, because I still see it with attachment 619432 [details] [diff] [review].

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to [e-mail] from comment #18)
> (In reply to comment #14)
> > ...although I'm still seeing "ASSERTION: Wrong bounds" when dragging tabs...
> Must be something else, because I still see it with attachment 619432 [details] [diff] [review]
> [details] [diff] [review].

Could you file this as a new bug with detailed STR, please? I'm not seeing assertions when I drag tabs around in my current debug build, so I must be missing something....