The default bug view has changed. See this FAQ.

nsTextBoxFrame doesn't include glyph overflow in its overflow rects

RESOLVED FIXED in mozilla15

Status

()

Core
Layout: Text
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 615974 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS

XUL 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.
Attachment #615974 - Flags: feedback?(jfkthame)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 746422
(Assignee)

Comment 2

5 years ago
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 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.
Attachment #615974 - Flags: feedback?(jfkthame) → feedback+
Summary: nsTextBoxFrame doesn't include glpyh overflow in its overflow rects → nsTextBoxFrame doesn't include glyph overflow in its overflow rects
(Assignee)

Comment 4

5 years ago
Created attachment 616433 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v2
Attachment #615974 - Attachment is obsolete: true
Attachment #616433 - Flags: review?(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).
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 616510 [details] [diff] [review]
Include the LOOSE_INK_EXTENTS v3
Attachment #616433 - Attachment is obsolete: true
Attachment #616433 - Flags: review?(jfkthame)
Attachment #616510 - Flags: review?(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.
Attachment #616510 - Flags: review?(jfkthame) → review+
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.
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/35dfe0d44d83
Assignee: nobody → matt.woodrow
https://hg.mozilla.org/mozilla-central/rev/35dfe0d44d83
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Comment 12

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

Updated

5 years ago
Depends on: 749658
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.
...although I'm still seeing "ASSERTION: Wrong bounds" when dragging tabs...
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.
Yes. Please fix!
Oh, I see that patch now :-)

Updated

5 years ago
Depends on: 750139
(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].
(In reply to neil@parkwaycc.co.uk 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....
You need to log in before you can comment on or make changes to this bug.