587318 - [d2d] TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bbox.xhtml | a.getBBox().height - got 19, expected 19.453125

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Looks like something might be up with our text measuring

Comment 2

[Jeff Muizelaar [:jrmuizel]]

This is happening because getBBox() is returning a different height for the bounding box of <text>y</text> than <text>a</text>. I'm not sure what the correct behavior here should be. Every browser I've tested returns matching heights except for IE9.

Comment 3

[Robert Longson [:longsonr]]

From the SVG specification, section 7.11
(http://www.w3.org/TR/SVG/coords.html#ObjectBoundingBox), the full
glyph cell should be accounted for:

"For 'text' elements, for the purposes of the bounding box calculation, each
glyph is treated as a separate graphics element. The calculations assume that
all glyphs occupy the full glyph cell. For example, for horizontal text, the
calculations assume that each glyph extends vertically to the full ascent and
descent values for the font."

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Here are the metrics differences from MeasureText in gdi and d2d
d2d: 
+	pos	{x=-58.000000000000000 y=-960.00000000000000 }	gfxPoint
+	size	{width=628.00000000000000 height=1245.0000000000000 }	gfxSize

gdi

+	pos	{x=-64.000000000000000 y=-960.00000000000000 }	gfxPoint
+	size	{width=640.00000000000000 height=1216.0000000000000 }	gfxSize

Comment 5

[Jeff Muizelaar [:jrmuizel]]

The problem here is that we are getting metrics that extend beyond the descent. I think this may also explain our intermittent failures as it seems we can get different extents based on NeedsGlyphExtents().

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> I think this may also explain our intermittent failures as it seems we can get
> different extents based on NeedsGlyphExtents().

I think this second part is wrong. I think NeedsGlyphExtents() is true in both the GDI and DWrite cases. Perhaps it has something to do with detail glyphs?

Comment 7

[Bas Schouten (:bas.schouten)]

This is caused by the fix to bug 568191. We need to think about how we're going to fix both.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Only pad horizontal glyph metrics

This mimics the behavior on GDI by only padding the horizontal metrics. It did not cause any new test failures on test runs on birch, and it fixes this mochitest failure.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 466094 [details] [diff] [review]
Only pad horizontal glyph metrics

Jonathan is probably a better reviewer for this.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Also, perhaps we should just switch to using a GlyphRunAnalysis as it should actually give us the information we want.

Comment 11

[John Daggett (:jtd)]

Comment on attachment 466094 [details] [diff] [review]
Only pad horizontal glyph metrics

I think this is probably okay but I'm still a little queasy, it seems odd that you would tweak the width but not the height.  Plus'ing for now.

Jonathan, can you double-check this and reopen if needed?

Comment 12

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/mozilla-central/rev/16aa8f0aa149

Comment 13

[Jonathan Kew [:jfkthame]]

I haven't done an updated build yet, but I suspect this will regress the rendering of attachment 447506 [details] (from bug 568191), which illustrated the need to pad cairo's glyph extents vertically as well as horizontally under directwrite. In particular, see lines 2 and 5 of that example.

AFAIK, we don't currently have a reftest that will detect this (though we could try to construct one), so from that point of view we can get away with the change for now, but it's not really satisfactory - the glyph clipping illustrated in attachment 447507 [details] (screenshot of attachment 447506 [details] without padding extents, before bug 568191 landed) is quite ugly.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to comment #13)
> I haven't done an updated build yet, but I suspect this will regress the
> rendering of attachment 447506 [details] (from bug 568191), which illustrated the need to
> pad cairo's glyph extents vertically as well as horizontally under directwrite.
> In particular, see lines 2 and 5 of that example.
> 
> AFAIK, we don't currently have a reftest that will detect this (though we could
> try to construct one), so from that point of view we can get away with the
> change for now, but it's not really satisfactory - the glyph clipping
> illustrated in attachment 447507 [details] (screenshot of attachment 447506 [details] without
> padding extents, before bug 568191 landed) is quite ugly.

Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite + GDI perhaps?

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to comment #14)
> (In reply to comment #13)
> > I haven't done an updated build yet, but I suspect this will regress the
> > rendering of attachment 447506 [details] [details] (from bug 568191), which illustrated the need to
> > pad cairo's glyph extents vertically as well as horizontally under directwrite.
> > In particular, see lines 2 and 5 of that example.
> > 
> > AFAIK, we don't currently have a reftest that will detect this (though we could
> > try to construct one), so from that point of view we can get away with the
> > change for now, but it's not really satisfactory - the glyph clipping
> > illustrated in attachment 447507 [details] [details] (screenshot of attachment 447506 [details] [details] without
> > padding extents, before bug 568191 landed) is quite ugly.
> 
> Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite
> + GDI perhaps?

Yes, it was. So maybe going to D2D will mitigate that issue.

Out of curiosity, have you checked whether this regresses any tests when running with DW+GDI? Or are we going to treat that case as (relatively) unimportant? I realize tinderbox isn't currently testing it, at least.

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I haven't done an updated build yet, but I suspect this will regress the
> > > rendering of attachment 447506 [details] [details] [details] (from bug 568191), which illustrated the need to
> > > pad cairo's glyph extents vertically as well as horizontally under directwrite.
> > > In particular, see lines 2 and 5 of that example.
> > > 
> > > AFAIK, we don't currently have a reftest that will detect this (though we could
> > > try to construct one), so from that point of view we can get away with the
> > > change for now, but it's not really satisfactory - the glyph clipping
> > > illustrated in attachment 447507 [details] [details] [details] (screenshot of attachment 447506 [details] [details] [details] without
> > > padding extents, before bug 568191 landed) is quite ugly.
> > 
> > Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite
> > + GDI perhaps?
> 
> Yes, it was. So maybe going to D2D will mitigate that issue.
> 
> Out of curiosity, have you checked whether this regresses any tests when
> running with DW+GDI? Or are we going to treat that case as (relatively)
> unimportant? I realize tinderbox isn't currently testing it, at least.

I haven't. I don't think it's unimportant at all, but I do think it can wait :). So as you say, relatively, I'd say it's unimportant.