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.

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 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).

(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 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.

(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....

Note

You need to
log in
before you can comment on or make changes to this bug.